Date:	Fri, 26 Sep 2008 08:49:40 -0500
From:	Corey Minyard <minyard@....org>
To:	paulmck@...ux.vnet.ibm.com
Cc:	Linux Kernel <linux-kernel@...r.kernel.org>,
	netdev@...r.kernel.org, jarkao2@...il.com, shemminger@...tta.com
Subject: Re: [PATCH 1/1] Use RCU for the UDP hash lock

Paul E. McKenney wrote:
> On Thu, Sep 25, 2008 at 10:18:33PM -0500, Corey Minyard wrote:
>   
>> From: Corey Minyard <cminyard@...sta.com>
>>
>> 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@...sta.com>
>> ---
>>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists