Re: [GIT] Networking

David Miller
2010-08-16T19:35:51Z

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 15 Aug 2010 16:47:17 +0200

> Le dimanche 15 août 2010 à 12:55 +0200, Eric Dumazet a écrit :
>
>> We have one lock per cpu, and only one cpu can possibly lock its
>> associated lock under softirq. So the usual lockdep check, warning a
>> lock is taken with BH enabled, while same lock was taken inside softirq
>> handler is triggering a false positive here.
>>
>> I believe no existing lockdep annotation can instruct lockdep this use
>> is OK, I guess we have following choice :
>>
>> 1) Mask BH again, using xt_info_wrlock_lockdep(cpu) instead of
>> xt_info_wrlock(cpu).
>>
>> xt_info_wrlock_lockdep() being a variant, that disables BH in case
>> CONFIG_PROVE_LOCKING=y
>>
>> 2) temporally switch off lockdep in get_counters(), using a
>> lockdep_off()/lockdep_on() pair, and a comment why this is necessary.
>>
>
> In any case, here is patch implementing the later

I'm hesistent to say that we should put this kind of patch in.

It will shut up lockdep for this specific case, but it also means
that if we do any other kinds of locking in this sequence we will
not validate it.

The valuable of this is open for debate I guess.

But locking is hard so I would say that disabling lockdep to kill a
warning it generates should be an absolute last resort.

I also don't think making the locking mechanics conditional upon
LOCKDEP is sane either, exactly because it means lockdep is testing
something other than what actually gets used in practice. :-)