Add lock to mod resources tracking#10960
Conversation
Convert mod_data_blob_handler_new() to a Zephyr syscall following the same pattern as mod_alloc_ext(), mod_free(), and mod_fast_get(). This allows modules running in user mode to call the function through the syscall interface. The z_vrfy_ handler validates the processing_module pointer and its heap region before dispatching to the implementation. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
jsarha
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1,
plbossart and
ranj063
as code owners
There was a problem hiding this comment.
Pull request overview
This PR adds locking around module resource tracking in the module adapter to allow safe resource bookkeeping when allocations/frees can occur from multiple threads (e.g., IPC thread vs DP thread). It also removes the now-obsolete thread-check Kconfig option and adjusts the blob handler creation API to follow the Zephyr syscall pattern in full Zephyr application builds.
Changes:
- Add a
k_spinlocktostruct module_resourcesand use it to protect tracked resource allocation/free paths. - Convert
mod_data_blob_handler_new()to a__syscallAPI for full Zephyr application builds (withz_impl_/z_vrfy_and mrsh glue). - Remove
MODULE_MEMORY_API_DEBUGKconfig and the associated thread-check logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/include/sof/audio/module_adapter/module/generic.h |
Adds spinlock to module resource struct; updates blob handler API to syscall vs z_impl_ mapping depending on build. |
src/audio/module_adapter/module/generic.c |
Initializes and applies the new spinlock around most resource tracking operations; adds syscall verification/marshalling for blob handler creation. |
src/audio/module_adapter/Kconfig |
Removes the legacy thread-safety debug-check option. |
| struct module_resources *res = &mod->priv.resources; | ||
| struct module_resource *container; | ||
|
|
||
| MEM_API_CHECK_THREAD(res); | ||
|
|
||
| container = container_get(mod); | ||
| if (!container) |
| k_spin_unlock(&res->lock, key); | ||
|
|
||
| /* Make sure resource lists and accounting are reset */ | ||
| mod_resource_init(mod); |
| struct module_resources *res = &mod->priv.resources; | ||
|
|
||
| /* Init memory list */ | ||
| k_spinlock_init(&res->lock); |
There was a problem hiding this comment.
why a spinlock? Spinlocks disable interrupts. I'd really prefer a mutex if possible
There was a problem hiding this comment.
Well, the memory operations should be fast, but sure I can easily make it mutex.
|
|
||
| MEM_API_CHECK_THREAD(res); | ||
|
|
||
| key = k_spin_lock(&res->lock); |
There was a problem hiding this comment.
so, I suppose we should add locking everywhere where we currently call MEM_API_CHECK_THREAD(), so I've also missed mod_balloc_align(), right?
There was a problem hiding this comment.
Yep, Copilot pointed that out too, I am already doing that.
Convert mod_balloc_align() to a Zephyr syscall following the same pattern as mod_alloc_ext(), mod_free(), mod_fast_get(), and mod_data_blob_handler_new(). This allows modules running in user mode to allocate aligned buffer memory blocks through the syscall interface. The z_vrfy_ handler validates the processing_module pointer and its heap region before dispatching to the implementation. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Add a k_mutex to struct module_resources and take it around every objpool access in z_impl_mod_balloc_align(), z_impl_mod_alloc_ext(), z_impl_mod_data_blob_handler_new(), z_impl_mod_fast_get(), z_impl_mod_free(), and mod_free_all(). This prevents concurrent access to the resource tracking pool when a module's IPC thread and processing thread operate in parallel (e.g. set_large_config arriving while the module is running). In mod_free_all() the mutex is released before mod_resource_init() re-initializes it, so the lock is not held across re-initialization. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Remove the MEM_API_CHECK_THREAD() macro, CONFIG_MODULE_MEMORY_API_DEBUG Kconfig option, and the rsrc_mngr field from struct module_resources. This single-thread-owner check is now redundant: the previous commit added a spinlock that properly serializes all accesses to the resource pool, making the debug-only thread identity warning unnecessary. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
jsarha
force-pushed
the
add_lock_to_mod_resources_tracking
branch
from
c2d0ff2 to
1504a2c
Compare
|
@lyakh comments should addressed now. |
|
|
||
| #if CONFIG_COMP_BLOB | ||
| struct comp_data_blob_handler *mod_data_blob_handler_new(struct processing_module *mod); | ||
| #if defined(__ZEPHYR__) && defined(CONFIG_SOF_FULL_ZEPHYR_APPLICATION) |
There was a problem hiding this comment.
not for this PR, but maybe we can drop globally the first part from these checks - I think we have a few of them across the code base
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Code looks good. The only thing I'm thinking is whether this is really needed, but going through the discussion in #10862 helps to provide context. So strictly not mandatory with current configs, but given this can be used from DP modules, bit of defensive programming on the slow paths makes sense here.
| @@ -13,17 +13,6 @@ menu "Processing modules" | |||
| containers to allocate at once is selected by this | |||
This PR is a result from the discussion in this PR: #10862