netdev - Re: [PATCH] netfilter: use per-CPU r**ursive lock {XV}

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 28 Apr 2009 08:09:10 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Eric Dumazet <dada1@...mosbay.com>
cc:	Stephen Hemminger <shemminger@...tta.com>,
	Evgeniy Polyakov <zbr@...emap.net>,
	Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <peterz@...radead.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
	David Miller <davem@...emloft.net>,
	Jarek Poplawski <jarkao2@...il.com>,
	Paul Mackerras <paulus@...ba.org>, paulmck@...ux.vnet.ibm.com,
	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
Subject: Re: [PATCH] netfilter: use per-CPU r**ursive lock {XV}



On Tue, 28 Apr 2009, Eric Dumazet wrote:
> 
> Instead of submitting a full patch again, we could first submit a new
>  include file containg all comments and inline functions ?

Well, I actually already suggested to David that he should just merge the 
last patch I saw floating around (with the "recursive" -> "readwrite" fix 
in the comment ;), so that we can at least get the basic issue fixed, and 
then we can tweak it a bit with smaller patches flying around.

And at least right now, the difference between the rwlock and the 
"count+spinlock" should be basically almost unnoticeable, and a very small 
implementation issue. They're entirely interchangeable, after all.

> This include file could be local to netfilter, with a big stick on
> it to forbids its use on other areas (No changes in Documentation/ )
>
> Then, as soon as we can go back to pure RCU solution, we can safely
> delete this controversial-locking-nesting-per-cpu-thing ?

I don't have any strogn preferences, but I'd almost prefer to not abstract 
things out even that much. It's already pretty well hidden inside 
<netfilter/x_tables.h>, I'd hate to add a new file just for this. 

As to just adding more commenting that it must not be used anywhere else, 
I certainly agree with that.

> Instead of local/global name that Paul suggested, that was about
> 'global' locking all locks at the same time. Not any more the good
> name IMHO
> 
> Maybe something like local/remote or owner/foreigner ?

local/remote works for me, and yes, since we only take the remote side one 
CPU at a time, I guess "global" is misleading. But "owner/foreigner" 
sounds pretty odd.

> One comment about this comment you wrote :
> 
> /*
>  * The "writer" side needs to get exclusive access to the lock,
>  * regardless of readers.  This must be called with bottom half
>  * processing (and thus also preemption) disabled. 
>  */
> 
> Its true that BH should be disabled if caller runs
> on the cpu it wants to lock. 
> For other ones (true foreigners), there is
> no requirement about BH (current cpu could be interrupted
> by a softirq and packets could fly)

Yes. Other CPU's just require preemption protection. 

> We could use following construct and not require disabling BH
> more than a short period of time.
> (But preemption disabled for the whole duration)
> 
> preempt_disable(); // could be cpu_migration_disable();
> 
> int curcpu = smp_processor_id();
> /*
>  * Gather stats for current cpu : must disable BH
>  * before trying to lock.
>  */
> local_bh_disable();
> xt_info_wrlock(curcpu);
> // copy stats of this cpu on my private data (not shown here)
> xt_info_wrunlock(curcpu);
> local_bh_enable();
> 
> for_each_possible_cpu(cpu) {
> 	if (cpu == curcpu)
> 		continue;
> 	xt_info_wrlock(cpu);
> 	// fold stats of "cpu" on my private data (not shown here)
> 	xt_info_wrunlock((cpu);
> }
> preempt_enable(); // could be cpu_migration_enable();

Agreed. 

> So your initial comment could be changed to :
> 
> /*
>  * The "writer" side needs to get exclusive access to the lock,
>  * regardless of readers. If caller is about to lock its own lock,
>  * he must have disabled BH before. For other cpus, no special
>  * care but preemption disabled to guarantee no cpu migration.
>  */

Ack.

			Linus
--
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