Skip to content

Commit 22546b7

Browse files
tstrukdledford
tstruk
authored and
dledford
committed
IB/hfi1: Fix softlockup issue
Soft lockups can occur because the mad processing on different CPUs acquire the spin lock dc8051_lock: [534552.835870] [<ffffffffa026f993>] ? read_dev_port_cntr.isra.37+0x23/0x160 [hfi1] [534552.835880] [<ffffffffa02775af>] read_dev_cntr+0x4f/0x60 [hfi1] [534552.835893] [<ffffffffa028d7cd>] pma_get_opa_portstatus+0x64d/0x8c0 [hfi1] [534552.835904] [<ffffffffa0290e7d>] hfi1_process_mad+0x48d/0x18c0 [hfi1] [534552.835908] [<ffffffff811dc1f1>] ? __slab_free+0x81/0x2f0 [534552.835936] [<ffffffffa024c34e>] ? ib_mad_recv_done+0x21e/0xa30 [ib_core] [534552.835939] [<ffffffff811dd153>] ? __kmalloc+0x1f3/0x240 [534552.835947] [<ffffffffa024c3fb>] ib_mad_recv_done+0x2cb/0xa30 [ib_core] [534552.835955] [<ffffffffa0237c85>] __ib_process_cq+0x55/0xd0 [ib_core] [534552.835962] [<ffffffffa0237d70>] ib_cq_poll_work+0x20/0x60 [ib_core] [534552.835964] [<ffffffff810a7f3b>] process_one_work+0x17b/0x470 [534552.835966] [<ffffffff810a8d76>] worker_thread+0x126/0x410 [534552.835969] [<ffffffff810a8c50>] ? rescuer_thread+0x460/0x460 [534552.835971] [<ffffffff810b052f>] kthread+0xcf/0xe0 [534552.835974] [<ffffffff810b0460>] ? kthread_create_on_node+0x140/0x140 [534552.835977] [<ffffffff81696418>] ret_from_fork+0x58/0x90 [534552.835980] [<ffffffff810b0460>] ? kthread_create_on_node+0x140/0x140 This issue is made worse when the 8051 is busy and the reads take longer. Fix by using a non-spinning lock procure. Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> Reviewed-by: Mike Marciszyn <mike.marciniszyn@intel.com> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com> Signed-off-by: Doug Ledford <dledford@redhat.com>
1 parent b6eac93 commit 22546b7

File tree

3 files changed

+57
-38
lines changed

3 files changed

+57
-38
lines changed

‎drivers/infiniband/hw/hfi1/chip.c

