Linux virtualization list

* Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE()
From: Dmitry Vyukov via Virtualization @ 2016-11-25 17:28 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Mark Rutland, Davidlohr Bueso, KVM list, Michael S. Tsirkin,
	netdev, Boqun Feng, dbueso, LKML, virtualization, Paul McKenney,
	Linus Torvalds
In-Reply-To: <20161125161709.GQ3092@twins.programming.kicks-ass.net>

On Fri, Nov 25, 2016 at 5:17 PM, Peter Zijlstra <peterz@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]);
                                 ^
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply