On 10/21/19 3:22 PM, Douglas Gilbert wrote: > On 2019-10-18 6:05 a.m., Hannes Reinecke wrote: [ .. ] >> One thing to keep in mind here is that 'set_bit()' is not atomic; it >> needs to be followed by a memory barrier or being replaced by >> test_and_set_bit() if possible. >> Please audit the code if that poses a problem. > > Hi, > set_bit() and friends are documented in Documentation/atomic_bitops.txt > so it would be surprising if they were not _atomic_ . There are variants > documented in that file that _maybe_ non-atomic, they are the one that > start with __ such as __set_bit(). > > So what I believe Documentation/atomic_bitops.txt is trying to say (in > a sloppy kind of way) is that set_bit() and clear_bit() have _relaxed_ > memory order. C/C++ standards (from 2011 onwards) have 6 memory orders: > - relaxed [no memory order guarantees] > - consume [C++17 says "please don't use"!] > - acquire > - release > - acquire-release > - sequentially consistent ["cst"] > > That list is from weakest to strongest. C/C++ default everything they > can to "cst" as it requires the least thinking and is therefore the > safest, but has the highest runtime overhead (depending somewhat on > the platform). > > Linux adds a few wrinkles to the above as C/C++ are only considering > threads while Linux additionally has ISRs, DMA, memory-mapped IO and > the like to consider. > > From what I have read in the Documentation directories, most > descriptions are written by gifted and un-gifted amateurs, apart from > one amazing exception: Documentation/memory-barriers.txt . Now they > are professions and the authors put their names, in full, at the > top which IMO is always a good sign. > > > Back to the review at hand, if set_bit() has relaxed memory order > and needs to be seen by another thread (or ISR/bottom half) then it > is relying on a following atomic operation that has stronger memory > ordering guarantees (than relaxed). In my code I tend to use > __set_bit() and __clear_bit() when a file descriptor or request object > is being created, as no other thread should know of its existence at > that time. > > Doug Gilbert > > Reference: C++ Concurrency in action, Second edition, Anthony Williams > [As the C11 and C++11 concurrency subcommittees were "manned" by the > same folks, they tried to make their memory models as compatible as > possible.] > > That's all I wanted to know. Thanks for the confirmation. Reviewed-by: Hannes Reinecke <hare@xxxxxxxx> Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@xxxxxxx +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
