Messages in this thread | | | Date | Sun, 13 Sep 2015 09:10:24 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH tip/core/rcu 3/4] md/bitmap: Fix list_entry_rcu usage |
| |
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 <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).
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
| |