| Message ID | 20210322123846.3024549-1-maximmi@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,net] bonding: Work around lockdep_is_held false positives | expand |
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 <maximmi@nvidia.com> >> Reviewed-by: Tariq Toukan <tariqt@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.fsf@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? > Thanks >
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 74cbbb22470b..b2fe4e93cb8e 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -4391,9 +4391,7 @@ int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave) int agg_id = 0; int ret = 0; -#ifdef CONFIG_LOCKDEP - WARN_ON(lockdep_is_held(&bond->mode_lock)); -#endif + WARN_ON(spin_is_locked(&bond->mode_lock)); usable_slaves = kzalloc(struct_size(usable_slaves, arr, bond->slave_cnt), GFP_KERNEL);
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 <maximmi@nvidia.com> Reviewed-by: Tariq Toukan <tariqt@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(). However, there is no reliable way to check that some lock is not held without taking it ourselves (because the lock may be taken by another thread after the check). Could someone explain why this code tries to make this check? Maybe we could figure out some better way to achieve the original goal. drivers/net/bonding/bond_main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)