zephyr: reimplement cached heap zone on single Zephyr sys_heap #4857
Conversation
kv2019i
requested review from
andyross,
iuliana-prodan,
keyonjie,
lgirdwood and
lyakh
zephyr/wrapper.c
Outdated
There was a problem hiding this comment.
This isn't needed for uncached allocations. Or rather, it's needed if some other layer is going to convert the pointer to a cached pointer, but IMHO that's a bug that should be fixed elsewhere. As long as the memory is used in the uncached mapping there's no possible pollution of cache lines that can occur.
There was a problem hiding this comment.
Perhaps we have to "virtually ban" the use of uncache_to_cache(). Currently there are only 4 uses of it in the entire SOF tree. 3 of them seem ok, of which one is below, replaced by z_soc_cached_ptr() by this patch. 1 of them seems to be exactly the case, that @andyross is talking about - in _balloc_unlocked(). So, any call to rballoc() without the SOF_MEM_FLAG_COHERENT flag set or in single-core configurations, would be such a case?
There was a problem hiding this comment.
@andyross The scenario I was worried about was the sys_heap metadata. So without this, uncached allocations can be put on the same cacheline as the metadata of a cached allocation. But, but, you are right that this itself is not a problem as sys_heap never touches the metadata via a cached pointer.
Let me revise the patch.
UPDATE: hmm, without this additional alignment for uncached, I'm getting systematic failures in our multiple-pause-resume SOF test. Will continue to debug this.
UPDATE2: found it. it seems XTOS is doing this alignment for other reasons and app code is expecting this alignment. Added a separate patch and change the comment.
zephyr/wrapper.c
Outdated
There was a problem hiding this comment.
Your PR will go in first probably, so I can update.
zephyr/wrapper.c
Outdated
There was a problem hiding this comment.
Perhaps we have to "virtually ban" the use of uncache_to_cache(). Currently there are only 4 uses of it in the entire SOF tree. 3 of them seem ok, of which one is below, replaced by z_soc_cached_ptr() by this patch. 1 of them seems to be exactly the case, that @andyross is talking about - in _balloc_unlocked(). So, any call to rballoc() without the SOF_MEM_FLAG_COHERENT flag set or in single-core configurations, would be such a case?
kv2019i
force-pushed
the
topic/z-cachedheap
branch
from
809b7d5 to
ada16fe
Compare
kv2019i
requested review from
dbaluta,
lbetlej,
mmaka1 and
plbossart
as code owners
kv2019i
force-pushed
the
topic/z-cachedheap
branch
from
ada16fe to
1c28e05
Compare
|
Fixed one checkpatch warning, no other changes in the new revision I just pushed. |
|
Checkpatch complains about Kconfig documentation, but there is documentation, so I'd argue this can be skipped. |
kv2019i
force-pushed
the
topic/z-cachedheap
branch
from
1c28e05 to
04311e7
Compare
andyross
left a comment
andyross
left a comment
There was a problem hiding this comment.
This all looks good to me. I do worry about the seeming mixing of cached/uncached blocks at a higher level that this is exposing, but the code here looks correctly conservative.
The heap code was invalidating blocks on allocation, but that's the wrong side of the pipe. By definition, new heap memory will/should/must be written before being used (because the memory is undefined), so any cached contents are irrelevant as they'll be overwritten. But when the user is finished with the block and frees it, there may still be live dirty cache lines in the region on the current CPU. Those must be invalidated, otherwise they will be evicted from the cache at some point in the future, on top of the memory region now being used for different purposes on another CPU. Remove the invalidate on allocation. Add it back in free. Leverage a new Zephyr sys_heap_usable_size() routine to get the size so we don't have to store it in an extra header. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
kv2019i
force-pushed
the
topic/z-cachedheap
branch
from
04311e7 to
49e0ffa
Compare
Implement the cached SOF heap zones using a single sys_heap object. Compared to old implementation which used two separate sys_heaps, this implementation enables dynamic cached/uncached partitioning of the heap. To ensure coherency of the Zephyr heap operation, if cached heap is enabled, all allocations must be aligned to dcache linesize. Add a Kconfig option CONFIG_SOF_ZEPHYR_HEAP_CACHED to turn on/off the cached heap logic, with default set to no for all platforms. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Set default as yes for SOF_ZEPHYR_HEAP_CACHED for Intel cAVS. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
The XTOS allocator aligns non-cachec allocations to the platform cacheline size. Test results indicate some SOF application code relies on this behaviour, so align Zephyr's rmalloc implementation to match the behaviour. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
kv2019i
force-pushed
the
topic/z-cachedheap
branch
from
49e0ffa to
e01d8c9
Compare
|
Pushed a new version to address a compiler error for imx build. The on-device failures should be unrelated as no on-device tests are done for Zephyr and this touches wrapper.c only. @dbaluta @iuliana-prodan @paulstelian97 I can't test the IMX Zephyr builds. but this should be ok (keep the pre-patch behaviour with non-cached heap on IMX). |
|
CI showing known DMA suspend timeouts on xtos builds. Unrelated. |
4 participants
Take Andy's #4851 and re-implement the cached heap.
The old implementation doesn't even compile anymore, so no need to merge 4851 separately.
This requires more testing, but sharing for early reviews.