Re: [PATCH 1/1] Use RCU for the UDP hash lock

[Corey Minyard <minyard@xxxxxxx>]

Paul E. McKenney wrote:
> On Thu, Sep 25, 2008 at 10:18:33PM -0500, Corey Minyard wrote:
>   
> > From: Corey Minyard <cminyard@xxxxxxxxxx>
> >
> > Convert access to the udp_hash table to use RCU.
> >     
>
> Looks much better!
>
> Some rcu_dereference() fixes, a comment fix, and a question below.
>
> 							Thanx, Paul
>
>   
> > Signed-off-by: Corey Minyard <cminyard@xxxxxxxxxx>
> > ---
> >  include/linux/rculist.h |   19 +++++++++++++++++
> >  include/net/sock.h      |   51 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/net/udp.h       |    9 ++++---
> >  net/ipv4/udp.c          |   47 ++++++++++++++++++++++++------------------
> >  net/ipv6/udp.c          |   17 ++++++++-------
> >  5 files changed, 111 insertions(+), 32 deletions(-)
> >
> > This patch is the second try; I believe I fixed all issues that people
> > raised.  Thanks to everyone who commented on this.
> >
> > I beat on this for a few hours with my test program, too.
> >
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index eb4443c..4d3cc58 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -397,5 +397,24 @@ static inline void hlist_add_after_rcu(struct hlist_node *prev,
> >  		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; }); \
> >  		pos = rcu_dereference(pos->next))
> >
> > +
> > +/**
> > + * hlist_for_each_entry_from_rcu - iterate over rcu list starting from pos
> > + * @tpos:      the type * to use as a loop cursor.
> > + * @pos:       the &struct hlist_node to use as a loop cursor.
> > + * @head:      the head for your list.
> > + * @member:    the name of the hlist_node within the struct.
> > + *
> > + * This list-traversal primitive may safely run concurrently with
> > + * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> > + * as long as the traversal is guarded by rcu_read_lock().
> > + */
> > +#define hlist_for_each_entry_from_rcu(tpos, pos, member)                \
> > +	for (;                                                          \
> > +	     rcu_dereference(pos) && ({ prefetch(pos->next); 1; }) &&    \
> > +	       ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; }); \
> > +	     pos = pos->next)
> >     
>
> Always apply rcu_dereference() to whatever it was that you
> rcu_assign_pointer()ed to.  You don't need the first rcu_dereference()
> because you (hopefully) used rcu_dereference() either directly or 
> indirectly when picking up the pointer in the first place.  You -do-
> need one on the ->next, however.
>
> So something like this:
>
> +#define hlist_for_each_entry_from_rcu(tpos, pos, member)                \
> +	for (;                                                          \
> +	     (pos) && ({ prefetch(pos->next); 1; }) &&    \
> +	       ({ tpos = hlist_entry(pos, typeof(*tpos), member); 1; }); \
> +	     pos = rcu_dereference((pos)->next))
>   
Yes, of course, I've changed it.

> Interesting, though -- you repeat whatever one you stopped on
> previously, unlike the _continue_ variants.
>   
Yes, it is interesting, but it preserves the semantics of 
hlist_for_each_entry_from().  It's the semantics you want in this case, 
and I think the "from" name implies the semantics.

>   
> > +	/*
> > +	 * Note that this is safe, even with an RCU lock.
> > +	 * udp_lib_unhash() is the removal function, it calls
> > +	 * synchronize_sched() and the socket counter cannot go to
> >     
>
> synchronize_rcu(), right?
>   
Yes, thanks.
>   
> > +	 * zero until it returns.  So if we increment it inside the
> > +	 * RCU read lock, it should never go to zero and then be
> > +	 * incremented again.
> >     
>
> So the caller of udp_lib_unhash() does the decrement?  Looks like this
> might be sk_common_release(), but too many pointers to functions.  One
> could also argue for udp_disconnect()...
>   
I don't believe udp_disconnect() releases the socket.  
sk_common_release() seems to be the place where the refcount is 
decremented.  But wherever it is done, it would have to be after the unhash.

/me fires up the test harness again.

Thanks,

-corey
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html