memory management: Add virtual heap memory allocators

[dabekjakub]

Add virtual heap allocators that allocate on proper heaps and map physical memory where nesscessary.
Add free function that frees up the virtual heap memory and unmaps physical memory when possible.

Some relevant links to understand context
https://thesofproject.github.io/latest/architectures/firmware/sof-zephyr/rtos_layer/memory_management/heap_sharing.html
https://thesofproject.github.io/latest/architectures/firmware/sof-zephyr/rtos_layer/memory_management/index.html

https://github.com/orgs/thesofproject/discussions/7514

Added a presentation that outlines the intent of the commit I crafted at the start of this commit lifecycle.
virtual heaps.pdf

Summarizing potential benefits that this api introduces:

• Avoid wasting cache line size that happens currently with almost all allocations that are single core
• Improves cache coherency handling by redirecting traffic from separate core to separate heaps
• Powering up and down of memory banks is dynamic and lets us save power if done efficiently
• Allows easy mapping for user-space heap allocations
• Allows for Opportunistic allocator to be created "paging file"
• Virtual memory aggregation across cores – can ease the process of debugging in compare to using zones and single system heap
• Flexibility when defining zones by clients
• It is aligned to better serve future platform memory architecture

[marcinszkudlinski]

tip: add pre-commit hook to your local git repo and run checkpatch at every commit. Sometimes its annoying, but helps

      #!/bin/sh
      set -e exec
      exec git diff --cached | (...your patch..._)/scripts/checkpatch.pl -

[dabekjakub]

I knew there were issues in check patch and I intended to fix theme before changing it to full PR. I made this draft so we can discuss the arch approach not the cosmetics.

[marcinszkudlinski]

More review is coming. This is a "classic", later I'll focus on the main code

ANYWAY - please use checkpatch even in draft PRs. Pls add comments.
It makes a revew - any review, even draft - easier and faster

[lyakh]

looks like not all comments have been addressed yet.

[lyakh]

@dabekjakub please use #7688 to test this PR, make sure it passes (currently it doesn't. hint: default configuration alignment test fails). Feel free to add more tests to it or I'll do it myself once the first test passes.

[dabekjakub]

@lyakh i have My own tests that i will use. We talked offline about it and agreed that we will merge it after internal tests were passed. If You know what is wrong please do tell but I cannot switch testing procedures, especialy when I do not understand at first glance what You did in #7688 Also i noted that it is not fully tested yet so problems can yet surrface.

[lyakh]

@lyakh i have My own tests that i will use. We talked offline about it and agreed that we will merge it after internal tests were passed. If You know what is wrong please do tell but I cannot switch testing procedures, especialy when I do not understand at first glance what You did in #7688 Also i noted that it is not fully tested yet so problems can yet surrface.

@dabekjakub I remember a different decision. I think the decision was to commit this your work together with a test. And especially now that we know that a most primitive test fails, I don't think we can merge this without reasonable tests.

[dabekjakub]

Took time to look at Your test. It is basic scenario of heap creation. In my tests I have multiple heaps and multiple allocations. I will stick to my testing. And yes i observe the same issue.

[kv2019i]

@dabekjakub I don't think you need to use Guennadi's tests, but you noted there's no infra to add tests and #7688 now provides that. So please do consider that. This will enable you to push the tests to upstream as well, which again is useful.

[dabekjakub]

We were supposed to talk about how that infrastructure gonna look like too. There were supposed to be discussion about how we enable runtime testing mode.
Right now i have a test that covers most of basic cases and tries some advanced.
I do not share it simply because it looks bad with a lot of memory prints etc but it gets the job done.

[dabekjakub]

Current code has required changes @lyakh @mmaka1 that we agreed upon and I consider it final review ready.
I have run a test which involved full scenario of multiple heaps and allocations and that passed. However there must be full test coverage introduced after merge and before use in production scenarios. Of course after we agree upon the way that we want to do it - I will open separate discussion to work it out .
PS:
I am working out some sort of fail at the heap free in multiple iterations - could be test issue thought.

[kv2019i]

