| Message ID | 20211013141852.128104.2682.stgit@awfm-01.cornelisnetworks.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-next] IB/hfi1: Fix abba locking issue with sc_disable() | expand |
On Wed, Oct 13, 2021 at 10:18:52AM -0400, Dennis Dalessandro wrote: > From: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com> > > sc_disable() after having disabled the send context wakes up > any waiters by calling hfi1_qp_wakeup() while holding the > waitlock for the sc. > > This is contrary to the model for all other calls to hfi1_qp_wakeup() > where the waitlock is dropped and a local is used to drive calls > to hfi1_qp_wakeup(). > > Fix by moving the sc->piowait into a local list and driving the wakeup > calls from the list. > > Signed-off-by: Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com> > Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com> > drivers/infiniband/hw/hfi1/pio.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/hw/hfi1/pio.c b/drivers/infiniband/hw/hfi1/pio.c > index 489b436..3d42bd2 100644 > +++ b/drivers/infiniband/hw/hfi1/pio.c > @@ -878,6 +878,7 @@ void sc_disable(struct send_context *sc) > { > u64 reg; > struct pio_buf *pbuf; > + LIST_HEAD(wake_list); > > if (!sc) > return; > @@ -912,19 +913,21 @@ void sc_disable(struct send_context *sc) > spin_unlock(&sc->release_lock); > > write_seqlock(&sc->waitlock); > - while (!list_empty(&sc->piowait)) { > + if (!list_empty(&sc->piowait)) > + list_move(&sc->piowait, &wake_list); > + write_sequnlock(&sc->waitlock); > + while (!list_empty(&wake_list)) { > struct iowait *wait; > struct rvt_qp *qp; > struct hfi1_qp_priv *priv; > > - wait = list_first_entry(&sc->piowait, struct iowait, list); > + wait = list_first_entry(&wake_list, struct iowait, list); > qp = iowait_to_qp(wait); > priv = qp->priv; > list_del_init(&priv->s_iowait.list); > priv->s_iowait.lock = NULL; > hfi1_qp_wakeup(qp, RVT_S_WAIT_PIO | HFI1_S_WAIT_PIO_DRAIN); > } > - write_sequnlock(&sc->waitlock); clangd tells me there is no read side to this seqlock? Why is it a seqlock if everything is a write side? Indeed I was wondering because I don't know of any seq lock safe algorithm for list manipulation - but it turns out this is just an obfuscated spinlock. Please send a patch to fix it. Applied to for-rc Thanks, Jason
diff --git a/drivers/infiniband/hw/hfi1/pio.c b/drivers/infiniband/hw/hfi1/pio.c index 489b436..3d42bd2 100644 --- a/drivers/infiniband/hw/hfi1/pio.c +++ b/drivers/infiniband/hw/hfi1/pio.c @@ -878,6 +878,7 @@ void sc_disable(struct send_context *sc) { u64 reg; struct pio_buf *pbuf; + LIST_HEAD(wake_list); if (!sc) return; @@ -912,19 +913,21 @@ void sc_disable(struct send_context *sc) spin_unlock(&sc->release_lock); write_seqlock(&sc->waitlock); - while (!list_empty(&sc->piowait)) { + if (!list_empty(&sc->piowait)) + list_move(&sc->piowait, &wake_list); + write_sequnlock(&sc->waitlock); + while (!list_empty(&wake_list)) { struct iowait *wait; struct rvt_qp *qp; struct hfi1_qp_priv *priv; - wait = list_first_entry(&sc->piowait, struct iowait, list); + wait = list_first_entry(&wake_list, struct iowait, list); qp = iowait_to_qp(wait); priv = qp->priv; list_del_init(&priv->s_iowait.list); priv->s_iowait.lock = NULL; hfi1_qp_wakeup(qp, RVT_S_WAIT_PIO | HFI1_S_WAIT_PIO_DRAIN); } - write_sequnlock(&sc->waitlock); spin_unlock_irq(&sc->alloc_lock); }