Had to drop crossover module, as drc changes have a dependency to it. It will part of the next module conversions PR.

Had to drop crossover module, as drc changes have a dependency to it. It will part of the next module conversions PR.

Hi. May I know what is the main reason for dropping crossover rather than converting its API, multiple outputs perhaps?
With difficulty of conversion, crossover can be dropped as there should be no application taking crossover as a component. That's said, part of functions of crossover are depended by multiband_drc which need to be retained.

Had to drop crossover module, as drc changes have a dependency to it. It will part of the next module conversions PR.

Hi. May I know what is the main reason for dropping crossover rather than converting its API, multiple outputs perhaps? With difficulty of conversion, crossover can be dropped as there should be no application taking crossover as a component. That's said, part of functions of crossover are depended by multiband_drc which need to be retained.

@johnylin76 , Crossover, Multiband DRC, and DRC have interdependencies and to avoid build breakage between the commits they need to be converted at once. They are converted here: #10289

(The other module conversion PRs are #10292 , #10293 , #10294, #10295 and #10296)

I'm not a fan of removing memory deallocation from the code. While I understand the idea of tracking memory allocated by a module to detect potential leaks and clean up unfreed memory, I don't think we should relieve modules from managing their own memory. This leads to discussions like: if function xxx returns an error, will the module be freed or not? And when our memory allocation strategy inevitably changes again in a year, are we going to reintroduce deallocation?
I believe optional tracking of allocated blocks is fine, but each module should still properly manage the memory it allocates.

I'm not a fan of removing memory deallocation from the code. While I understand the idea of tracking memory allocated by a module to detect potential leaks and clean up unfreed memory, I don't think we should relieve modules from managing their own memory. This leads to discussions like: if function xxx returns an error, will the module be freed or not? And when our memory allocation strategy inevitably changes again in a year, are we going to reintroduce deallocation? I believe optional tracking of allocated blocks is fine, but each module should still properly manage the memory it allocates.

@softwarecki It is optional, module can still call mod_alloc()/mod_free() operations as needed. The tracking has worked well on Linux as it simplified error handling code around memory resources since these are rarely validated code paths. It also allowed for easier Linux module code reviews as code complexity decreased.

You are right though, it should still be up to the module developer on what they prefer - they can still use manual mod_free() if they want.

Btw, I dont expect any module client level memory changes after this, the lower levels will become virtual regions at the pipeline/DP module level. See #10281

@lgirdwood
I agree that this simplifies things. However, as this PR shows, it also introduces new doubts like: do we need to free memory manually in this case, or will it be freed automatically? Additionally, we'll end up adding comments explaining why a specific mod_free is important and cannot be removed. In the classic approach, it's clear that everything must always be freed.

We shouldn't forget that SOF is an embedded solution running on limited resources, including memory. Linux has far more memory available. For this reason, I would personally prefer this functionality to be configurable in KConfig. That way, we can choose whether we want to track allocations made by a module and, if so, whether to use it for automatic memory cleanup or just as a debug feature to detect leaks.

Maybe we should simply split this into two stages: first, switch modules to use the new allocation mechanism, and once we see how it performs and know its memory overhead in practice, we can decide whether to keep it permanently, make it configurable, or eventually remove manual deallocation.

In any case, I wouldn't remove memory deallocation from existing code. If we decide to bring it back in the future, it will require a lot of work. As for flexibility, if someone creates a new module assuming memory bookkeeping will be active and memory will free itself, that's their choice.

The only thing this resource tracking does is that when a module is unloaded, whether it was due to an error or an unload request from the host side, all its resources are always freed. It is very useful in error handling situations and makes the code simpler and easier to manage. This is particularly true in init() functions where the module builds up the data structures it needs to do its job. This often leads to multi layered error handling code where memory leaks can easily happen.

What remains is that the developer will still have to understand what runtime allocated resources are still needed within the lifetime of the module and what are not. This problem is not a new one and it is not caused by the resource tracking, it is just something that is not solved by it. However, the tracking still makes sure that no resource leak remains after the module is unloaded.

But if this is too scary a jump to do now, I can put the the memory tracking code behind a config option, and keep the error branch resource freeing code, and free function code (I actually considered keeping the mod->free() contents my self, but decided to follow Linux devm model completely).

I just want to know if this is what everybody wants, and can agree to, before I go through all the module implementations once more again. So is this the way to go @softwarecki & @lgirdwood ?

edit:
Sorry, I can not put resource tracking code behind a config option because the modules dts, cadence, and waves, rely on the resource tracking (module_allocate_memory() that was recently renamed to mod_alloc()) and have been using that for years. However, I can restore the error clean up and module unload code to explicitly free everything like before. I'll make a new version of this PR shortly and and add volume module to it, so that there are more actively used and tested module converted in this PR.

Thanks for the detailed explanation. A few concerns from the embedded side:

The only thing this resource tracking does is that when a module is unloaded

The resource tracking is exercised not only at unload, but also on allocation and on manual frees. Even if allocation is relatively fast, freeing a buffer requires scanning the list of all tracked allocations, which makes free O(n). That is a non-trivial runtime cost.

Engineers will naturally lean on the convenience of automatic cleanup, but that convenience consumes extra RAM, CPU cycles, and energy. We already observe a noticeable CPU peak during pipeline startup; adding tracking overhead on the hot path will likely make this worse. Given this is an embedded system, I would prioritize performance and tight resource usage over convenience.

Since some modules (dts, cadence, waves) already depend on tracking, perhaps they could continue using module_allocate_memory explicitly. For the rest, it might be worth considering making mod_alloc configurable. Either as a raw allocator without tracking or with tracking, selectable via Kconfig. Modules not using tracking would retain their explicit error and unload cleanup paths. This could help maintain compatibility where it's needed, while avoiding a runtime cost across all modules, while still keeping the tracking feature available as a valuable tool for debugging.

@softwarecki & @lgirdwood
The list of reserved resources should never be too big as the list is module instance specific. Here, for example, are results from my new Zephyr shell command[1] while runninnig couple of streams[2] using nocodec topology on MTL RVP:

        comp id      containers used   high water mark (the module instance names are picked from Linux SOF driver log)
~$ sof module_container_usage
comp id 0x00000005        3 usage        3 hwm         (host-copier.0.playback)
comp id 0x0000000b        1 usage        1 hwm         (micsel.1.1)
comp id 0x00000007        3 usage        3 hwm         (gain.1.1)
comp id 0x00000003        1 usage        1 hwm         (mixin.1.1)
comp id 0x00000004        1 usage        1 hwm         (mixout.2.1)
comp id 0x00010007        3 usage        3 hwm         (gain.2.1)
comp id 0x00000011        0 usage        0 hwm         (smart_amp.2.1)*
comp id 0x00010005        2 usage        2 hwm         (dai-copier.SSP.NoCodec-0.playback)
comp id 0x00020005        2 usage        2 hwm         (dai-copier.SSP.NoCodec-2.capture)
comp id 0x00000009        5 usage        5 hwm         (src.11.1)
comp id 0x00030005        3 usage        3 hwm         (host-copier.2.capture)
comp id 0x00040005        3 usage        3 hwm         (host-copier.2.playback)
comp id 0x00020007        3 usage        3 hwm         (gain.5.1)
comp id 0x00010009        5 usage        5 hwm         (src.5.1)
comp id 0x00010003        1 usage        1 hwm         (mixin.5.1)
comp id 0x00010004        1 usage        1 hwm         (mixout.6.1)
comp id 0x00030007        3 usage        3 hwm         (gain.6.1)
comp id 0x00050005        2 usage        2 hwm         (dai-copier.SSP.NoCodec-2.playback)
~$

* smart-amp has never been converted to a module so it does not use module API either. Converting it should probably be on somebody's TODO-list. \me hides

As it can be seen here, the amount of tracked resources is usually well below 10. But if we ever have a module instance that reserves hundreds of tracked resource units, it should be straight forward enough to store the cointainers into a Zephyr red-black-tree[3] and make mod_free() O(log(n)) scalable, but with the current usage that should not be necessary.

Anyway, if this does not convince you, how about making the resource tracking per module configurable? E.g. there would be a function that should be called in module init before the first mod_alloc call (approriate safequards in place to avoid miss use) that turns the resource tracking on. Wihtout it the mod_alloc() and friends would not track resource usage and would only forward the calls to approriate backends. I could then turn the tracking on for the modules that currently need it, and rest would keep it off until we decide to turn the tracking on for them, either for good, or maybe just for debugging purposes.

[1] #10339
[2] aplay -Dhw:0,0 -f dat /dev/zero
aplay -Dhw:0,2 -d 10 wav/stereo_up_down_sweep.wav (a 44.1kHz wav file)
arecord -Dhw:0,2 -f S16_LE -c 2 -r 32000 -d 10 -vvv recording.wav
(The SSP2 pipelines were moved to core 0 to avoid cache issues when reporting container usage)
[3] zephyr/include/zephyr/sys/rb.h

Even if allocation is relatively fast, freeing a buffer requires scanning the list of all tracked allocations, which makes free O(n).

@softwarecki in fact automated clean up at module destruction time gives us a chance of a significant optimisation - just drop the whole module heap without freeing every allocation in it.

As I understand it, the shown usage is in containers, each holding 16 allocations. The log shows 42 containers in use - each is 392 bytes, so tracking costs us over 13 KB.

making the resource tracking per module configurable?

Seems like an acceptable solution to me.

just drop the whole module heap without freeing every allocation in it.

@lyakh In that case, we don't need tracking at all - there's no risk of memory leaks. Thats yet another argument in favor of making this solution configurable.

As I understand it, the shown usage is in containers, each holding 16 allocations. The log shows 42 containers in use - each is 392 bytes, so tracking costs us over 13 KB.

Each module instance allocatest (5 * sizeof(int) = ) 20 byte containers in 16 container chunks. One such allocation takes 20 * 16 + 2 * sizeof(int) = 328 bytes. The first chunk is enough for all the module instances in the above example. So tracking takes 328 bytes / module instance. In the above example there is 18 module instances so the consumed memory adds up to 18 * 328 = 5904 bytes, so bit under 6kb. (I think I should shrink the CONFIG_MODULE_MEMORY_API_CONTAINER_CHUNK_SIZE default to 12, so that a chunk would fit nicely to four cache-lines.)

making the resource tracking per module configurable?

Seems like an acceptable solution to me.

Ok, I'll prepare a PR to make that change.

These new versions of module conversions use module API to allocate the resources, but the code still has all the resource freeing code in place.

Fixed microphone privacy related compile error from copier, and rebased.

SOFCI TEST