Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932878AbcKYR30 (ORCPT ); Fri, 25 Nov 2016 12:29:26 -0500 Received: from mail-lf0-f52.google.com ([209.85.215.52]:33250 "EHLO mail-lf0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932707AbcKYR3Q (ORCPT ); Fri, 25 Nov 2016 12:29:16 -0500 MIME-Version: 1.0 In-Reply-To: <20161125161709.GQ3092@twins.programming.kicks-ass.net> References: <1479983114-17190-1-git-send-email-mark.rutland@arm.com> <20161124222357-mutt-send-email-mst@kernel.org> <20161125112203.GA26611@leverpostej> <32dfca07-59f3-b75a-3154-cf6b6c8538f0@de.ibm.com> <20161125122356.GB26611@leverpostej> <20161125124044.GN3092@twins.programming.kicks-ass.net> <20161125124404.GI3174@twins.programming.kicks-ass.net> <20161125145512.GA4014@Boquns-MacBook-Pro.local> <20161125161004.GA30181@leverpostej> <20161125161709.GQ3092@twins.programming.kicks-ass.net> From: Dmitry Vyukov Date: Fri, 25 Nov 2016 18:28:53 +0100 Message-ID: Subject: Re: [PATCH 0/3] virtio/vringh: kill off ACCESS_ONCE() To: Peter Zijlstra Cc: Mark Rutland , Boqun Feng , Christian Borntraeger , "Michael S. Tsirkin" , LKML , Davidlohr Bueso , dbueso@suse.de, jasowang@redhat.com, KVM list , netdev , Paul McKenney , virtualization@lists.linux-foundation.org, Linus Torvalds Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAPHTUP3015981 Content-Length: 3903 Lines: 92 On Fri, Nov 25, 2016 at 5:17 PM, Peter Zijlstra 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]); ^