Module api additions for cadence code and taking them into use #10366
Conversation
|
@jsarha I'd do one of the 2 things:
But again - nobody is doing that, even our |
For that I would at least need to know how to test it. And there probably was some reason why the mod_alloc() predecessor was taken into use here. But if someone can tell me how to test this thing, then sure, I should be able to do this.
I that would probably be added complexity that nobody would ever use. The two levels should be all that anybody would ever need (init-prepare-reset-free cycle). The point with two levels is that it is very close to the planned static/dynamic heap concept. It should be quite straight forward to drop all dynamic allocations, when we get there. |
| * call, so that all memory in prepare()/reset() cycle would | ||
| * be allocated from lvl2. | ||
| */ | ||
| mod_alloc_lvl2_init(mod); |
There was a problem hiding this comment.
We dont need the second level as I'm moving all module infra to use mod_alloc() see #10281 i.e. init code will be able to use mod_alloc().
There was a problem hiding this comment.
@lgirdwood , Ok. But that does not solve the issue with this cadence module. I guess I just have to follow @lyakh suggestion and implement manual freeing for reset() callback and just hope it works even if I can not test it myself.
Change all memory allocations to go through mod_alloc() and friends. To achieve this calling of mod_free_all() in reset()-callback has to end. Instead store all memory allocated by the codec library in a table and free the contents manually. Signed-off-by: Jyri Sarha <[email protected]>
jsarha
force-pushed
the
module_api_additions_for_cadence
branch
from
8bb496a to
b97d18d
Compare
|
@lyakh , @lgirdwood , Ok this version keeps book of the memory allocated in the prepare phase internally inside the module and frees it in reset() without any external help. Please review carefully, as I have no way to test this code. |
| if (scratch) | ||
| mod_free(mod, scratch); | ||
| if (persistent) | ||
| mod_free(mod, persistent); |
There was a problem hiding this comment.
looks like these two mod_free() calls were redundant: scratch and persistent are copies of ptr on the iteration, that failed, but this is called during the prepare() step, so then clean up would be performed and all module allocated memory would be freed anyway
| ret = -EINVAL; | ||
| goto err; | ||
| } | ||
| cd->mem_to_be_freed[i] = ptr; |
There was a problem hiding this comment.
You could call the array just mem_table or similar, but not so important
|
@dbaluta , could you give this PR a try before I mark it ready? Our CI does not test Cadence codec, and I have no idea how I could do it manually. |
|
No feedback from @dbaluta yet, but I mark this ready for review. |
There was a problem hiding this comment.
Pull Request Overview
This pull request replaces the previous allocation approach in the Cadence codec module with a more explicit memory management system that tracks allocations for later cleanup. The key changes implement tracking of memory tables to enable proper cleanup during reset/free operations, replacing the generic mod_free_all() approach with targeted memory management.
Key Changes:
- Added explicit tracking of allocated memory tables through new fields
mem_to_be_freedandmem_to_be_freed_len - Replaced
rballoc/rzalloc/rmalloc/rfreecalls with module-specific allocation APIs (mod_balloc/mod_zalloc/mod_alloc/mod_free) - Introduced
free_memory_tables()function to systematically free tracked allocations instead of usingmod_free_all()
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/include/sof/audio/module_adapter/module/cadence.h | Added two new fields to track memory allocations that need to be freed later |
| src/audio/module_adapter/module/cadence.c | Replaced allocation functions, implemented memory table tracking, and added explicit cleanup function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ret; | ||
| } | ||
|
|
||
| cd->mem_to_be_freed = mod_zalloc(mod, no_mem_tables * sizeof(*cd->mem_to_be_freed)); |
There was a problem hiding this comment.
Memory allocation for mem_to_be_freed occurs after memory tables are allocated, but if this allocation fails, the already-allocated memory tables will leak. This allocation should occur before the memory table loop, or the error path should be updated to free any already-allocated tables.
|
@jsarha allow me 1 day to test this. Will get back to you with results by Thursday |
@jsarha This looks like something new. Could you share more details about these plans? |
5 participants
This is a bit crude way to solve the issue with cadence codec's prepare() / reset() allocation cycle, but I did not want to implement anything more complex than what is needed. This concept also maps well on the comming static allocation vs. dynamic heap concept.