The volume of comments is good. The second bit array is a nice concept, although it bugs me a bit to have one array on Zephyr side and one here in SOF code, but if that's the split now then it is what it is. Still this is very complex code to review. Some comments inline, but run out of time trying to understand all bits of the alloc/free paths.

[dabekjakub]

Tested with asserts on @lyakh I want to talk with @lgirdwood offilne to be sure on the fix for multi thread but it is good to test if You want to do so.

[lyakh]

Tested with asserts on @lyakh I want to talk with @lgirdwood offilne to be sure on the fix for multi thread but it is good to test if You want to do so.

so have you cleaned up all the asserts? None are triggering any more?

[dabekjakub]

@lyakh Run my basic scenarios Create heap/ alloc / alloc /free / free / free heap. And it worked with asserts on.

[lyakh]

Still not a complete review, just your changes from the previous version, but I haven't finished reviewing the previous version, so I'll have to do that. But please also fix gcc compilation breakage e.g. https://github.com/thesofproject/sof/actions/runs/5659675012/job/15333903191?pr=7276

[lyakh]

this is now an "uncached allocation" - is this on purpose? All the allocations are now uncached, I thought it was an important aspect of this allocator to use cached access? Or you're converting addresses somewhere? Haven't checked yet

[dabekjakub]

Yes, it was needed to change adresses of spin locks in bittarays to uncached. And i decided not to split allocation for parts of struct. The Struct data that we are talking about here should not generate big overhead and the changes only apply to information about heap and its allocations. The buffer itself is still provided by zephyr defines and the allocated ptrs from this buffer (allocated using this api) and this structure that saves heap info are separate.

[lyakh]

can we just make it

	size_t bitarray_size = SOF_DIV_ROUND_UP(cfg->block_bundles_table[i].number_of_blocks, 8 * sizeof(uint32_t));

because if .number_of_blocks + 31 > 0xffffffff I don't think this will work anyway.

[dabekjakub]

I would like to stay as true to the implementation that already exists in zephyr for calculation of the size. It is complicated as is would not like to convert it anymore since this will be easier for people to understand when they cross reference it with sys_bittaray implementation.

[lyakh]

please, also check thread-locality and thread context

[dabekjakub]

Disagree with handling anything about threads here. It should be under the managment code that handles the threads.

[lyakh]

sorry, don't understand what you mean, what code that handles the threads? Is this code handling multiple cores? This is a function that has its limitations: it should run only on a specific core and only in specific thread context, so it should check all the conditions.

[dabekjakub]

As far as I understand it this should not be handling threads in any capacity and it is management code that does it. @mmaka1 didn't we agree upon it on the syncs ?

[lyakh]

please, check the definition of SOF_DIV_ROUND_UP() - it's doing a division, the only way it can overflow (which is presumably what you're protecting against by typecasting to 64 bits), is from the addition operation. And we really shouldn't be allocating anything where alloc_size + block_size - 1 > 0xffffffff

[dabekjakub]

So what really is Your point here ? You want to remove the protection ? I know that we will not be allocating sizes like that but who knows what will we use it for later. I talked it over with @tlissows and we came to conclusion that if it does not hurt anyone and makes this code safer then it should be there.

[lyakh]

I wouldn't use 64-bit types. Type-casts are generally a rather crude instrument and should be avoided where possible, and here I don't see a need for them, and it causes a significant overhead - a 64-bit division.

[dabekjakub]

And I want it to be safe this way. The overhead seems negligible. And casts are tools to be used.

[lyakh]

This version passed the previous version of my basic test in #7688 - that's already good. I then went ahead and added a call to vmh_alloc() to it - and it caused an exception. The exception is happening inside vmh_alloc(). I added some debugging there (see patch attached)
04.gitdiff.txt
and got this output:

[    0.524288] <err> vmh: vmh_alloc: error -12 allocating 26 of 3264 of size 64
[    0.524311] <err> vmh: vmh_alloc: error -12 allocating 13 of 1632 of size 128
[    0.524316] <err> vmh: vmh_alloc: error -12 allocating 7 of 816 of size 256
[    0.524323] <err> vmh: vmh_alloc: error -12 allocating 4 of 408 of size 512
[    0.524330] <err> vmh: vmh_alloc: error -12 allocating 2 of 204 of size 1024
[    0.524336] <err> vmh: vmh_alloc: NULL allocator #5
[    0.524341] <inf> boot_test: sof_boot_test: test 0xa007b9d0 returned -12

