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 netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
