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 07:16:32AM +0200, Eric Dumazet wrote:
> Stephen Hemminger a écrit :
> > On Thu, 16 Apr 2009 18:28:12 -0700
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> 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.  ;-)
> >>
> >> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > I am glad to see this worked on, but would rather not use RCU in this case
> > of iptables. It would be good for some of the other long grace period sutff.
> > 
> > The code to per-cpu entry consolidation by alloc/flip in 2.6.30-rc2 was
> > hard to debug and more convoluted so it probably would be a long term maintaince
> > nightmare.  The issue was the variable size skip structure so it made
> > for lots of iterators, etc. If the non-RCU per-cpu spinlock version is just
> > as fast, it is easier to understand.
> 
> I agree that for 2.6.30, we could use a per-cpu spinlock as your last patch did,
> this would be very risky to push this new RCU right now. 

I completely agree that this RCU is absolutely -not- 2.6.30 material.  ;-)

> But this new stuff looks very promising, (no more locked ops on fast path),
> and considering new percpu_{add|sub...} infra, very fast :
> 
> static inline void rcu_read_unlock_fgp(void)
> {
> 	barrier();
> 	percpu_sub(rcu_fgp_active_readers, 1); /* one instruction on x86 */
> 	preempt_enable();
> }

Very cool!!!  If I had seen this, I had forgotten about it.  I will
give it a try, but only after getting it working the old way.  (What,
me paranoid?)

> I wonder if IPI are really necessary on x86 if we use percpu_sub() since
> it already contains a barrier, and rcu_read_lock_fgp(void) also ends with
> a barrier() call...

Hmmmm...  But x86 can still execute a later load before an earlier
store, so it seems to me that there would be the potential for even
an x86 CPU to pull loads from the critical section up before the final
store of the percpu_sub(), right?  If so, we really do still need the
IPIs on x86.

							Thanx, Paul