i.e. the allocator fails to allocate memory in the 5 blocks of the heap - which is wrong, because there seems to be plenty of free memory in all of them - this is the first allocation attempt, and having failed those 5 blocks it attempts to use the 6th block, which is apparently not present, so it hits a NULL dereference.

[lyakh]

The new version fixed the crash. Now the new version of #7688 produces these errors:

[    0.523508] <wrn> vmhtest: vmh_test_single: Ignoring failure to allocate 1616 in non-contiguous mode
[    0.524056] <err> vmhtest: vmh_test_single: Free error -14
[    0.524065] <err> vmhtest: vmh_test_single: Free heap error -90
[    0.524071] <err> vmhtest: vmh_test: Contiguous test error -14
[    0.524078] <inf> boot_test: sof_boot_test: test 0xa007bad0 returned -14

i.e. vmh_free() in line 57 of vmh.c returns an error.

[lyakh]

we have been discussing ways to reduce the number of heap allocations in SOF, and here we have an example of a single object creation process using multiple such allocations. Maybe we could calculate a size and allocate the whole memory in 1 go. Can be a follow-up optimisation

[dabekjakub]

Yea I was exploring that idea myself. Agreed As a followup.

[lyakh]

so for each new heap we do 5 * block-number + 1 heap allocations... That's really too many

[dabekjakub]

Agreed that this could be followed up later.

[lyakh]

in principle we need 3 values, i.e. 2 bits per block: free, used-single-or-last, used-linked-to-next. So in theory we'd need an array of 2-bit values, but that would be difficult to work with

[dabekjakub]

You have two bits per block now. One bit is taken from the mem_block bitarray that saves information if it is allocated or not and the second bittaray is saved in heap struct that saves information if the block is linked to the next one. Aggregating both these arrays You end up with all information You need. I think that I explained it clearly in the in code comments.

[lyakh]

yes, sorry, it's me who didn't explain it clearly in my comment. I just meant that those two bitarrays actually represent an array of 2-bit fields, just in a slightly cumbersome and non-obvious way.

[dabekjakub]

So what is the point of this comment ? Do You want any changes here ? I do not understand.

[lyakh]

same line, please

[lyakh]

If I'm right about ptr about needing an array instead of it, then this is wrong too, and the following code as well. I'll stop here.

[dabekjakub]

same line produces warnings in checkpath

[dabekjakub]

Did I miss some comment here ? Because i do not understand What should You be right about here ?

[lyakh]

what are use-cases when we must have contiguous physical pages? DMA? Or would DMA also use virtual addresses and thus only care about virtual contiguity?

[dabekjakub]

This has nothing to do with contiguous physical pages.
This is firstly checking if we have virtual blocks of sufficient size by allocating on memblocks. If allocation was successful we allocate physical pages if needed.
And the answer is we do not need it and I do not force anything like that here.

[lyakh]

ah, ok, so we just map them into a contiguous address region, but physical pages can be discontiguous. Maybe it would be good to clarify that somewhere in a comment

[dabekjakub]

When this gets in, one of the next steps would be documenting it. I think adding anything about span physical mapping would only cause confusion here since there is a clear distinction in wording - allocation / mapping.

[lyakh]

can we make a function vmh_is_page_mapped() and just call it for each physical page? Eventually for contiguous allocators you can just check the first and the last page, but that can be optimised later.

[dabekjakub]

I would refactor it after the implementation of zephyr physical memory counting - this will simplify this implementation and make it much shorter.

[dabekjakub]

@lyakh I was unable to reproduce the issue you present with my latest changes.- Haven't explicitly fix any bugs but still changes were made.

[lgirdwood]

@lyakh I was unable to reproduce the issue you present with my latest changes.- Haven't explicitly fix any bugs but still changes were made.

ok, we can extend the UTs after merge and as clients are added.

[lgirdwood]

SOFCI TEST