On Fri, Aug 20, 2010 at 09:03:03PM +0300, Julian Anastasov wrote: > > Hello, > > On Fri, 20 Aug 2010, Simon Horman wrote: > > > Signed-off-by: Simon Horman <horms@xxxxxxxxxxxx> > > > > --- > > > > I'm still getting my head around RCU, so review would be greatly appreciated. > > > > It occurs to me that this code is not performance critical, so > > perhaps simply replacing the rwlock with a spinlock would be better? > > This specific code does not need RCU conversion, see below Agreed. > > Index: nf-next-2.6/net/netfilter/ipvs/ip_vs_sched.c > > =================================================================== > > --- nf-next-2.6.orig/net/netfilter/ipvs/ip_vs_sched.c 2010-08-20 22:21:01.000000000 +0900 > > +++ nf-next-2.6/net/netfilter/ipvs/ip_vs_sched.c 2010-08-20 22:21:51.000000000 +0900 > > @@ -35,7 +35,7 @@ > > static LIST_HEAD(ip_vs_schedulers); > > > > /* lock for service table */ > > -static DEFINE_RWLOCK(__ip_vs_sched_lock); > > +static DEFINE_SPINLOCK(ip_vs_sched_mutex); > > Here is what I got as list of locking points: > > __ip_vs_conntbl_lock_array: > - can benefit from RCU, main benefits come from here > > - ip_vs_conn_unhash() followed by ip_vs_conn_hash() is tricky with RCU, > needs more thinking, eg. when cport is changed > > cp->lock, cp->refcnt: > - not a problem > > tcp_app_lock, udp_app_lock, sctp_app_lock: > - can benefit from RCU (once per connection) > > svc->sched_lock: > - only 1 read_lock, mostly writers that need exclusive access > - so, not suitable for RCU, can be switched to spin_lock for speed > > __ip_vs_sched_lock: > - not called by packet handlers, no need for RCU > - used only by one ip_vs_ctl user (configuration) and the > scheduler modules > - can remain RWLOCK, no changes in locking are needed > > __ip_vs_svc_lock: > - spin_lock, use RCU > - restrictions for schedulers with .update_service method > because svc->sched_lock is write locked, see below > > __ip_vs_rs_lock: > - spin_lock, use RCU > > Schedulers: > - every .schedule method has its own locking, two examples: > - write_lock: to protect the scheduler state (can be > changed to spin_lock), see WRR. Difficult for RCU. > - no lock: relies on IP_VS_WAIT_WHILE, no state > is protected explicitly, fast like RCU, see WLC > > Scheduler state, eg. mark->cl: > - careful RCU assignment, may be all .update_service methods > should use copy-on-update (WRR). OTOH, ip_vs_wlc_schedule (WLC) > has no locks at all, thanks to the IP_VS_WAIT_WHILE, so > it is fast as RCU. > > Statistics: > dest->stats.lock, svc->stats.lock, ip_vs_stats.lock: > - called for every packet, BAD for SMP, see ip_vs_in_stats(), > ip_vs_out_stats(), ip_vs_conn_stats() > > curr_sb_lock: > - called for every packet depending on conn state > - No benefits from RCU, should be spin_lock > > To summarize: > > - the main problem remains stats: > dest->stats.lock, svc->stats.lock, ip_vs_stats.lock > > - RCU benefits when connection processes many packets per connection, eg. > for TCP, SCTP, not much for UDP. No gains for the 1st > packet in connection. > > - svc: no benefits from RCU, some schedulers protect state and > need exclusive access, others have no state (and they do not use > locks even now) Thanks for the list. It looks like a good basis for some conversion work. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
