[RFC] Direct use of cached / not-cached shared memory alias instead of coherent.h in case of buffers

[marcinszkudlinski]

As everybody knows, not coherent cache is a serious complication we must deal with.

There are 2 ways of share data between cores:

1. Using non-cached alias - read/write directly from L2 RAM
2. 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:

1. not shared data. 2 ways were measured:

• cached memory ensuring cache hit
• using coherent.h in not-shared mode

2. 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

1. 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
2. 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

3. 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.

[marcinszkudlinski]

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]

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

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]

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]

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

• cross core queues for DP and connected pipelines
• IDC
• system operations like loading modules, memory drop to IMR
• DMA operations

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]

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 .shared flag to true and with uncached access to struct comp_buffer?