Skip to content

Commit 120f28a

Browse files
pkitszelanguy11
authored andcommitted
iavf: get rid of the crit lock
Get rid of the crit lock. That frees us from the error prone logic of try_locks. Thanks to netdev_lock() by Jakub it is now easy, and in most cases we were protected by it already - replace crit lock by netdev lock when it was not the case. Lockdep reports that we should cancel the work under crit_lock [splat1], and that was the scheme we have mostly followed since [1] by Slawomir. But when that is done we still got into deadlocks [splat2]. So instead we should look at the bigger problem, namely "weird locking/scheduling" of the iavf. The first step to fix that is to remove the crit lock. I will followup with a -next series that simplifies scheduling/tasks. Cancel the work without netdev lock (weird unlock+lock scheme), to fix the [splat2] (which would be totally ugly if we would kept the crit lock). Extend protected part of iavf_watchdog_task() to include scheduling more work. Note that the removed comment in iavf_reset_task() was misplaced, it belonged to inside of the removed if condition, so it's gone now. [splat1] - w/o this patch - The deadlock during VF removal: WARNING: possible circular locking dependency detected sh/3825 is trying to acquire lock: ((work_completion)(&(&adapter->watchdog_task)->work)){+.+.}-{0:0}, at: start_flush_work+0x1a1/0x470 but task is already holding lock: (&adapter->crit_lock){+.+.}-{4:4}, at: iavf_remove+0xd1/0x690 [iavf] which lock already depends on the new lock. [splat2] - when cancelling work under crit lock, w/o this series, see [2] for the band aid attempt WARNING: possible circular locking dependency detected sh/3550 is trying to acquire lock: ((wq_completion)iavf){+.+.}-{0:0}, at: touch_wq_lockdep_map+0x26/0x90 but task is already holding lock: (&dev->lock){+.+.}-{4:4}, at: iavf_remove+0xa6/0x6e0 [iavf] which lock already depends on the new lock. [1] fc2e6b3 ("iavf: Rework mutexes for better synchronisation") [2] pkitszel@52dddbfc2bb60294083f5711a158a Fixes: d1639a1 ("iavf: fix a deadlock caused by rtnl and driver's lock circular dependencies") Signed-off-by: Przemek Kitszel <[email protected]> Reviewed-by: Aleksandr Loktionov <[email protected]> Tested-by: Rafal Romanowski <[email protected]> Signed-off-by: Tony Nguyen <[email protected]>
1 parent 05702b5 commit 120f28a

File tree

3 files changed

+38
-151
lines changed

3 files changed

+38
-151
lines changed

drivers/net/ethernet/intel/iavf/iavf.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,6 @@ struct iavf_adapter {
268268
struct list_head vlan_filter_list;
269269
int num_vlan_filters;
270270
struct list_head mac_filter_list;
271-
struct mutex crit_lock;
272271
/* Lock to protect accesses to MAC and VLAN lists */
273272
spinlock_t mac_vlan_list_lock;
274273
char misc_vector_name[IFNAMSIZ + 9];

drivers/net/ethernet/intel/iavf/iavf_ethtool.c

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1258,7 +1258,6 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx
12581258
{
12591259
struct ethtool_rx_flow_spec *fsp = &cmd->fs;
12601260
struct iavf_fdir_fltr *fltr;
1261-
int count = 50;
12621261
int err;
12631262

12641263
netdev_assert_locked(adapter->netdev);
@@ -1281,22 +1280,13 @@ static int iavf_add_fdir_ethtool(struct iavf_adapter *adapter, struct ethtool_rx
12811280
if (!fltr)
12821281
return -ENOMEM;
12831282

1284-
while (!mutex_trylock(&adapter->crit_lock)) {
1285-
if (--count == 0) {
1286-
kfree(fltr);
1287-
return -EINVAL;
1288-
}
1289-
udelay(1);
1290-
}
1291-
12921283
err = iavf_add_fdir_fltr_info(adapter, fsp, fltr);
12931284
if (!err)
12941285
err = iavf_fdir_add_fltr(adapter, fltr);
12951286

12961287
if (err)
12971288
kfree(fltr);
12981289

1299-
mutex_unlock(&adapter->crit_lock);
13001290
return err;
13011291
}
13021292

@@ -1439,9 +1429,9 @@ iavf_set_adv_rss_hash_opt(struct iavf_adapter *adapter,
14391429
{
14401430
struct iavf_adv_rss *rss_old, *rss_new;
14411431
bool rss_new_add = false;
1442-
int count = 50, err = 0;
14431432
bool symm = false;
14441433
u64 hash_flds;
1434+
int err = 0;
14451435
u32 hdrs;
14461436

14471437
netdev_assert_locked(adapter->netdev);
@@ -1469,15 +1459,6 @@ iavf_set_adv_rss_hash_opt(struct iavf_adapter *adapter,
14691459
return -EINVAL;
14701460
}
14711461

1472-
while (!mutex_trylock(&adapter->crit_lock)) {
1473-
if (--count == 0) {
1474-
kfree(rss_new);
1475-
return -EINVAL;
1476-
}
1477-
1478-
udelay(1);
1479-
}
1480-
14811462
spin_lock_bh(&adapter->adv_rss_lock);
14821463
rss_old = iavf_find_adv_rss_cfg_by_hdrs(adapter, hdrs);
14831464
if (rss_old) {
@@ -1506,8 +1487,6 @@ iavf_set_adv_rss_hash_opt(struct iavf_adapter *adapter,
15061487
if (!err)
15071488
iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_ADD_ADV_RSS_CFG);
15081489

1509-
mutex_unlock(&adapter->crit_lock);
1510-
15111490
if (!rss_new_add)
15121491
kfree(rss_new);
15131492

0 commit comments

Comments
 (0)