Messages in this thread | | | Date | Thu, 25 Sep 2008 14:21:55 -0500 | From | Corey Minyard <> | Subject | Re: [PATCH 1/1] Use RCU for the UDP hash lock |
| |
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
| |