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

Corey Minyard
2008-09-26T13:49:56Z

Paul E. McKenney wrote:
> On Thu, Sep 25, 2008 at 10:18:33PM -0500, Corey Minyard wrote:
>
>> From: Corey Minyard <cminyard@mvista.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@mvista.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