Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932156AbaFBVMd (ORCPT ); Mon, 2 Jun 2014 17:12:33 -0400 Received: from mail-ve0-f171.google.com ([209.85.128.171]:48172 "EHLO mail-ve0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752848AbaFBVMb (ORCPT ); Mon, 2 Jun 2014 17:12:31 -0400 MIME-Version: 1.0 In-Reply-To: <20140602210227.GE22231@linux.vnet.ibm.com> References: <20140602162525.GH16155@laptop.programming.kicks-ass.net> <20140602163032.GI16155@laptop.programming.kicks-ass.net> <538CB56E.5010709@hp.com> <20140602200525.GD13930@laptop.programming.kicks-ass.net> <20140602210227.GE22231@linux.vnet.ibm.com> Date: Mon, 2 Jun 2014 14:12:30 -0700 X-Google-Sender-Auth: S6Ld09e0rC8nI5NqWjHWneWLnxw Message-ID: Subject: Re: [PATCH v2] introduce atomic_pointer to fix a race condition in cancelable mcs spinlocks From: Linus Torvalds To: Paul McKenney Cc: Peter Zijlstra , Waiman Long , Mikulas Patocka , "James E.J. Bottomley" , Helge Deller , John David Anglin , Parisc List , Linux Kernel Mailing List , "Vinod, Chegu" , Thomas Gleixner , Rik van Riel , Andrew Morton , Davidlohr Bueso , Peter Anvin , Andi Kleen , "Chandramouleeswaran, Aswin" , "Norton, Scott J" , Jason Low Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 2, 2014 at 2:02 PM, Paul E. McKenney wrote: > > In the ->qlen case, interrupts are disabled and the current CPU is > the only one who can write, so the read need not be volatile. In the > ->n_barrier_done, modifications are done holding ->barrier_mutex, so again > the read need not be volatile. In the sync_rcu_preempt_exp_count case, > modifications are done holding sync_rcu_preempt_exp_mutex, so once again, > the read need not be volatile. So I could do something like: > > ACCESS_ONCE(rdp->qlen) = rdp->qlen + 1; > > But that still makes gcc generate bad code > > The reason I was not all that worried about this is that these are not > in fastpaths, and the last two are especially not in fastpaths. > > Suggestions? So I think it probably *works*, but even so splitting it up to use ACCESS_ONCE() on just the write is probably a better option, if only because it would then make it much easier to change if we do end up splitting reads and writes. Because from a gcc code generation standpoint, using "volatile" will always be horrible, because gcc will never be able to turn it into a read-modify-write cycle. Arguable gcc _should_ be able to do that (it is certainly allowable within the virtual machine definition), but I understand why it doesn't ("volatile? Let's not optimize anything at all, because it's special"). So "ACCESS_ONCE() + R-M-W" operation is actually pretty much guaranteed to be "ACCESS_TWICE()", which may well be ok (performance may not matter, and even when it does most architectures don't actually have r-m-w instructions and when they do they aren't always even faster), but I think it's just horribly horribly bad from a conceptual and readability standpoint because it's so misleading. So I'd actually rather see two explicit ACCESS_ONCE() calls - once to read, once to write. Because that at least describes what is happening, unlike the current situation. Put another way: I can understand why you do it, and I can even agree that it is "correct" from a functionality standpoint. But even despite that all, I really don't like the construct very much.. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/