Re: [PATCH v5 05/23] sg: bitops in sg_device

[Hannes Reinecke <hare@suse.de>]

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@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer