From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754309AbbIMQLT (ORCPT ); Sun, 13 Sep 2015 12:11:19 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:39319 "EHLO e36.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752992AbbIMQLR (ORCPT ); Sun, 13 Sep 2015 12:11:17 -0400 X-Helo: d03dlp02.boulder.ibm.com X-MailFrom: paulmck@linux.vnet.ibm.com X-RcptTo: linux-kernel@vger.kernel.org Date: Sun, 13 Sep 2015 09:10:24 -0700 From: "Paul E. McKenney" To: Patrick Marlier Cc: Steven Rostedt , NeilBrown , linux-kernel@vger.kernel.org, mingo@kernel.org, laijs@cn.fujitsu.com, dipankar@in.ibm.com, akpm@linux-foundation.org, mathieu.desnoyers@efficios.com, josh@joshtriplett.org, tglx@linutronix.de, peterz@infradead.org, dhowells@redhat.com, edumazet@google.com, dvhart@linux.intel.com, fweisbec@gmail.com, oleg@redhat.com, bobby.prani@gmail.com, wangyun@linux.vnet.ibm.com Subject: Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage Message-ID: <20150913161023.GM4029@linux.vnet.ibm.com> Reply-To: paulmck@linux.vnet.ibm.com References: <1431470787-4702-1-git-send-email-paulmck@linux.vnet.ibm.com> <1431470787-4702-3-git-send-email-paulmck@linux.vnet.ibm.com> <20150512223853.55e0ed90@grimm.local.home> <20150513125839.371ef677@notabene.brown> <5557819E.1060001@gmail.com> <20150518120647.0c3cecd8@notabene.brown> <20150518094321.2012a66a@gandalf.local.home> <20150519220725.GA6776@linux.vnet.ibm.com> <20150911230554.GA26655@linux.vnet.ibm.com> <55F54AA9.3020102@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55F54AA9.3020102@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15091316-0021-0000-0000-000012C21B5D Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Sep 13, 2015 at 12:06:33PM +0200, Patrick Marlier wrote: > > On 09/12/2015 01:05 AM, Paul E. McKenney wrote: > >On Tue, May 19, 2015 at 03:07:25PM -0700, Paul E. McKenney wrote: > >>On Mon, May 18, 2015 at 09:43:21AM -0400, Steven Rostedt wrote: > >>>On Mon, 18 May 2015 12:06:47 +1000 > >>>NeilBrown wrote: > >>> > >>> > >>>>>struct mddev { > >>>>>... > >>>>> struct list_head disks; > >>>>>...} > >>>>> > >>>>>struct list_head { > >>>>> struct list_head *next, *prev; > >>>>>}; > >>>>> > >>>>>The tricky thing is that "list_entry_rcu" before and after the patch is > >>>>>reading the same thing. > >>>> > >>>>No it isn't. > >>>>Before the patch it is passed the address of the 'next' field. After the > >>>>patch it is passed the contents of the 'next' field. > >>> > >>>Right. > >>> > >>>> > >>>> > >>>>> > >>>>>However in your case, the change I proposed is probably wrong I trust > >>>>>you on this side. :) What's your proposal to fix it with the rculist patch? > >>>> > >>>>What needs fixing? I don't see anything broken. > >>>> > >>>>Maybe there is something in this "rculist patch" that I'm missing. Can you > >>>>point me at it? > >>>> > >>> > >>>Probably some debugging tool like sparse notices that the assignment > >>>isn't a true list entry and complains about it. In other words, I think > >>>the real fix is to fix the debugging tool to ignore this, because the > >>>code is correct, and this is a false positive failure, and is causing > >>>more harm than good, because people are sending out broken patches due > >>>to it. > >> > >>OK, finally did the history trawling that I should have done to begin with. > >> > >>Back in 2010, Arnd added the __rcu pointer checking in sparse. > >>But the RCU list primitives were used on non-RCU-protected lists, so > >>some casting pain was required to avoid sparse complaints. (Keep in > >>mind that the list_head structure does not mark ->next with __rcu.) > >>Arnd's workaround was to copy the pointer to the local stack, casting > >>it to an __rcu pointer, then use rcu_dereference_raw() to do the needed > >>traversal of an RCU-protected pointer. > >> > >>This of course resulted in an extraneous load from the stack, which > >>Patrick noticed in his performance work, and which motivated him to send > >>the patches. > >> > >>Perhaps what I should do is create an rcu_dereference_nocheck() for use > >>in list traversals, that omits the sparse checking. That should get rid > >>of both the sparse warnings and the strange casts. > >> > >>The code in md probably needs to change in any case, as otherwise we are > >>invoking rcu_dereference_whatever() on a full struct list_head rather > >>than on a single pointer. Or am I missing something here? > > > >Finally getting back to this one... > > > >I switched to lockless_dereference() instead of rcu_dereference_raw(), > >and am running it through the testing gamut. Patrick, are you OK with > >this change? > > Paul, > > This sounds good to me. It should fix the performance issue (will > check with my benchmark). Thank you, looking forward to seeing the results! > I think for drivers/md/bitmap.c:next_active_rdev() the problem was > fixed but do you know if it also fixed for > net/netfilter/core.c:nf_hook_slow()? It does appear so. The statement now reads: elem = list_entry_rcu(state->hook_list, struct nf_hook_ops, list); And ->hook_list is defined as follows: struct list_head *hook_list; Thanx, Paul