Re: [PATCH] netfilter: use per-cpu spinlock rather than RCU (v3)

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Fri, Apr 17, 2009 at 08:12:14AM +0200, Peter Zijlstra wrote:
> On Thu, 2009-04-16 at 18:28 -0700, Paul E. McKenney wrote:
> > On Thu, Apr 16, 2009 at 04:49:55PM -0700, Paul E. McKenney wrote:
> > > On Thu, Apr 16, 2009 at 03:33:54PM -0700, David Miller wrote:
> > > > From: Patrick McHardy <kaber@trash.net>
> > > > Date: Thu, 16 Apr 2009 15:11:31 +0200
> > > > 
> > > > > Linus Torvalds wrote:
> > > > >> On Wed, 15 Apr 2009, Stephen Hemminger wrote:
> > > > >>> The counters are the bigger problem, otherwise we could just free
> > > > >>> table
> > > > >>> info via rcu.  Do we really have to support: replace where the counter
> > > > >>> values coming out to user space are always exactly accurate, or is it
> > > > >>> allowed to replace a rule and maybe lose some counter ticks (worst
> > > > >>> case
> > > > >>> NCPU-1).
> > > > >> Why not just read the counters fromt he old one at RCU free time (they
> > > > >> are guaranteed to be stable at that point, since we're all done with
> > > > >> those entries), and apply them at that point to the current setup?
> > > > > 
> > > > > We need the counters immediately to copy them to userspace, so waiting
> > > > > for an asynchronous RCU free is not going to work.
> > > > 
> > > > It just occurred to me that since all netfilter packet handling
> > > > goes through one place, we could have a sort-of "netfilter RCU"
> > > > of sorts to solve this problem.
> > > 
> > > OK, I am putting one together...
> > > 
> > > It will be needed sooner or later, though I suspect per-CPU locking
> > > would work fine in this case.
> > 
> > And here is a crude first cut.  Untested, probably does not even compile.
> > 
> > Straight conversion of Mathieu Desnoyers's user-space RCU implementation
> > at git://lttng.org/userspace-rcu.git to the kernel (and yes, I did help
> > a little, but he must bear the bulk of the guilt).  Pick on srcu.h
> > and srcu.c out of sheer laziness.  User-space testing gives deep
> > sub-microsecond grace-period latencies, so should be fast enough, at
> > least if you don't mind two smp_call_function() invocations per grace
> > period and spinning on each instance of a per-CPU variable.
> > 
> > Again, I believe per-CPU locking should work fine for the netfilter
> > counters, but I guess "friends don't let friends use hashed locks".
> > (I would not know for sure, never having used them myself, except of
> > course to protect hash tables.)
> > 
> > Most definitely -not- for inclusion at this point.  Next step is to hack
> > up the relevant rcutorture code and watch it explode on contact.  ;-)
> 
> One comment, its again a global thing..
> 
> I've been playing with the idea for a while now to make all RCU
> implementations into proper objects so that you can do things like:
> 
>   struct atomic_rcu_domain my_rcu_domain = create_atomic_rcu();
> 
>   atomic_rcu_read_lock(&my_rcu_domain());
>   ...
> 
>   atomic_rcu_read_unlock(&my_rcu_domain());
> 
> and
> 
>   call_atomic_rcu(&my_rcu_domain, &my_obj->rcu_head, do_something);
> 
> etc..
> 
> We would have:
> 
>   atomic_rcu  --  'classic' non preemptible RCU (treercu these days)
>   sleep_rcu   --  'preemptible' RCU
> 
> Then have 3 default domains:
> 
> sched_rcu     -- always atomic_rcu

This is the call_rcu_sched() variant.

> rcu           -- depends on PREEMPT_RCU

This is the call_rcu() variant.

> preempt_rcu   -- always sleep_rcu

I guess that this one could allow sleeping on mutexes...  Does anyone
need to do that?

> This would allow generic code to:
>   1) use preemptible RCU for those cases where needed
>   2) create smaller RCU domains where needed, such as in this case
>   3) mostly do away with SRCU

#3 would be good!  But...

At an API level, there are two differences between SRCU and the other
RCU implementations:

a.	The return value from srcu_read_lock() is passed to
	srcu_read_unlock().

b.	There is a control block passed in to each SRCU primitive.

Difference (a) could potentially be taken care of with a few tricks I
am trying in the process of getting preemptrcu merged into treercu.

Your approach to (b) certainly makes it uniform, there are >500
occurrences of rcu_read_lock() and rcu_read_unlock() each, but only
a very few occurrences of srcu_read_lock() and srcu_read_unlock()
(like exactly one each!).  So adding an argument to rcu_read_lock()
does not sound at all reasonable.

> Now I realize that the presented RCU implementation has a different
> grace period method than the existing ones that use the timer tick to
> drive the state machine, so 2) might not be too relevant here. But maybe
> we can do something with different grace periods too.
> 
> Anyway, just an idea because I always get a little offended at the hard
> coded global variables in all these RCU implementations :-)

I am thinking in terms of adding a synchronize_rcu_bh() with the desired
properties.  That way we avoid yet another RCU flavor.  (What can I say?
I got carried away!)  Also, since the rcu-bh flavor is used only by
networking, we have a fair amount of freedom to tweak it.  It will take
longer than introducing a new flavor, but Steve Hemminger has a good
solution already, and RCU really isn't the thing to do quick hacks on.

							Thanx, Paul