Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE()

[Dmitry Vyukov]

On Fri, Nov 25, 2016 at 5:17 PM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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]);
                                 ^