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

[Linus Torvalds]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/