On Thu, Mar 26, 2015 at 7:22 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-next" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
