| Message ID | 20220504202817.98247-1-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-next,v2] RDMA/rxe: Fix "RDMA/rxe: Cleanup rxe_mcast.c" | expand |
On Wed, May 04, 2022 at 03:28:17PM -0500, Bob Pearson wrote: > rxe_mcast.c currently uses _irqsave spinlocks for rxe->mcg_lock > while rxe_recv.c uses _bh spinlocks for the same lock. > > Additionally the current code issues a warning that _irqrestore > is restoring hardware interrupts while some interrupts are > enabled. This is traced to calls to the calls to dev_mc_add/del(). > > Change the locking of rxe->mcg_lock in rxe_mcast.c to use > spin_(un)lock_bh() which matches that in rxe_recv.c. Also move > the calls to dev_mc_add and dev_mc_del outside of spinlocks. > > Fixes: 6090a0c4c7c6 ("RDMA/rxe: Cleanup rxe_mcast.c") > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> > --- > v2 > Addressed comments from Jason re not placing calls to dev_mc_add/del > inside of spinlocks. I split this into two patches and put it into for-rc Please check, and try to write commit messages in this style - "Fix 'some other commit'" is not a good subject, state what bug the patch is correcting. One patch per bug Include the oops messages in the commit messages. Jason
On Thu, May 5, 2022 at 4:28 AM Bob Pearson <rpearsonhpe@gmail.com> wrote: > > rxe_mcast.c currently uses _irqsave spinlocks for rxe->mcg_lock > while rxe_recv.c uses _bh spinlocks for the same lock. > > Additionally the current code issues a warning that _irqrestore > is restoring hardware interrupts while some interrupts are > enabled. This is traced to calls to the calls to dev_mc_add/del(). Hi, Bob As Jason commented, can you show us the warning? And how to reproduce this problem? Thanks, Zhu Yanjun > > Change the locking of rxe->mcg_lock in rxe_mcast.c to use > spin_(un)lock_bh() which matches that in rxe_recv.c. Also move > the calls to dev_mc_add and dev_mc_del outside of spinlocks. > > Fixes: 6090a0c4c7c6 ("RDMA/rxe: Cleanup rxe_mcast.c") > Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> > --- > v2 > Addressed comments from Jason re not placing calls to dev_mc_add/del > inside of spinlocks. > > drivers/infiniband/sw/rxe/rxe_mcast.c | 81 ++++++++++++--------------- > 1 file changed, 35 insertions(+), 46 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_mcast.c b/drivers/infiniband/sw/rxe/rxe_mcast.c > index ae8f11cb704a..873a9b10307c 100644 > --- a/drivers/infiniband/sw/rxe/rxe_mcast.c > +++ b/drivers/infiniband/sw/rxe/rxe_mcast.c > @@ -38,13 +38,13 @@ static int rxe_mcast_add(struct rxe_dev *rxe, union ib_gid *mgid) > } > > /** > - * rxe_mcast_delete - delete multicast address from rxe device > + * rxe_mcast_del - delete multicast address from rxe device > * @rxe: rxe device object > * @mgid: multicast address as a gid > * > * Returns 0 on success else an error > */ > -static int rxe_mcast_delete(struct rxe_dev *rxe, union ib_gid *mgid) > +static int rxe_mcast_del(struct rxe_dev *rxe, union ib_gid *mgid) > { > unsigned char ll_addr[ETH_ALEN]; > > @@ -143,11 +143,10 @@ static struct rxe_mcg *__rxe_lookup_mcg(struct rxe_dev *rxe, > struct rxe_mcg *rxe_lookup_mcg(struct rxe_dev *rxe, union ib_gid *mgid) > { > struct rxe_mcg *mcg; > - unsigned long flags; > > - spin_lock_irqsave(&rxe->mcg_lock, flags); > + spin_lock_bh(&rxe->mcg_lock); > mcg = __rxe_lookup_mcg(rxe, mgid); > - spin_unlock_irqrestore(&rxe->mcg_lock, flags); > + spin_unlock_bh(&rxe->mcg_lock); > > return mcg; > } > @@ -159,17 +158,10 @@ struct rxe_mcg *rxe_lookup_mcg(struct rxe_dev *rxe, union ib_gid *mgid) > * @mcg: new mcg object > * > * Context: caller should hold rxe->mcg lock > - * Returns: 0 on success else an error > */ > -static int __rxe_init_mcg(struct rxe_dev *rxe, union ib_gid *mgid, > - struct rxe_mcg *mcg) > +static void __rxe_init_mcg(struct rxe_dev *rxe, union ib_gid *mgid, > + struct rxe_mcg *mcg) > { > - int err; > - > - err = rxe_mcast_add(rxe, mgid); > - if (unlikely(err)) > - return err; > - > kref_init(&mcg->ref_cnt); > memcpy(&mcg->mgid, mgid, sizeof(mcg->mgid)); > INIT_LIST_HEAD(&mcg->qp_list); > @@ -184,8 +176,6 @@ static int __rxe_init_mcg(struct rxe_dev *rxe, union ib_gid *mgid, > */ > kref_get(&mcg->ref_cnt); > __rxe_insert_mcg(mcg); > - > - return 0; > } > > /** > @@ -198,7 +188,6 @@ static int __rxe_init_mcg(struct rxe_dev *rxe, union ib_gid *mgid, > static struct rxe_mcg *rxe_get_mcg(struct rxe_dev *rxe, union ib_gid *mgid) > { > struct rxe_mcg *mcg, *tmp; > - unsigned long flags; > int err; > > if (rxe->attr.max_mcast_grp == 0) > @@ -209,36 +198,38 @@ static struct rxe_mcg *rxe_get_mcg(struct rxe_dev *rxe, union ib_gid *mgid) > if (mcg) > return mcg; > > + /* check to see if we have reached limit */ > + if (atomic_inc_return(&rxe->mcg_num) > rxe->attr.max_mcast_grp) { > + err = -ENOMEM; > + goto err_dec; > + } > + > /* speculative alloc of new mcg */ > mcg = kzalloc(sizeof(*mcg), GFP_KERNEL); > if (!mcg) > return ERR_PTR(-ENOMEM); > > - spin_lock_irqsave(&rxe->mcg_lock, flags); > + spin_lock_bh(&rxe->mcg_lock); > /* re-check to see if someone else just added it */ > tmp = __rxe_lookup_mcg(rxe, mgid); > if (tmp) { > + spin_unlock_bh(&rxe->mcg_lock); > + atomic_dec(&rxe->mcg_num); > kfree(mcg); > - mcg = tmp; > - goto out; > + return tmp; > } > > - if (atomic_inc_return(&rxe->mcg_num) > rxe->attr.max_mcast_grp) { > - err = -ENOMEM; > - goto err_dec; > - } > + __rxe_init_mcg(rxe, mgid, mcg); > + spin_unlock_bh(&rxe->mcg_lock); > > - err = __rxe_init_mcg(rxe, mgid, mcg); > - if (err) > - goto err_dec; > -out: > - spin_unlock_irqrestore(&rxe->mcg_lock, flags); > - return mcg; > + /* add mcast address outside of lock */ > + err = rxe_mcast_add(rxe, mgid); > + if (!err) > + return mcg; > > + kfree(mcg); > err_dec: > atomic_dec(&rxe->mcg_num); > - spin_unlock_irqrestore(&rxe->mcg_lock, flags); > - kfree(mcg); > return ERR_PTR(err); > } > > @@ -268,7 +259,6 @@ static void __rxe_destroy_mcg(struct rxe_mcg *mcg) > __rxe_remove_mcg(mcg); > kref_put(&mcg->ref_cnt, rxe_cleanup_mcg); > > - rxe_mcast_delete(mcg->rxe, &mcg->mgid); > atomic_dec(&rxe->mcg_num); > } > > @@ -280,11 +270,12 @@ static void __rxe_destroy_mcg(struct rxe_mcg *mcg) > */ > static void rxe_destroy_mcg(struct rxe_mcg *mcg) > { > - unsigned long flags; > + /* delete mcast address outside of lock */ > + rxe_mcast_del(mcg->rxe, &mcg->mgid); > > - spin_lock_irqsave(&mcg->rxe->mcg_lock, flags); > + spin_lock_bh(&mcg->rxe->mcg_lock); > __rxe_destroy_mcg(mcg); > - spin_unlock_irqrestore(&mcg->rxe->mcg_lock, flags); > + spin_unlock_bh(&mcg->rxe->mcg_lock); > } > > /** > @@ -339,25 +330,24 @@ static int rxe_attach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp) > { > struct rxe_dev *rxe = mcg->rxe; > struct rxe_mca *mca, *tmp; > - unsigned long flags; > int err; > > /* check to see if the qp is already a member of the group */ > - spin_lock_irqsave(&rxe->mcg_lock, flags); > + spin_lock_bh(&rxe->mcg_lock); > list_for_each_entry(mca, &mcg->qp_list, qp_list) { > if (mca->qp == qp) { > - spin_unlock_irqrestore(&rxe->mcg_lock, flags); > + spin_unlock_bh(&rxe->mcg_lock); > return 0; > } > } > - spin_unlock_irqrestore(&rxe->mcg_lock, flags); > + spin_unlock_bh(&rxe->mcg_lock); > > /* speculative alloc new mca without using GFP_ATOMIC */ > mca = kzalloc(sizeof(*mca), GFP_KERNEL); > if (!mca) > return -ENOMEM; > > - spin_lock_irqsave(&rxe->mcg_lock, flags); > + spin_lock_bh(&rxe->mcg_lock); > /* re-check to see if someone else just attached qp */ > list_for_each_entry(tmp, &mcg->qp_list, qp_list) { > if (tmp->qp == qp) { > @@ -371,7 +361,7 @@ static int rxe_attach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp) > if (err) > kfree(mca); > out: > - spin_unlock_irqrestore(&rxe->mcg_lock, flags); > + spin_unlock_bh(&rxe->mcg_lock); > return err; > } > > @@ -405,9 +395,8 @@ static int rxe_detach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp) > { > struct rxe_dev *rxe = mcg->rxe; > struct rxe_mca *mca, *tmp; > - unsigned long flags; > > - spin_lock_irqsave(&rxe->mcg_lock, flags); > + spin_lock_bh(&rxe->mcg_lock); > list_for_each_entry_safe(mca, tmp, &mcg->qp_list, qp_list) { > if (mca->qp == qp) { > __rxe_cleanup_mca(mca, mcg); > @@ -421,13 +410,13 @@ static int rxe_detach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp) > if (atomic_read(&mcg->qp_num) <= 0) > __rxe_destroy_mcg(mcg); > > - spin_unlock_irqrestore(&rxe->mcg_lock, flags); > + spin_unlock_bh(&rxe->mcg_lock); > return 0; > } > } > > /* we didn't find the qp on the list */ > - spin_unlock_irqrestore(&rxe->mcg_lock, flags); > + spin_unlock_bh(&rxe->mcg_lock); > return -EINVAL; > } > > -- > 2.34.1 >
diff --git a/drivers/infiniband/sw/rxe/rxe_mcast.c b/drivers/infiniband/sw/rxe/rxe_mcast.c index ae8f11cb704a..873a9b10307c 100644 --- a/drivers/infiniband/sw/rxe/rxe_mcast.c +++ b/drivers/infiniband/sw/rxe/rxe_mcast.c @@ -38,13 +38,13 @@ static int rxe_mcast_add(struct rxe_dev *rxe, union ib_gid *mgid) } /** - * rxe_mcast_delete - delete multicast address from rxe device + * rxe_mcast_del - delete multicast address from rxe device * @rxe: rxe device object * @mgid: multicast address as a gid * * Returns 0 on success else an error */ -static int rxe_mcast_delete(struct rxe_dev *rxe, union ib_gid *mgid) +static int rxe_mcast_del(struct rxe_dev *rxe, union ib_gid *mgid) { unsigned char ll_addr[ETH_ALEN]; @@ -143,11 +143,10 @@ static struct rxe_mcg *__rxe_lookup_mcg(struct rxe_dev *rxe, struct rxe_mcg *rxe_lookup_mcg(struct rxe_dev *rxe, union ib_gid *mgid) { struct rxe_mcg *mcg; - unsigned long flags; - spin_lock_irqsave(&rxe->mcg_lock, flags); + spin_lock_bh(&rxe->mcg_lock); mcg = __rxe_lookup_mcg(rxe, mgid); - spin_unlock_irqrestore(&rxe->mcg_lock, flags); + spin_unlock_bh(&rxe->mcg_lock); return mcg; } @@ -159,17 +158,10 @@ struct rxe_mcg *rxe_lookup_mcg(struct rxe_dev *rxe, union ib_gid *mgid) * @mcg: new mcg object * * Context: caller should hold rxe->mcg lock - * Returns: 0 on success else an error */ -static int __rxe_init_mcg(struct rxe_dev *rxe, union ib_gid *mgid, - struct rxe_mcg *mcg) +static void __rxe_init_mcg(struct rxe_dev *rxe, union ib_gid *mgid, + struct rxe_mcg *mcg) { - int err; - - err = rxe_mcast_add(rxe, mgid); - if (unlikely(err)) - return err; - kref_init(&mcg->ref_cnt); memcpy(&mcg->mgid, mgid, sizeof(mcg->mgid)); INIT_LIST_HEAD(&mcg->qp_list); @@ -184,8 +176,6 @@ static int __rxe_init_mcg(struct rxe_dev *rxe, union ib_gid *mgid, */ kref_get(&mcg->ref_cnt); __rxe_insert_mcg(mcg); - - return 0; } /** @@ -198,7 +188,6 @@ static int __rxe_init_mcg(struct rxe_dev *rxe, union ib_gid *mgid, static struct rxe_mcg *rxe_get_mcg(struct rxe_dev *rxe, union ib_gid *mgid) { struct rxe_mcg *mcg, *tmp; - unsigned long flags; int err; if (rxe->attr.max_mcast_grp == 0) @@ -209,36 +198,38 @@ static struct rxe_mcg *rxe_get_mcg(struct rxe_dev *rxe, union ib_gid *mgid) if (mcg) return mcg; + /* check to see if we have reached limit */ + if (atomic_inc_return(&rxe->mcg_num) > rxe->attr.max_mcast_grp) { + err = -ENOMEM; + goto err_dec; + } + /* speculative alloc of new mcg */ mcg = kzalloc(sizeof(*mcg), GFP_KERNEL); if (!mcg) return ERR_PTR(-ENOMEM); - spin_lock_irqsave(&rxe->mcg_lock, flags); + spin_lock_bh(&rxe->mcg_lock); /* re-check to see if someone else just added it */ tmp = __rxe_lookup_mcg(rxe, mgid); if (tmp) { + spin_unlock_bh(&rxe->mcg_lock); + atomic_dec(&rxe->mcg_num); kfree(mcg); - mcg = tmp; - goto out; + return tmp; } - if (atomic_inc_return(&rxe->mcg_num) > rxe->attr.max_mcast_grp) { - err = -ENOMEM; - goto err_dec; - } + __rxe_init_mcg(rxe, mgid, mcg); + spin_unlock_bh(&rxe->mcg_lock); - err = __rxe_init_mcg(rxe, mgid, mcg); - if (err) - goto err_dec; -out: - spin_unlock_irqrestore(&rxe->mcg_lock, flags); - return mcg; + /* add mcast address outside of lock */ + err = rxe_mcast_add(rxe, mgid); + if (!err) + return mcg; + kfree(mcg); err_dec: atomic_dec(&rxe->mcg_num); - spin_unlock_irqrestore(&rxe->mcg_lock, flags); - kfree(mcg); return ERR_PTR(err); } @@ -268,7 +259,6 @@ static void __rxe_destroy_mcg(struct rxe_mcg *mcg) __rxe_remove_mcg(mcg); kref_put(&mcg->ref_cnt, rxe_cleanup_mcg); - rxe_mcast_delete(mcg->rxe, &mcg->mgid); atomic_dec(&rxe->mcg_num); } @@ -280,11 +270,12 @@ static void __rxe_destroy_mcg(struct rxe_mcg *mcg) */ static void rxe_destroy_mcg(struct rxe_mcg *mcg) { - unsigned long flags; + /* delete mcast address outside of lock */ + rxe_mcast_del(mcg->rxe, &mcg->mgid); - spin_lock_irqsave(&mcg->rxe->mcg_lock, flags); + spin_lock_bh(&mcg->rxe->mcg_lock); __rxe_destroy_mcg(mcg); - spin_unlock_irqrestore(&mcg->rxe->mcg_lock, flags); + spin_unlock_bh(&mcg->rxe->mcg_lock); } /** @@ -339,25 +330,24 @@ static int rxe_attach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp) { struct rxe_dev *rxe = mcg->rxe; struct rxe_mca *mca, *tmp; - unsigned long flags; int err; /* check to see if the qp is already a member of the group */ - spin_lock_irqsave(&rxe->mcg_lock, flags); + spin_lock_bh(&rxe->mcg_lock); list_for_each_entry(mca, &mcg->qp_list, qp_list) { if (mca->qp == qp) { - spin_unlock_irqrestore(&rxe->mcg_lock, flags); + spin_unlock_bh(&rxe->mcg_lock); return 0; } } - spin_unlock_irqrestore(&rxe->mcg_lock, flags); + spin_unlock_bh(&rxe->mcg_lock); /* speculative alloc new mca without using GFP_ATOMIC */ mca = kzalloc(sizeof(*mca), GFP_KERNEL); if (!mca) return -ENOMEM; - spin_lock_irqsave(&rxe->mcg_lock, flags); + spin_lock_bh(&rxe->mcg_lock); /* re-check to see if someone else just attached qp */ list_for_each_entry(tmp, &mcg->qp_list, qp_list) { if (tmp->qp == qp) { @@ -371,7 +361,7 @@ static int rxe_attach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp) if (err) kfree(mca); out: - spin_unlock_irqrestore(&rxe->mcg_lock, flags); + spin_unlock_bh(&rxe->mcg_lock); return err; } @@ -405,9 +395,8 @@ static int rxe_detach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp) { struct rxe_dev *rxe = mcg->rxe; struct rxe_mca *mca, *tmp; - unsigned long flags; - spin_lock_irqsave(&rxe->mcg_lock, flags); + spin_lock_bh(&rxe->mcg_lock); list_for_each_entry_safe(mca, tmp, &mcg->qp_list, qp_list) { if (mca->qp == qp) { __rxe_cleanup_mca(mca, mcg); @@ -421,13 +410,13 @@ static int rxe_detach_mcg(struct rxe_mcg *mcg, struct rxe_qp *qp) if (atomic_read(&mcg->qp_num) <= 0) __rxe_destroy_mcg(mcg); - spin_unlock_irqrestore(&rxe->mcg_lock, flags); + spin_unlock_bh(&rxe->mcg_lock); return 0; } } /* we didn't find the qp on the list */ - spin_unlock_irqrestore(&rxe->mcg_lock, flags); + spin_unlock_bh(&rxe->mcg_lock); return -EINVAL; }
rxe_mcast.c currently uses _irqsave spinlocks for rxe->mcg_lock while rxe_recv.c uses _bh spinlocks for the same lock. Additionally the current code issues a warning that _irqrestore is restoring hardware interrupts while some interrupts are enabled. This is traced to calls to the calls to dev_mc_add/del(). Change the locking of rxe->mcg_lock in rxe_mcast.c to use spin_(un)lock_bh() which matches that in rxe_recv.c. Also move the calls to dev_mc_add and dev_mc_del outside of spinlocks. Fixes: 6090a0c4c7c6 ("RDMA/rxe: Cleanup rxe_mcast.c") Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> --- v2 Addressed comments from Jason re not placing calls to dev_mc_add/del inside of spinlocks. drivers/infiniband/sw/rxe/rxe_mcast.c | 81 ++++++++++++--------------- 1 file changed, 35 insertions(+), 46 deletions(-)