From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753782AbcEPORu (ORCPT ); Mon, 16 May 2016 10:17:50 -0400 Received: from mail-pf0-f182.google.com ([209.85.192.182]:33039 "EHLO mail-pf0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751738AbcEPORt (ORCPT ); Mon, 16 May 2016 10:17:49 -0400 Subject: Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field To: paulmck@linux.vnet.ibm.com, Peter Zijlstra References: <1462580424-40333-1-git-send-email-Waiman.Long@hpe.com> <5733AC64.6020306@hurleysoftware.com> <20160513150749.GT3192@twins.programming.kicks-ass.net> <573615AD.60300@hurleysoftware.com> <20160516110948.GM3193@twins.programming.kicks-ass.net> <20160516121719.GC3528@linux.vnet.ibm.com> Cc: Waiman Long , Ingo Molnar , linux-kernel@vger.kernel.org, Davidlohr Bueso , Jason Low , Dave Chinner , Scott J Norton , Douglas Hatch , kcc@google.com, dvyukov@google.com From: Peter Hurley Message-ID: <5739D686.302@hurleysoftware.com> Date: Mon, 16 May 2016 07:17:42 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160516121719.GC3528@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/16/2016 05:17 AM, Paul E. McKenney wrote: > On Mon, May 16, 2016 at 01:09:48PM +0200, Peter Zijlstra wrote: >> On Fri, May 13, 2016 at 10:58:05AM -0700, Peter Hurley wrote: >>>> Note that barrier() and READ_ONCE() have overlapping but not identical >>>> results and the combined use actually makes sense here. >>>> >>>> Yes, a barrier() anywhere in the loop will force a reload of the >>>> variable, _however_ it doesn't force that reload to not suffer from >>>> load tearing. >>>> >>>> Using volatile also forces a reload, but also ensures the load cannot >>>> be torn IFF it is of machine word side and naturally aligned. >>>> >>>> So while the READ_ONCE() here is pointless for forcing the reload; >>>> that's already ensured, we still need to make sure the load isn't torn. >>> >>> If load tearing a naturally aligned pointer is a real code generation >>> possibility then the rcu list code is broken too (which loads ->next >>> directly; cf. list_for_each_entry_rcu() & list_for_each_entry_lockless()). >>> >>> For 4.4, Paul added READ_ONCE() checks for list_empty() et al, but iirc >>> that had to do with control dependencies and not load tearing. >> >> Well, Paul is the one who started the whole load/store tearing thing, so >> I suppose he knows what he's doing. > > That had to do with suppressing false positives for one of Dmitry > Vjukov's concurrency checkers. I suspect that Peter Hurley is right > that continued use of that checker would identify other places needing > READ_ONCE(), but from what I understand that is on hold pending a formal > definition of the Linux-kernel memory model. (KCC and Dmitry (CCed) > can correct my if I am confused on this point.) > >> That said; its a fairly recent as things go so lots of code hasn't been >> updated yet, and its also a very unlikely thing for a compiler to do; >> since it mostly doesn't make sense to emit multiple instructions where >> one will do, so its not a very high priority thing either. >> >> But from what I understand, the compiler is free to emit all kinds of >> nonsense for !volatile loads/stores. > > That is quite true. :-/ > >>> OTOH, this patch might actually produce store-tearing: >>> >>> +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem) >>> +{ >>> + /* >>> + * We check the owner value first to make sure that we will only >>> + * do a write to the rwsem cacheline when it is really necessary >>> + * to minimize cacheline contention. >>> + */ >>> + if (sem->owner != RWSEM_READER_OWNED) >>> + sem->owner = RWSEM_READER_OWNED; >>> +} >> >> Correct; which is why we should always use {READ,WRITE}_ONCE() for >> anything that is used locklessly. > > Completely agreed. Improve readability of code by flagging lockless > shared-memory accesses, help checkers better find bugs, and prevent the > occasional compiler mischief! I think this would be a mistake for 3 reasons: 1. If READ_ONCE()/WRITE_ONCE() is necessary to prevent load/store tearing of any normally-atomic type (char/int/long/void*), then _every_ access would require READ_ONCE()/WRITE_ONCE(), thus eliminating any possibility of compiler optimization (eg. eliding redundant loads) where it would otherwise be possible. 2. Makes a mess of otherwise readable code. 3. Error-prone; ie., easy to overlook in review. There is no practical difference between _always_ using READ_ONCE()/WRITE_ONCE() (to prevent tearing) and declaring the field volatile. So we've come full-circle from volatile-considered-harmful. Regards, Peter Hurley