Virtualization - Nov 2016 - [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE()

On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov
wrote:
> 
> READ/WRITE_ONCE imply atomicity. Even if their names don't spell it (a
> function name doesn't have to spell all of its guarantees). Most of
> the uses of READ/WRITE_ONCE will be broken if they are not atomic.
In practice, this is certainly the assumption made by many/most users of
the *_ONCE() accessors.

Looking again, Linus does seem to agree that word-sized accesses should
result in single instructions (and be single-copy atomic) [1], so in
contrast to [2], that's clearly *part* of the point of the *_ONCE()
accessors...
> "Read once but not necessary atomically" is a very subtle
primitive
> which is very easy to misuse.
I agree. Unfortunately, Linus does not appear to [2].
> What are use cases for such primitive that won't be OK with "read
once
> _and_ atomically"?
I have none to hand.

Thanks,
Mark.

[1] http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02674.html
[2] http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02670.html

On Fri, Nov 25, 2016 at 05:17:09PM +0100, Peter Zijlstra
wrote:
> On Fri, Nov 25, 2016 at 04:10:04PM +0000, Mark Rutland wrote:
> > On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov wrote:
> 
> > > What are use cases for such primitive that won't be OK with
"read once
> > > _and_ atomically"?
> > 
> > I have none to hand.
> 
> Whatever triggers the __builtin_memcpy() paths, and even the size==8
> paths on 32bit.
Lockref, per:

http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02294.html

In that specific case, a torn value just means we'll retry until we get
a non torn value, due to the cmpxchg. For that case, all we need is the
value to be reloaded per invocation of READ_ONCE().

This guy seems to have the full story:

http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02389.html
http://lkml.iu.edu/hypermail/linux/kernel/1503.3/02558.html

Thanks,
Mark.

On 11/25/2016 05:17 PM, Peter Zijlstra wrote:
> On Fri, Nov 25, 2016 at 04:10:04PM +0000, Mark Rutland wrote:
>> On Fri, Nov 25, 2016 at 04:21:39PM +0100, Dmitry Vyukov wrote:
> 
>>> What are use cases for such primitive that won't be OK with
"read once
>>> _and_ atomically"?
>>
>> I have none to hand.
> 
> Whatever triggers the __builtin_memcpy() paths, and even the size==8
> paths on 32bit.
> 
> You could put a WARN in there to easily find them.
There were several cases that I found during writing the *ONCE stuff.
For example there are some 32bit ppc variants with 64bit PTEs. Some for
others (I think sparc). And the mm/ code is perfectly fine with these
PTE accesses being done NOT atomic.

> 
> The advantage of introducing the SINGLE_{LOAD,STORE}() helpers is that
> they compiletime validate this the size is 'right' and can runtime
check
> alignment constraints.
> 
> IE, they are strictly stronger than {READ,WRITE}_ONCE().
>

On Fri, Nov 25, 2016 at 5:17 PM, Peter Zijlstra <peterz at infradead.org>
wrote:
>> > What are use cases for such primitive that won't be OK with
"read once
>> > _and_ atomically"?
>>
>> I have none to hand.
>
> Whatever triggers the __builtin_memcpy() paths, and even the size==8
> paths on 32bit.
>
> You could put a WARN in there to easily find them.
>
> The advantage of introducing the SINGLE_{LOAD,STORE}() helpers is that
> they compiletime validate this the size is 'right' and can runtime
check
> alignment constraints.
>
> IE, they are strictly stronger than {READ,WRITE}_ONCE().

Uh, so, READ/WRITE_ONCE are non-atomic now. I missed that.

If READ/WRITE_ONCE are non-atomic, half of kernel is broken. All these
loads of flags, ringbuffer positions, pointers, etc are broken.

What about restoring READ/WRITE_ONCE as atomic, and introducing
separate primitives for _non_ atomic loads/stores?
It seems to me that there is just a dozen of cases that don't need
atomicity and where performance is any important (though, some of
these should probably try to write to shared memory less frequently
and save hundreds of cycles, rather than try to save few cycles on
local instructions).

I've compiled kernel with restored size checks in
READ/WRITE/ACCESS_ONCE and the following places seem to expect that
access is actually atomic (while it is not).
But if we don't guarantee that word-sized READ/WRITE_ONCE are atomic,
then I am sure we can find a hundred more of broken places.


arch/x86/entry/vdso/vdso32/../vclock_gettime.c:297:18: note: in
expansion of macro ?ACCESS_ONCE?
  time_t result = ACCESS_ONCE(gtod->wall_time_sec);

kernel/events/ring_buffer.c:160:10: error: call to
?__compiletime_assert_160? declared with attribute error: Need native
word sized stores/loads for atomicity.
   tail = READ_ONCE(rb->user_page->data_tail);

kernel/events/core.c:5145:16: error: call to
?__compiletime_assert_5145? declared with attribute error: Need native
word sized stores/loads for atomicity.
   aux_offset = ACCESS_ONCE(rb->user_page->aux_offset);
                ^
kernel/events/core.c:5146:14: error: call to
?__compiletime_assert_5146? declared with attribute error: Need native
word sized stores/loads for atomicity.
   aux_size = ACCESS_ONCE(rb->user_page->aux_size);

drivers/cpufreq/cpufreq_governor.c:283:8: error: call to
?__compiletime_assert_283? declared with attribute error: Need native
word sized stores/loads for atomicity.
  lst = READ_ONCE(policy_dbs->last_sample_time);
        ^
drivers/cpufreq/cpufreq_governor.c:301:7: error: call to
?__compiletime_assert_301? declared with attribute error: Need native
word sized stores/loads for atomicity.
   if (unlikely(lst != READ_ONCE(policy_dbs->last_sample_time))) {

net/core/gen_estimator.c:136:3: error: call to
?__compiletime_assert_136? declared with attribute error: Need native
word sized stores/loads for atomicity.
   WRITE_ONCE(e->rate_est->bps, (e->avbps + 0xF) >> 5);
   ^
net/core/gen_estimator.c:142:3: error: call to
?__compiletime_assert_142? declared with attribute error: Need native
word sized stores/loads for atomicity.
   WRITE_ONCE(e->rate_est->pps, (e->avpps + 0xF) >> 5);

fs/proc_namespace.c:28:10: error: call to ?__compiletime_assert_28?
declared with attribute error: Need native word sized stores/loads for
atomicity.
  event = ACCESS_ONCE(ns->event);

drivers/md/dm-stats.c:700:32: error: call to
?__compiletime_assert_700? declared with attribute error: Need native
word sized stores/loads for atomicity.
   shared->tmp.sectors[READ] += ACCESS_ONCE(p->sectors[READ]);
                                ^
drivers/md/dm-stats.c:701:33: error: call to
?__compiletime_assert_701? declared with attribute error: Need native
word sized stores/loads for atomicity.
   shared->tmp.sectors[WRITE] += ACCESS_ONCE(p->sectors[WRITE]);
                                 ^