Maxim Mikityanskiy <maxi...@nvidia.com> wrote:
>On 2021-03-22 16:09, Leon Romanovsky wrote:
>> On Mon, Mar 22, 2021 at 02:38:46PM +0200, Maxim Mikityanskiy wrote:
>>> After lockdep gets triggered for the first time, it gets disabled, and
>>> lockdep_enabled() will return false. It will affect lockdep_is_held(),
>>> which will start returning true all the time. Normally, it just disables
>>> checks that expect a lock to be held. However, the bonding code checks
>>> that a lock is NOT held, which triggers a false positive in WARN_ON.
>>>
>>> This commit addresses the issue by replacing lockdep_is_held with
>>> spin_is_locked, which should have the same effect, but without suffering
>>> from disabling lockdep.
>>>
>>> Fixes: ee6377147409 ("bonding: Simplify the xmit function for modes that
>>> use xmit_hash")
>>> Signed-off-by: Maxim Mikityanskiy <maxi...@nvidia.com>
>>> Reviewed-by: Tariq Toukan <tar...@nvidia.com>
>>> ---
>>> While this patch works around the issue, I would like to discuss better
>>> options. Another straightforward approach is to extend lockdep API with
>>> lockdep_is_not_held(), which will be basically !lockdep_is_held() when
>>> lockdep is enabled, but will return true when !lockdep_enabled().
>>
>> lockdep_assert_not_held() was added in this cycle to tip: locking/core
>> https://yhbt.net/lore/all/161475935945.20312.2870945278690244669.tip-bot2@tip-bot2/
>> https://yhbt.net/lore/all/878s779s9f....@codeaurora.org/
>
>Thanks for this suggestion - I wasn't aware that this macro was recently
>added and I could use it instead of spin_is_locked.
>
>Still, I would like to figure out why the bonding code does this test at
>all. This lock is not taken by bond_update_slave_arr() itself, so why is
>that a problem in this code?
The goal, I believe, is to insure that the mode_lock is not held
by the caller when entering bond_update_slave_arr. I suspect this is
because bond_update_slave_arr may sleep. One calling context notes this
in a comment:
void bond_3ad_handle_link_change(struct slave *slave, char link)
{
[...]
/* RTNL is held and mode_lock is released so it's safe
* to update slave_array here.
*/
bond_update_slave_arr(slave->bond, NULL);
However, as far as I can tell, lockdep_is_held() does not test
for "lock held by this particular context" but instead is "lock held by
any context at all." As such, I think the test is not valid, and should
be removed.
The code in question was added by:
commit ee6377147409a00c071b2da853059a7d59979fbc
Author: Mahesh Bandewar <mahe...@google.com>
Date: Sat Oct 4 17:45:01 2014 -0700
bonding: Simplify the xmit function for modes that use xmit_hash
Mahesh, Nikolay, any thoughts?
-J
---
-Jay Vosburgh, jay.vosbu...@canonical.com
