Skip to content

zephyr: all allocations from heap must be aligned to cacheline #7322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
marcinszkudlinski wants to merge 1 commit into from
Closed

zephyr: all allocations from heap must be aligned to cacheline #7322

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions zephyr/lib/alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,20 @@ static void *heap_alloc_aligned(struct k_heap *h, size_t min_align, size_t bytes
struct sys_memory_stats stats;
#endif

/*

@kv2019i kv2019i Mar 22, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcinszkudlinski Can you explain a bit more how cache inv on secondary core will lead to issues? You can't get multiple allocations via rmalloc to same cacheline.

The change might still be ok given we anyways pass PLATFORM_DCACHE_ALIGN to non-cached allocs (only difference is that we don't pad-up the alloc size).

Only scenario I can come up with if have heap blocks ABC (legend: each letter is a 64 byte cacheline). So if you do a non-cached SOF allocation for 16 payload, that will be allocated with "heap_alloc_aligned(h, 64, 16)". Zephyr will use cacheline A for metadata and B for allocation.

Now, it is possible if you next allocated another 16 byte, the free 48 bytes of B are used for Zephyr heap metadata for next allocation, so metadata will be writtne to B and payload (buffer passed to SOF app code) in block C.

I can see this PR to modify the behaviour such that allocation size is padded to full 64 bytes and there will never be any free space at the end of the allocation (that could be used for heap metadata).

But given these are uncached allocs, I don't see how the invalidation is a problem here. E.g. the cacheline B that has mixed data, is used noncached by all parties so there's no coherency issue.

@lyakh One concern I have is __coherent_init(). That is allocating a shared buffer, but casts it to cached buffer and does cache management ops. We did have a PR to pad the coherent struct, but that was not merged. I think we do need to pad the allocation size to a full multiple of cacheline size. Otherwise we run risk that on the last memory block of the coherent object, sys_heap might put metadata on the same block.

@marcinszkudlinski marcinszkudlinski Mar 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

problem:
cacheline is 64bytes long
zephyr puts metadata (8 bytes? no matter, lets assume 8) in front of allocated region

lets assume

  1. allocate 16 bytes (from cached area) by core 1
    from 0 to 7 - metadata (zepyr does use uncached alias __section(".heap_mem") is in uncached area)
    from 8 to 23 - data - translated to cached alias

  2. core 1 is making changes in bytes 8-23, at the moment in cache only

  3. allocate 8 bytes (from cached area) by core 2, as before
    from 24 to 31 - metadata
    from 32 - 40 - data

  4. for any reason core 1 writes back data to RAM from the cache. whole cacheline is written, heap metadata 23-41 is lost

that exactly what was happening during my debug of the defect I mentioned

to avoid this there's a trick - always keep 1 separate cacheline between each allocation - just for metadata. It is done by aligning both start and length of every single allocation to cacheline size

I,m not an author of this, I'm not claiming the algorithm is perfect On contrary, I'm considering it as a dirty trick with enormous memory waste. Anyway, at the moment it is as it is.
This PR is not trying to make it better, it is just making the trick work as it was design to work. And we need it quick because its affecting every multicore operation.

@kv2019i kv2019i Mar 22, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcinszkudlinski Ack, but that's exactly why heap_alloc_aligned_cached() pads the alloc size to a full cachelline. So if you see this in debug, something in the code doing the allocation is wrong. E.g. which code does the alloc in step 1. This is clearly wrong if you allocate uncached memory and then cast to cached alias. All bets are off at this point. Only exception should be coherent.h (and if that is buggy, that should be fixed).

@lyakh lyakh Mar 22, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern I have is __coherent_init(). That is allocating a shared buffer, but casts it to cached buffer and does cache management ops. We did have a PR to pad the coherent struct, but that was not merged. I think we do need to pad the allocation size to a full multiple of cacheline size. Otherwise we run risk that on the last memory block of the coherent object, sys_heap might put metadata on the same block.

@kv2019i We know about that possibility and that was why we moved allocation inside coherent_init(_thread)() but... As I was writing that I decided to double-check and duh :-( #7323

@marcinszkudlinski marcinszkudlinski Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the allocation that crashes is:

static inline void *__coherent_init_thread(size_t offset, const size_t size)
{
void *object = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, size);

the heap metadata is corrupted at some point of operaion

@kv2019i kv2019i Mar 22, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, thanks @marcinszkudlinski . So we need #7323 as urgent fix.

@marcinszkudlinski marcinszkudlinski Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, it is doing basically the same as my fix than, just at other point.
Mine is safer (more tolerant to mistakes), yours - more memory friendly
Up to you what is to be closed

Anyway - we DO need to make heap better, there's a lot of small allocations and using 2 cachelines for most of them is something we simply cannot afford
FYI: @softwarecki @dabekjakub

* Zephyr sys_heap stores metadata at start of each
* heap allocation. To ensure no allocated cached buffer
* overlaps the same cacheline with the metadata chunk,
* align both allocation start and size of allocation
* to cacheline. As cached and non-cached allocations are
* mixed, same rules need to be followed for both type of
* allocations.
*/
#ifdef CONFIG_SOF_ZEPHYR_HEAP_CACHED
min_align = MAX(PLATFORM_DCACHE_ALIGN, min_align);
bytes = ALIGN_UP(bytes, min_align);
#endif

key = k_spin_lock(&h->lock);
ret = sys_heap_aligned_alloc(&h->heap, min_align, bytes);
k_spin_unlock(&h->lock, key);
Expand All @@ -172,20 +186,6 @@ static void __sparse_cache *heap_alloc_aligned_cached(struct k_heap *h,
{
void __sparse_cache *ptr;

/*
* Zephyr sys_heap stores metadata at start of each
* heap allocation. To ensure no allocated cached buffer
* overlaps the same cacheline with the metadata chunk,
* align both allocation start and size of allocation
* to cacheline. As cached and non-cached allocations are
* mixed, same rules need to be followed for both type of
* allocations.
*/
#ifdef CONFIG_SOF_ZEPHYR_HEAP_CACHED
min_align = MAX(PLATFORM_DCACHE_ALIGN, min_align);
bytes = ALIGN_UP(bytes, min_align);
#endif

ptr = (__sparse_force void __sparse_cache *)heap_alloc_aligned(h, min_align, bytes);

#ifdef CONFIG_SOF_ZEPHYR_HEAP_CACHED
Expand Down