Re: linux-next: build warnings after merge of the access_once tree

[Christian Borntraeger <borntraeger@de.ibm.com>]

Am 26.03.2015 um 17:15 schrieb Linus Torvalds:
> On Thu, Mar 26, 2015 at 7:22 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> So we can either just remove the READ_ONCE(), or replace it with a
>> leading barrier() call just to be on the paranoid side of things.
> 
> NOOO!
> 
>> Any preferences?
> 
> Not a preference: a _requirement_.
> 
> Get rid of the f*cking size checks etc on READ_ONCE() and friends.

Oh I just added that check back then because some guy named
Linus suggested something like that ;-)

--- snip ---
(Btw, it's not just aggregate types, even non-aggregate types like
"long long" are not necessarily safe, to give the same 64-bit on
x86-32 example. So adding an assert that it's smaller or equal in size
to a "long" might also not be unreasonable)
--- snip ---

https://www.marc.info/?l=linux-kernel&m=141565366209769&w=1

I am fine with Peters patch :-)

Christian

> 
> They are about - wait for it - "reading a value once".
> 
> Note how it doesn't say ANYTHING about "atomic" or anything like that.
> It's about reading *ONCE*.


> 
>> Something like so, but with a sensible comment I suppose.
> 
> Hell f*cking no. The "something like so" is huge and utter crap,
> because the barrier is on the wrong side.
> 
>> -       old.lock_count = READ_ONCE(lockref->lock_count);                        \
>> +       barrier();                                                              \
>> +       old.lock_count = lockref->lock_count;                                   \
>>         while (likely(arch_spin_value_unlocked(old.lock.rlock.raw_lock))) {     \
>>                 struct lockref new = old, prev = old;                           \
> 
> The WHOLE point of the READ_ONCE (formerly ACCESS_ONCE) is that it
> tells the compiler that it cannot reload the value.
> 
> Notice how it is *not* about atomicitiy. The compiler can read the
> value in fifteen pieces, randomly mixing one bit or five. Nobody
> cares.
> 
> But the important thing is that ONCE IT IS READ, it is never read
> again. That's the "once" part.
> 
> Why is that important? It's important because we have to absolutely
> guartantee that the value we *test* is the same value we use later.
> That's a common concern with mutable variables, and is the only reason
> for READ_ONCE() in the first place.
> 
> The whole atomicity etc crap was just that - crap. It was never about
> atomicitiy. It was about the compiler not reloading values.
> 
> So no. No barriers. No "removal of READ_ONCE". Just get rid of the
> broken "sanity" checks in the READ_ONCE implementation that are just
> pure garbage.
> 
> The checks in ACCESS_ONCE() were because apparently gcc got things
> wrong - dropping the volatile - for aggregate types. They were never
> supposed to be about atomicity, even if there clearly was some
> confusion about that.
> 
> Really. Just get rid of the checks - they were wrong. They were
> clearly very close to *introducing* a bug, rather than fixing anything
> at all.
> 
>                              Linus
>