Repository navigation
[RFC] Direct use of cached / not-cached shared memory alias instead of coherent.h in case of buffers #8006
Description
As everybody knows, not coherent cache is a serious complication we must deal with.
There are 2 ways of share data between cores:
- Using non-cached alias - read/write directly from L2 RAM
- Using a cached alias and a mechanism of writeback/invalidation to ensure coherent data between cores
Currently Zephyr uses solution 1, SOF uses solution 2
Shared memory in general has a lot of advantages - it is easy to use, the code is "obvious correct" instead of "correct". The structures may be used by several cores at the same time (standard multithread rules for sharing data apply of course)
The question is - how much it is slower?
I made some measurements on MTL RVP - access to the data using several methods:
- not shared data. 2 ways were measured:
- cached memory ensuring cache hit
- using coherent.h in not-shared mode
- shared data - 3 ways were measured:
- not cached memory alias
- using coherent.h in shared mode
- manual invalidation before operations / writeback after operations
all results in CPU cycles
NOT SHARED MEMORY
| ID | desc | Cached (hit) | coherent.h (not shared) |
|---|---|---|---|
| 1 | read single dword | 30 | 45 |
| 2 | read double dword | 31 | 44 |
| 3 | read cacheline (16 dword) | 80 | 98 |
| 4 | read 64dwords | 226 | 257 |
| 5 | read 1024dword | 3132 | 3449 |
| 6 | read twice 1024 | 6234 | 6852 |
| 7 | write single dword | 30 | 44 |
| 8 | write double dword | 31 | 43 |
| 9 | write cacheline (16dword) | 84 | 43 |
| 10 | write 64 dwords | 195 | 211 |
| 11 | wrtite 1024 dwords | 2632 | 2748 |
| 12 | twice write 1024 dwords | 5535 | 5749 |
no surprise here - using of direct access to memory is faster
SHARED MEMORY
| ID | desc | uncahced | cached (with inv/wb) | coherent.h (shared) |
|---|---|---|---|---|
| 1 | read single dword | 35 | 38 | 119 |
| 2 | read double dword | 35 | 39 | 119 |
| 3 | read cacheline (16 dword) | 115 | 87 | 175 |
| 4 | read 64dwords | 344 | 234 | 336 |
| 5 | read 1024dword | 5034 | 3177 | 3575 |
| 6 | read twice 1024 | 10036 | 6279 | 6972 |
| 7 | write single dword | 32 | 37 | 121 |
| 8 | write double dword | 32 | 40 | 119 |
| 9 | write cacheline (16dword) | 76 | 81 | 172 |
| 10 | write 64 dwords | 200 | 208 | 302 |
| 11 | wrtite 1024 dwords | 2636 | 2749 | 2955 |
!!!!!!!!!!!!!!!!!! USING OF UNSHARED MEMORY ALIAS IS FASTER THAN COHERENT.H UNLESS WE WANT TO ACCESS MORE THAN 64 DWORDS!!!!!!!!!!!!!!!!!!
"64 dwords" means - less than 64 accesses from acquire till release. Probably 99% (or even 100%) of all accesses to structures like comp_buffer are like this.
I don't mean access to the audio samples in buffers - only to control structures common usage of buffer is:
sink_c = buffer_acquire(sink);
min_free_frames = MIN(min_free_frames, audio_stream_get_free_frames(&sink_c->stream));
buffer_release(sink_c);
in fact the worse case - 1 dword access
CONCLUSIONS
My proposition is to stop using coherent.h for comp_buffer/audio_stream, instead create those structures in shared (non-cached) or not-shared(cached) memory
Good news is that we do know if the buffer will be shared or not at allocation time. In current code marking a buffer as shared is performed by calling coherent_shared_thread, in my solution it would be by choosing proper memory alias
- cross core safety - exactly same as in current solution. Currently if a buffer would be used on several cores without marking it as shared the system will crash. Same in my approach - cached must be used on single core exclusively
- performance - better (!!!!) than currently
- in case of not shared buffers the gain of performance is obvoius
- even in case of shared buffers - probably 99% (or even 100%) of accesses will be smaller than 64 dword operations.
- shared buffers are rare in the system - currently 100% of buffers are not shared. Shared buffers will be used on cross core connected pipelines and in cross-core DP module ONLY.
- so - we do gain performance in every case
- code is WAY simpler and WAY more easy to develop/debug. That means - less cache related defect in the future (and those defects are extremely hard to track)
Of course the above don't apply to audio samples in buffers - access to sample streams must be cached with proper inv/writebacks. BUT currently buffer_acquire and buffer_release DO NOT do any cache releated operations on audio buffers themselves. So change I'm proposing does not affect it at all.
Activity
[-][RFC] Use not-cached shared memory alias instead of coherent.h in case of shared buffers[/-][+][RFC] Direct use of cached / not-cached shared memory alias instead of coherent.h in case of buffers[/+]marcinszkudlinski commentedon Aug 7, 2023
Correct me if I'm wrong - in IPC3 there's an IPC for buffer creation SOF_IPC_TPLG_BUFFER_NEW and at this moment there's no information about components the buffer will be bind to?
lgirdwood commentedon Aug 7, 2023
@marcinszkudlinski good data. Its interesting to see the difference between coherent shared and the wb/inv columns as the difference should be spinlock and some asserts.
Can you share your proposed new API where we have lockless buffer readers and writers.
Do we have any other (non buffer) cache shared data that can be changed to uncached (and made lockless where appropriate) ?
kv2019i commentedon Aug 7, 2023
Thanks @marcinszkudlinski for the data. Your conclusion on "less cache related defect in the future" is interesting given the coherent.h was added with the sole intent to reach a more robust infrastructure to manage cached/uncached usage (and to provide a more systematic approach to a family of bugs there was in early SOF releases). Back then we observed we had cached/uncached conversions in way too many places, and code that was originally "cache safe", got reused (by other people) in slightly different use-cases and different combinations of code, leading to subtle cache errors. You can see some of the original rationale in commit ada2595 (looping also @ranj063 and @lyakh to discussion), who've been debugging many of the pre-coherent.h functional bugs.
Now with that said, some overhead was expected (and cost of correctness and time saved in debug), but your numbers indicate some surprising high delta. Especially the non-shared case should match in performance. I wonder if you had the asserts enabled for the measurement (there's at least the CHECK_ATOMIC() check that will add a cost to every acquire/release).
OTOH, if we can improve the performance and come up with alternative way to combat the original problem (i.e. wide-spread misuse of cached_to_uncahced/uncached_to_cache operations around the codebase), I'm all for changing coherent.h and its use. FYI also to @btian1 who's been optimizing related code recently.
btian1 commentedon Aug 8, 2023
for me, I definitely support remove those cache/uncache operations, it makes SOF hard to read and understand, especially current APIs have so much versions(ipc3/ipc4/xtensa).
From performance, actually, our current module performance is still too high, we need spend big effort to make it better and better, if there is no acquire/release/invalidate/writeback for most of modules, then I definitely believe performance will be better.
Another topic is module peak mcps, still no rootcause, one big concern is those cache related operations, another is logging, once those operation are removed, peak mcps rootcause will be easier to find.
@marcinszkudlinski , could you share your sample code here? I want to learn something from your code, since I did not have a chance to touch zephyr yet.
marcinszkudlinski commentedon Aug 8, 2023
@kv2019i
The best approach is "use uncached for all data, cached for code". Nobody can deny that skipping cache is the easiest and 100% bug-proof way to solve all cache problems
This is exactly how Zephyr does it.
BUT - zephyr is SMP system, we have a perfect core separation, don't we? Threads, components, pipelines etc. are bind to single core and never moved from one to another.
That means - almost everything may be cached, as long as creation/usage/destruction is performed on the same core it is perfectly safe
All "cache sensitve" places are easy to identify:
and that's all. Those may either be uncached OR have smart cache operations. And again - unless there's a significant performance drop - the most easy and most bug-resistant way is to use it uncached
you say "use in diffent way" - that's why there MUST NOT be any extra "smart" cache operations in the code, 99% of the code must have a ready to use and safe pointers. The modules MUST NOT even know that the pointers are cached or uncached - this is up to queue/IDC/driver code
this way won't give any chance to "use diffrently"
lyakh commentedon Aug 14, 2023
Thanks @marcinszkudlinski for the data and the write-up! One interesting aspect of the data I noticed that I haven't seen being mentioned in the discussion yet - is a discrepancy between reading and writing, e.g. lines 5 and 11 represent the same test with the only difference being using reads instead of writes, and it looks like with (the most deterministic) uncached access reads are twice as slow as writes? Is some write buffering taking place there?
As for the coherent API use - as I started working with it, I immediately raised a concern, that it's very complex and therefore expensive in terms of developer time and error-prone, and potentially not even beneficial for performance. I also thought that simple uncached access to shared objects like buffer control structures would be both much simpler to work with and possibly not even slower. However we had cycle counts, needed for SRAM and for cache access and it has been estimated that using the coherent API would bring performance benefits. So, in general - yes, I absolutely agree that the coherent API is complex, expensive to maintain and error prone, and if it doesn't provide performance gains, we probably don't need it.
OTOH I think it would be useful to actually do some close to real use-case measurements: can we benchmark some simple capture or playback streaming with the coherent API, while forcing the
.sharedflag totrueand with uncached access tostruct comp_buffer?module: add pointers to sink/src to module structure
lgirdwood commentedon Aug 16, 2023
@marcinszkudlinski btw, is there a high level FEATURE tracking all these parts and dependecies ?
module: add pointers to sink/src to module structure
module: add pointers to sink/src to module structure
cujomalainey commentedon Sep 22, 2023
@marcinszkudlinski excellent experiment! Would you be able to share the code to do the test? I feel we should validate on other SoCs. That we can validate this is either a global truth or a subjective one to design.
cujomalainey commentedon Sep 22, 2023
And by other I mean @thesofproject/amd @thesofproject/nxp and @thesofproject/mediatek
lyakh commentedon Sep 25, 2023
@cujomalainey it was my impression that Intel DSP was the only architecture with incoherent caches, this my impression is based on the definitions by all other architectures:
In which case all the Intel cache-uncache handling should only be relevant for Intel?
lgirdwood commentedon Sep 29, 2023
@marcinszkudlinski I guess we are now finished with this update. i.e. all PRs now merged ?
marcinszkudlinski commentedon Sep 29, 2023
yes, task completed!