Re: [PATCH] netfilter: use per-cpu recursive lock (v11)

[Stephen Hemminger <shemminger@vyatta.com>]

On Tue, 21 Apr 2009 21:10:07 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Stephen Hemminger <shemminger@vyatta.com> wrote:
> 
> > +void xt_info_wrlock_bh(void)
> > +{
> > +	unsigned int i;
> > +
> > +	local_bh_disable();
> > +	for_each_possible_cpu(i) {
> > +		write_lock(&per_cpu(xt_info_locks, i));
> > +#if NR_CPUS > (PREEMPT_MASK - 1)
> > +		/*
> > +		 * Since spin_lock disables preempt, the following is
> > +		 * required to avoid overflowing the preempt counter
> > +		 */
> > +		preempt_enable_no_resched();
> > +#endif
> > +	}
> > +}
> 
> hm, this is rather ugly and it will make a lot of instrumentation 
> code explode.

Better general solutions:
    * use raw_spin_lock
    * increase PREEMPT_BITS on 64 bit machine 
      and limit to 128 CPU or less on 32 bit
    * get rid of default preempt_disable in spin_lock

You choose. It is a general problem.

> Why not use the obvious solution: a _single_ wrlock for global 
> access and read_can_lock() plus per cpu locks in the fastpath?
> 
> That way there's no global cacheline bouncing (just the _reading_ of 
> a global cacheline - which will be nicely localized - on NUMA too) - 
> and we will hold at most 1-2 locks at once!
> 
> Something like:
> 
> 	__cacheline_aligned DEFINE_RWLOCK(global_wrlock);
> 
> 	DEFINE_PER_CPU(rwlock_t local_lock);
> 
> 
> 	void local_read_lock(void)
> 	{
> 	again:
> 		read_lock(&per_cpu(local_lock, this_cpu));
> 
> 		if (unlikely(!read_can_lock(&global_wrlock))) {
> 			read_unlock(&per_cpu(local_lock, this_cpu));
> 			/*
> 			 * Just wait for any global write activity:
> 			 */
> 			read_unlock_wait(&global_wrlock);
> 			goto again;
> 		}
> 	}

Quit trying to be so damn f*cking cool. We don't build the software
for locking instrumentation. Locking instrumentation needs to serve
the kernel, not the other way around. 

Your version fails for the case of nested local rules. Which was
the whole reason I switched to read/writer locks.

        CPU 1                            CPU 2
        local_read_lock
        ...                              global_write_lock
            local_read_lock
Stuck...

CPU 2 is waiting for CPU 1 to get out of its nested table processing
CPU 1 is waiting for write lock to get done