Messages in this thread | | | Subject | Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage | From | Patrick Marlier <> | Date | Sun, 13 Sep 2015 12:06:33 +0200 |
| |
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 <neilb@suse.de> 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
| |