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

Corey Minyard
2008-09-25T19:47:44Z

Stephen Hemminger wrote:
> On Thu, 25 Sep 2008 08:29:36 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
>
>> On Wed, Sep 24, 2008 at 03:46:20PM -0500, Corey Minyard wrote:
>>
>>> Stephen Hemminger wrote:
>>>
>>>>
>>>>
>>>>> static inline void udp_lib_unhash(struct sock *sk)
>>>>> {
>>>>> - write_lock_bh(&udp_hash_lock);
>>>>> - if (sk_del_node_init(sk)) {
>>>>> + spin_lock_bh(&udp_hash_wlock);
>>>>> + if (sk_del_node_rcu(sk)) {
>>>>> inet_sk(sk)->num = 0;
>>>>> sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
>>>>> }
>>>>> - write_unlock_bh(&udp_hash_lock);
>>>>> + spin_unlock_bh(&udp_hash_wlock);
>>>>> + synchronize_sched();
>>>>>
>>>>>
>>>> Could this be synchronize_rcu? You are using rcu_read_lock() protected
>>>> sections.
>>>>
>>>>
>>> I meant to comment on that. I wasn't sure which to use, so I chose the
>>> more conservative approach. synchronize_rcu() might be appropriate.
>>>
>> You do indeed need to match the update-side and read-side primitives:
>>
>> Update-side Read-side
>>
>> synchronize_rcu() rcu_read_lock()
>> call_rcu() rcu_read_unlock()
>>
>> call_rcu_bh() rcu_read_lock_bh()
>> rcu_read_unlock_bh()
>>
>> synchronize_sched() preempt_disable()
>> preempt_enable()
>> [and anything else
>> that disables either
>> preemption or irqs]
>>
>> synchronize_srcu() srcu_read_lock()
>> srcu_read_unlock()
>>
>>
>> Mixing RCU or RCU-SCHED with RCU-BH will fail in Classic RCU systems,
>> while mixing RCU or RCU-BH with RCU-SCHED will fail in preemptable RCU
>> systems. Mixing SRCU with any of the other flavors of RCU will fail
>> on any system.
>>
>> So please match them up correctly!
>>
Ok, will do. I read more on this, and I think I understand the issues
better.

>>
>
> Also, for consistency with other parts of networking code, don't introduce
> the synchronize_sched() or synchronize_srcu() pattern to network protocols
> unless there is a no other way to achieve the desired result.
>
Do you mean synchronize_rcu(), too? It seems to be used in the net
code. To avoid that I'd need to add a struct rcu_head to struct sock.
Would that be preferable?

Thanks,

-corey