Lines changed: 52 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -6410,18 +6410,17 @@ static void lcb_shutdown(struct hfi1_devdata *dd, int abort)
64106410
*
64116411
* The expectation is that the caller of this routine would have taken
64126412
* care of properly transitioning the link into the correct state.
6413+
* NOTE: the caller needs to acquire the dd->dc8051_lock lock
6414+
* before calling this function.
64136415
*/
6414-
static void dc_shutdown(struct hfi1_devdata *dd)
6416+
static void _dc_shutdown(struct hfi1_devdata *dd)
64156417
{
6416-
unsigned long flags;
6418+
lockdep_assert_held(&dd->dc8051_lock);
64176419

6418-
spin_lock_irqsave(&dd->dc8051_lock, flags);
6419-
if (dd->dc_shutdown) {
6420-
spin_unlock_irqrestore(&dd->dc8051_lock, flags);
6420+
if (dd->dc_shutdown)
64216421
return;
6422-
}
6422+
64236423
dd->dc_shutdown = 1;
6424-
spin_unlock_irqrestore(&dd->dc8051_lock, flags);
64256424
/* Shutdown the LCB */
64266425
lcb_shutdown(dd, 1);
64276426
/*
@@ -6432,35 +6431,45 @@ static void dc_shutdown(struct hfi1_devdata *dd)
64326431
write_csr(dd, DC_DC8051_CFG_RST, 0x1);
64336432
}
64346433

6434+
static void dc_shutdown(struct hfi1_devdata *dd)
6435+
{
6436+
mutex_lock(&dd->dc8051_lock);
6437+
_dc_shutdown(dd);
6438+
mutex_unlock(&dd->dc8051_lock);
6439+
}
6440+
64356441
/*
64366442
* Calling this after the DC has been brought out of reset should not
64376443
* do any damage.
6444+
* NOTE: the caller needs to acquire the dd->dc8051_lock lock
6445+
* before calling this function.
64386446
*/
6439-
static void dc_start(struct hfi1_devdata *dd)
6447+
static void _dc_start(struct hfi1_devdata *dd)
64406448
{
6441-
unsigned long flags;
6442-
int ret;
6449+
lockdep_assert_held(&dd->dc8051_lock);
64436450

6444-
spin_lock_irqsave(&dd->dc8051_lock, flags);
64456451
if (!dd->dc_shutdown)
6446-
goto done;
6447-
spin_unlock_irqrestore(&dd->dc8051_lock, flags);
6452+
return;
6453+
64486454
/* Take the 8051 out of reset */
64496455
write_csr(dd, DC_DC8051_CFG_RST, 0ull);
64506456
/* Wait until 8051 is ready */
6451-
ret = wait_fm_ready(dd, TIMEOUT_8051_START);
6452-
if (ret) {
6457+
if (wait_fm_ready(dd, TIMEOUT_8051_START))
64536458
dd_dev_err(dd, "%s: timeout starting 8051 firmware\n",
64546459
__func__);
6455-
}
6460+
64566461
/* Take away reset for LCB and RX FPE (set in lcb_shutdown). */
64576462
write_csr(dd, DCC_CFG_RESET, 0x10);
64586463
/* lcb_shutdown() with abort=1 does not restore these */
64596464
write_csr(dd, DC_LCB_ERR_EN, dd->lcb_err_en);
6460-
spin_lock_irqsave(&dd->dc8051_lock, flags);
64616465
dd->dc_shutdown = 0;
6462-
done:
6463-
spin_unlock_irqrestore(&dd->dc8051_lock, flags);
6466+
}
6467+
6468+
static void dc_start(struct hfi1_devdata *dd)
6469+
{
6470+
mutex_lock(&dd->dc8051_lock);
6471+
_dc_start(dd);
6472+
mutex_unlock(&dd->dc8051_lock);
64646473
}
64656474

64666475
/*
@@ -8513,16 +8522,11 @@ static int do_8051_command(
85138522
{
85148523
u64 reg, completed;
85158524
int return_code;
8516-
unsigned long flags;
85178525
unsigned long timeout;
85188526

85198527
hfi1_cdbg(DC8051, "type %d, data 0x%012llx", type, in_data);
85208528

8521-
/*
8522-
* Alternative to holding the lock for a long time:
8523-
* - keep busy wait - have other users bounce off
8524-
*/
8525-
spin_lock_irqsave(&dd->dc8051_lock, flags);
8529+
mutex_lock(&dd->dc8051_lock);
85268530

85278531
/* We can't send any commands to the 8051 if it's in reset */
85288532
if (dd->dc_shutdown) {
@@ -8548,10 +8552,8 @@ static int do_8051_command(
85488552
return_code = -ENXIO;
85498553
goto fail;
85508554
}
8551-
spin_unlock_irqrestore(&dd->dc8051_lock, flags);
8552-
dc_shutdown(dd);
8553-
dc_start(dd);
8554-
spin_lock_irqsave(&dd->dc8051_lock, flags);
8555+
_dc_shutdown(dd);
8556+
_dc_start(dd);
85558557
}
85568558

85578559
/*
@@ -8632,8 +8634,7 @@ static int do_8051_command(
86328634
write_csr(dd, DC_DC8051_CFG_HOST_CMD_0, 0);
86338635

86348636
fail:
8635-
spin_unlock_irqrestore(&dd->dc8051_lock, flags);
8636-
8637+
mutex_unlock(&dd->dc8051_lock);
86378638
return return_code;
86388639
}
86398640

@@ -12007,6 +12008,10 @@ static void free_cntrs(struct hfi1_devdata *dd)
1200712008
dd->scntrs = NULL;
1200812009
kfree(dd->cntrnames);
1200912010
dd->cntrnames = NULL;
12011+
if (dd->update_cntr_wq) {
12012+
destroy_workqueue(dd->update_cntr_wq);
12013+
dd->update_cntr_wq = NULL;
12014+
}
1201012015
}
1201112016

1201212017
static u64 read_dev_port_cntr(struct hfi1_devdata *dd, struct cntr_entry *entry,
@@ -12162,7 +12167,7 @@ u64 write_port_cntr(struct hfi1_pportdata *ppd, int index, int vl, u64 data)
1216212167
return write_dev_port_cntr(ppd->dd, entry, sval, ppd, vl, data);
1216312168
}
1216412169

12165-
static void update_synth_timer(unsigned long opaque)
12170+
static void do_update_synth_timer(struct work_struct *work)
1216612171
{
1216712172
u64 cur_tx;
1216812173
u64 cur_rx;
@@ -12171,8 +12176,8 @@ static void update_synth_timer(unsigned long opaque)
1217112176
int i, j, vl;
1217212177
struct hfi1_pportdata *ppd;
1217312178
struct cntr_entry *entry;
12174-
12175-
struct hfi1_devdata *dd = (struct hfi1_devdata *)opaque;
12179+
struct hfi1_devdata *dd = container_of(work, struct hfi1_devdata,
12180+
update_cntr_work);
1217612181

1217712182
/*
1217812183
* Rather than keep beating on the CSRs pick a minimal set that we can
@@ -12255,7 +12260,13 @@ static void update_synth_timer(unsigned long opaque)
1225512260
} else {
1225612261
hfi1_cdbg(CNTR, "[%d] No update necessary", dd->unit);
1225712262
}
12263+
}
1225812264

12265+
static void update_synth_timer(unsigned long opaque)
12266+
{
12267+
struct hfi1_devdata *dd = (struct hfi1_devdata *)opaque;
12268+
12269+
queue_work(dd->update_cntr_wq, &dd->update_cntr_work);
1225912270
mod_timer(&dd->synth_stats_timer, jiffies + HZ * SYNTH_CNT_TIME);
1226012271
}
1226112272

@@ -12491,6 +12502,13 @@ static int init_cntrs(struct hfi1_devdata *dd)
1249112502
if (init_cpu_counters(dd))
1249212503
goto bail;
1249312504

12505+
dd->update_cntr_wq = alloc_ordered_workqueue("hfi1_update_cntr_%d",
12506+
WQ_MEM_RECLAIM, dd->unit);
12507+
if (!dd->update_cntr_wq)
12508+
goto bail;
12509+
12510+
INIT_WORK(&dd->update_cntr_work, do_update_synth_timer);
12511+
1249412512
mod_timer(&dd->synth_stats_timer, jiffies + HZ * SYNTH_CNT_TIME);
1249512513
return 0;
1249612514
bail:

‎drivers/infiniband/hw/hfi1/hfi.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,7 @@ struct rvt_sge_state;
484484
#define HFI1_PART_ENFORCE_OUT 0x2
485485

486486
/* how often we check for synthetic counter wrap around */
487-
#define SYNTH_CNT_TIME 2
487+
#define SYNTH_CNT_TIME 3
488488

489489
/* Counter flags */
490490
#define CNTR_NORMAL 0x0 /* Normal counters, just read register */
@@ -962,8 +962,9 @@ struct hfi1_devdata {
962962
spinlock_t rcvctrl_lock; /* protect changes to RcvCtrl */
963963
/* around rcd and (user ctxts) ctxt_cnt use (intr vs free) */
964964
spinlock_t uctxt_lock; /* rcd and user context changes */
965-
/* exclusive access to 8051 */
966-
spinlock_t dc8051_lock;
965+
struct mutex dc8051_lock; /* exclusive access to 8051 */
966+
struct workqueue_struct *update_cntr_wq;
967+
struct work_struct update_cntr_work;
967968
/* exclusive access to 8051 memory */
968969
spinlock_t dc8051_memlock;
969970
int dc8051_timed_out; /* remember if the 8051 timed out */

‎drivers/infiniband/hw/hfi1/init.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1081,11 +1081,11 @@ struct hfi1_devdata *hfi1_alloc_devdata(struct pci_dev *pdev, size_t extra)
10811081
spin_lock_init(&dd->uctxt_lock);
10821082
spin_lock_init(&dd->hfi1_diag_trans_lock);
10831083
spin_lock_init(&dd->sc_init_lock);
1084-
spin_lock_init(&dd->dc8051_lock);
10851084
spin_lock_init(&dd->dc8051_memlock);
10861085
seqlock_init(&dd->sc2vl_lock);
10871086
spin_lock_init(&dd->sde_map_lock);
10881087
spin_lock_init(&dd->pio_map_lock);
1088+
mutex_init(&dd->dc8051_lock);
10891089
init_waitqueue_head(&dd->event_queue);
10901090

10911091
dd->int_counter = alloc_percpu(u64);

0 commit comments

Comments
 (0)