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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 21 Apr 2009 14:04:46 -0700
From:	Stephen Hemminger <shemminger@...tta.com>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Paul Mackerras <paulus@...ba.org>, paulmck@...ux.vnet.ibm.com,
	Eric Dumazet <dada1@...mosbay.com>,
	Evgeniy Polyakov <zbr@...emap.net>,
	David Miller <davem@...emloft.net>, kaber@...sh.net,
	jeff.chua.linux@...il.com, laijs@...fujitsu.com,
	jengelh@...ozas.de, r000n@...0n.net, linux-kernel@...r.kernel.org,
	netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
	benh@...nel.crashing.org, mathieu.desnoyers@...ymtl.ca
Subject: Re: [PATCH] netfilter: use per-cpu recursive lock (v11)

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

> 
> * Stephen Hemminger <shemminger@...tta.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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html