zephyr: Correct heap cache management #4851
Conversation
andyross
requested review from
dbaluta,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
|
As with all my submissions, this is more suggestion than validated submission. The logic is correct, but I can test it only through driver initialization right now. Note that this needs a Zephyr tree with zephyrproject-rtos/zephyr#39191 applied |
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>
andyross
force-pushed
the
heap-cache-fix
branch
from
44ccce8 to
2b31d22
Compare
|
|
||
| #ifdef ENABLE_CACHED_HEAP | ||
| z_xtensa_cache_flush_inv(z_soc_cached_ptr(mem), | ||
| sys_heap_usable_size(h, mem)); |
There was a problem hiding this comment.
do I understand it correctly, that sys_heap_usable_size() returns the size of an allocated memory area, available to the user, excluding the preceding header, i.e. the same size, that the used has used to allocate that memory? Which means, cache of the header area isn't invalidated here? So, if that area is then reused, it can still be overwritten by a later cache eviction?
There was a problem hiding this comment.
The heap backend only ever uses the uncached mapping, and by construction it won't live in the same cache lines as a cached heap block (it's fixed here in this file to be an integer number of 64-byte cache lines).
It's true that there's still an opportunity for code somewhere to (1) allocate an uncached heap block (which has a header in the same cache line) and (2) convert it manually to a cached region, polluting cache lines on top of the heap metadata. But we really can't protect against that, it's just an API violation. Code that is using cached data needs to do it in dedicated lines always, by definition.
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Thanks @andyross ! The current code is broken and won't compile if ENABLE_CACHED_HEAP is actually defined (broken by recent changes to heap sizes). I'll post a follow-up PR and incude Andy your patch in the series. It seems the zephyr-side change was already merged.
|
Closing since #4857 which includes this fix is merged. |
5 participants
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_block_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