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

Andrew Morton
2009-04-13T23:35:55Z

On Mon, 13 Apr 2009 16:20:00 -0700
Stephen Hemminger <shemminger@vyatta.com> wrote:

> On Mon, 13 Apr 2009 15:24:37 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
>
> > On Mon, 13 Apr 2009 09:53:09 -0700
> > Stephen Hemminger <shemminger@vyatta.com> wrote:
> >
> > > This is an alternative version of ip/ip6/arp tables locking using
> > > per-cpu locks. This avoids the overhead of synchronize_net() during
> > > update but still removes the expensive rwlock in earlier versions.
> > >
> > > The idea for this came from an earlier version done by Eric Duzamet.
> > > Locking is done per-cpu, the fast path locks on the current cpu
> > > and updates counters. The slow case involves acquiring the locks on
> > > all cpu's.
> > >
> > > The mutex that was added for 2.6.30 in xt_table is unnecessary since
> > > there already is a mutex for xt[af].mutex that is held.
> > >
> > > Tested basic functionality (add/remove/list), but don't have test cases
> > > for stress, ip6tables or arptables.
> > >
> > > unsigned int
> > > ipt_do_table(struct sk_buff *skb,
> > > @@ -339,9 +341,10 @@ ipt_do_table(struct sk_buff *skb,
> > >
> > > IP_NF_ASSERT(table->valid_hooks & (1 << hook));
> > >
> > > - rcu_read_lock_bh();
> > > - private = rcu_dereference(table->private);
> > > - table_base = rcu_dereference(private->entries[smp_processor_id()]);
> > > + local_bh_disable();
> > > + spin_lock(&__get_cpu_var(ip_tables_lock));
> >
> > spin_lock_bh()?
>
> No. get_cpu_var implies smp_processor_id which is not safe
> without preempt_disable (ie bh disable).

spin_lock_bh() will dtrt, but spelling it out seems a good idea.

It should have an explanatory comment, IMO.

> >
> > And it should be hotplug aware, really. num_possible_cpus() can exceed
> > num_online_cpus(). The extent by which possible>online is
> > controversial, but one can conceive of situations where it is "lots".
>
> It is doing right thing already with hotplug.

It's slow.

> This code still needs to count packets processed by previously online
> cpu, that is no longer there.

Those counts could be migrated off that CPU when it is offlined. As
percpucounter does.

> > Is lib/percpu_counter.c no good for this application? Unfixably no
> > good? That code automagically handles cpu hotplug.
>
> percpu_counter can't deal with the layout/load here.

Insufficient detail here for anyone to understand why percpucounter
cannot be adapted to this requirement.