[marcinszkudlinski]

There was a possible problems witch cache coherency when allocating memory from "uncached" zone. I.e. with SOF_MEM_ZONE_RUNTIME_SHARED flag set. In that case heap_alloc_aligned was called directly and alignment was not set.

if (zone_is_cached(zone) && !(flags & SOF_MEM_FLAG_COHERENT)) {
	ptr = (__sparse_force void *)heap_alloc_aligned_cached(heap, 0, bytes);
} else {
	ptr = heap_alloc_aligned(heap, PLATFORM_DCACHE_ALIGN, bytes);
}

Any following cache invalidation on secondary cores may lead to issues.

[lyakh]

Nno, I don't think this is correct. If memory is allocated as uncached it shouldn't be used with cache, so cache invalidation makes no sense. And if an adjacent memory area is allocated in cached more - that allocation will occupy complete cache lines, so it will not share any cache lines with other allocations - cached or uncached. If a memory is allocated to be used in cached mode on multiple cores, it should use the coherent API, which also takes care to synchronise caches correctly. Bug are certainly possible and if we find any we should fix them, but so far no such bugs are at least known to me.

[kv2019i]

Looking for a bit more detail on how this leads to problems, see comment inline.

[kv2019i]

@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]

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]

@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]

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]

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]

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

[marcinszkudlinski]

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

[marcinszkudlinski]

closed because of #7323