Re: [PATCH] x_tables : Use SMP friendly (percpu) rwlock_t to protect x_tables

[Eric Dumazet <dada1@cosmosbay.com>]

Stephen Hemminger a écrit :
> On Fri, 27 Jan 2006 16:46:05 +0100
> Eric Dumazet <dada1@cosmosbay.com> wrote:
> 
>> Stephen Hemminger a écrit :
>>> On Fri, 27 Jan 2006 16:16:15 +0100
>>> Eric Dumazet <dada1@cosmosbay.com> wrote:
>>>
>>>> This patch helps scalability of x_tables on SMP, especially NUMA machines.
>>>>
>>>> Instead of using a single rwlock_t to protect x_tables, use an array of 
>>>> rwlock_t, one for each possible cpu in the machine.
> 
> It makes no sense to have a per-cpu lock. Why not just use per-cpu counters
> and bh_disable()?
> 

If you examin the x_tables code, you'll discover :

A CPU A might enters the code (ipt_do_table()) because a packet is processed : 
it follows the rules, increment 64 bits packet counts and 64 bits bytes 
counters of matched rules... Then it leaves the code.
For this stage a read_lock_bh() was used to let this cpu does its job 
(including recursive calls apparently)

Then a CPU B wants to either :

[1] - replace the rules by a new set, atomically keeping all counters values
    (example : iptables -A INPUT -p tcp)
[2] - Read all the rules and associated counters atomically.
     (example : iptables -nvL INPUT)
[...] - Other interesting things like resetting al counters
     (example : iptables -Z)
This kind of task uses a write_lock_bh() to be sure CPU A will block in 
read_lock_bh()

Then a CPU C wants to to a task similar to B (imagine you have a periodic 
fetch of counters to feed a RRD database) : you want that CPU C doesnt read 
garbage values when CPU B adds a new rule in a chain, and while CPU A feed 
more network packets...


You need some kind of exclusion so that CPU B reads sane values, not values 
currently modified by CPU A. You need some kind of exclusion to let CPU B 
doing some table exchange (and keeps counters).

counters are already 'per-cpu' (ie each CPU only reads its own copy of rules 
and change its own counters), and bh_disable() wont permit exclusion between CPUS.

And finally you want to keep compatibility with user programs and existing API...

So using a per-cpu rwlock does make sense...

Eric