Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753410AbbBJSDp (ORCPT ); Tue, 10 Feb 2015 13:03:45 -0500 Received: from e36.co.us.ibm.com ([32.97.110.154]:47346 "EHLO e36.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752198AbbBJSDl (ORCPT ); Tue, 10 Feb 2015 13:03:41 -0500 Date: Tue, 10 Feb 2015 10:03:34 -0800 From: "Paul E. McKenney" To: Peter Hurley Cc: Pranith Kumar , Mathieu Desnoyers , Huang Ying , linux-kernel@vger.kernel.org, David Howells , stable@vger.kernel.org Subject: Re: [PATCH] llist: Fix missing lockless_dereference() Message-ID: <20150210180334.GS4166@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <1423274934-10625-1-git-send-email-mathieu.desnoyers@efficios.com> <54DA0FC6.3090204@hurleysoftware.com> <20150210163810.GP4166@linux.vnet.ibm.com> <54DA3FF4.3010700@hurleysoftware.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54DA3FF4.3010700@hurleysoftware.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15021018-0021-0000-0000-000008722B00 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3311 Lines: 91 On Tue, Feb 10, 2015 at 12:29:24PM -0500, Peter Hurley wrote: > On 02/10/2015 11:38 AM, Paul E. McKenney wrote: > > On Tue, Feb 10, 2015 at 09:03:50AM -0500, Peter Hurley wrote: > >> On 02/06/2015 09:08 PM, Mathieu Desnoyers wrote: > >>> A lockless_dereference() appears to be missing in llist_del_first(). > >>> It should only matter for Alpha in practice. > >>> > >>> Signed-off-by: Mathieu Desnoyers > >>> CC: Huang Ying > >>> CC: Paul McKenney > >>> CC: David Howells > >>> CC: Pranith Kumar > >>> CC: stable@vger.kernel.org # v3.1+ > >>> --- > >>> lib/llist.c | 8 +++++++- > >>> 1 file changed, 7 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/lib/llist.c b/lib/llist.c > >>> index f76196d..f34e176 100644 > >>> --- a/lib/llist.c > >>> +++ b/lib/llist.c > >>> @@ -26,6 +26,7 @@ > >>> #include > >>> #include > >>> #include > >>> +#include > >> > >> Pranith, > >> > >> I didn't realize you put lockless_dereference() in rcupdate.h > >> > >> If the point of lockless_reference() is to provide a utility function for > >> situations _not_ involving RCU, then it doesn't make sense to provide it > >> in an RCU header file. > > > > OK, I'll bite. Just where do you suggest putting it? ;-) > > Two possibilities: > 1. linux/compiler.h where READ/WRITE/ACCESS_ONCE() are, or > 2. a new arch-independent header sucked in by asm/barrier.h (because it's > basically a barrier abstraction, in the same way that smp_load_acquire/ > smp_store_release are) > > > > That question aside, lockless_dereference() does resemble the > > rcu_dereference() family of APIs. This of course means that having it in > > rcupdate.h near rcu_dereference() makes it easier to maintain, given that > > needed changes to one are likely to require at least review of the rest. > > I can understand how and why it got there. > But it's not an RCU abstraction, so having random users pulling in RCU headers > to get at a convenient (but not strictly necessary) helper function is less than > ideal. > > Honestly, I'd rather see the naked smp_read_barrier_depends() than wondering why > someone grabbed linux/rcupdate.h for the lockless list implementation. The usual fix for this problem is to list the API member as a comment at the end of the #include line. Thanx, Paul > Regards, > Peter Hurley > > > >>> /** > >>> @@ -67,7 +68,12 @@ struct llist_node *llist_del_first(struct llist_head *head) > >>> { > >>> struct llist_node *entry, *old_entry, *next; > >>> > >>> - entry = head->first; > >>> + /* > >>> + * Load entry before entry->next. Matches the implicit > >>> + * memory barrier before the cmpxchg in llist_add_batch(), > >>> + * which ensures entry->next is stored before entry. > >>> + */ > >>> + entry = lockless_dereference(head->first); > >>> for (;;) { > >>> if (entry == NULL) > >>> return NULL; > >>> > >> > > > -- 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/