Re: [vdr] [PATCH] Add thread safety to cRingBufferLinear
Tue, Feb 07, 2023 at 12:54:16AM +0100, Udo Richter wrote: Two-ended buffers are pretty good when used correctly, but nowadays they have a small chance of triggering memory ordering issues, where it is possible that written data to the buffer is still stuck in a distant cache, while the updated head pointer is already known to other CPU cores. To be absolutely safe, the head pointer would need atomic read/write, forcing ordering with a memory barrier. A nice explanation. Before C++11 (ISO/IEC 14882:2011), to my understanding, multi-threaded programs or data races were not part of the standard. Starting with C++11, data races actually are undefined behavior, and not just "implementation defined". I would recommend the following resources: https://en.cppreference.com/w/cpp/atomic/memory_order https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html The latter only covers loads and stores, not read-modify-write like std::atomic::fetch_add(), but it gives an idea how things look like in different processors. But in general the timing for such a bug is pretty narrow, and advanced CPUs (esp. intel/amd) make sure that things get stored in order anyway. With the IA-32 and AMD64 a.k.a. x86-64 ISA, the general expectation is that implementations use Total Store Order (TSO). This is also how the SPARC and the RISC-V RVTSO works. However, on ARM (and POWER and RISC-V RVWMO), weak memory ordering is the norm. On the ARM, some atomic operations would use retry loops, or with the recent LSE (Large System Extensions), special instructions. It should be remembered that there are VDR systems running on ARM, like the Raspberry Pi. Worst thing would also just be false data read, not a crash. If that false data were a stale pointer that points to now-freed memory, you could get a crash. Or you could get a hang or an assertion failure catching an inconsistent state. Somewhat related to this, I recently learned about Linux "restartable sequences" https://lwn.net/Articles/883104/ which could be an alternative to using mutexes in some lock-free data structures. I think it could work for things where data would be "published" by updating just one pointer at the end. I didn't think of an application of that in VDR yet, and I am not sure if there is any. It is better to keep things simple if the performance is acceptable. Marko ___ vdr mailing list vdr@linuxtv.org https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
Re: [vdr] [PATCH] Add thread safety to cRingBufferLinear
On 06.02.23 23:29, Klaus Schmidinger wrote: It is supposed to be shared by *exactly* two threads. One only writing 'head', the other only writing 'tail'. Two-ended buffers are pretty good when used correctly, but nowadays they have a small chance of triggering memory ordering issues, where it is possible that written data to the buffer is still stuck in a distant cache, while the updated head pointer is already known to other CPU cores. To be absolutely safe, the head pointer would need atomic read/write, forcing ordering with a memory barrier. But in general the timing for such a bug is pretty narrow, and advanced CPUs (esp. intel/amd) make sure that things get stored in order anyway. Worst thing would also just be false data read, not a crash. Cheers, Udo ___ vdr mailing list vdr@linuxtv.org https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
Re: [vdr] [PATCH] Add thread safety to cRingBufferLinear
On 06.02.23 21:11, Patrick Lerda wrote: On 03/02/2023 10:36, Klaus Schmidinger wrote: On 02.02.23 23:47, patrick9...@free.fr wrote: On 02/02/2023 23:27, Klaus Schmidinger wrote: On 02.02.23 23:16, Patrick Lerda wrote: Beside preventing crashes with vdr-2.6.3 this is required to get vdr to work properly with the gcc thread sanitizer. cRingBufferLinear was designed to be thread safe without locking. What "crashes with vdr-2.6.3" are you referring to? Klaus With a -fsanitize=thread compiled version of vdr, I had some crashes that happened quickly, for instance: ... Before making such deep changes to code that has been running flawlessly for years (or even decades) I need to be convinced that this is absolutely necessary. Is there a problem that occurs if you run VDR *without* -fsanitize=thread? Klaus I had in the past a crash from time to time, with vdr-2.6.3 this seems to be worse. Anyway, I was checking with vdr-2.4.7 and the problem is the same. This class is shared by at least 2 threads It is supposed to be shared by *exactly* two threads. One only writing 'head', the other only writing 'tail'. The concept was taken from the original DVB driver, which also implemented the ring buffer without locking. with more than one shared object; this means that without a mutex, the behavior is undefined from a C++ perspective. With -fsanitize=thread the compiler could add some jitter and that seems to trigger quickly a crash. You should check in your environment with -fsanitize=thread, this is fastest way to check for thread safety. If there really were such a big problem, I would expect it would happen a lot more often. However, this is the first time I hear of a problem that might stem from the implmentation of the ring buffer. What exactly is it that you're doing that causes this? Does it also happen if you don't do that? Klaus ___ vdr mailing list vdr@linuxtv.org https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
Re: [vdr] [PATCH] Add thread safety to cRingBufferLinear
On 03/02/2023 10:36, Klaus Schmidinger wrote: On 02.02.23 23:47, patrick9...@free.fr wrote: On 02/02/2023 23:27, Klaus Schmidinger wrote: On 02.02.23 23:16, Patrick Lerda wrote: Beside preventing crashes with vdr-2.6.3 this is required to get vdr to work properly with the gcc thread sanitizer. cRingBufferLinear was designed to be thread safe without locking. What "crashes with vdr-2.6.3" are you referring to? Klaus With a -fsanitize=thread compiled version of vdr, I had some crashes that happened quickly, for instance: ... Before making such deep changes to code that has been running flawlessly for years (or even decades) I need to be convinced that this is absolutely necessary. Is there a problem that occurs if you run VDR *without* -fsanitize=thread? Klaus I had in the past a crash from time to time, with vdr-2.6.3 this seems to be worse. Anyway, I was checking with vdr-2.4.7 and the problem is the same. This class is shared by at least 2 threads with more than one shared object; this means that without a mutex, the behavior is undefined from a C++ perspective. With -fsanitize=thread the compiler could add some jitter and that seems to trigger quickly a crash. You should check in your environment with -fsanitize=thread, this is fastest way to check for thread safety. Patrick ___ vdr mailing list vdr@linuxtv.org https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
