From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751960AbbIMKGi (ORCPT ); Sun, 13 Sep 2015 06:06:38 -0400 Received: from mail-wi0-f175.google.com ([209.85.212.175]:34911 "EHLO mail-wi0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751349AbbIMKGh (ORCPT ); Sun, 13 Sep 2015 06:06:37 -0400 Subject: Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage To: paulmck@linux.vnet.ibm.com References: <20150512224603.GA4531@linux.vnet.ibm.com> <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> 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 From: Patrick Marlier Message-ID: <55F54AA9.3020102@gmail.com> Date: Sun, 13 Sep 2015 12:06:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20150911230554.GA26655@linux.vnet.ibm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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). 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()? Thanks. -- Pat