* [PATCH for-next v2] RDMA/rxe: Fix "RDMA/rxe: Cleanup rxe_mcast.c"
@ 2022-05-04 20:28 Bob Pearson
2022-05-05 0:32 ` Jason Gunthorpe
2022-05-05 1:44 ` Zhu Yanjun
0 siblings, 2 replies; 3+ messages in thread
From: Bob Pearson @ 2022-05-04 20:28 UTC (permalink / raw)
To: jgg, zyjzyj2000, linux-rdma; +Cc: Bob Pearson
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH for-next v2] RDMA/rxe: Fix "RDMA/rxe: Cleanup rxe_mcast.c"
2022-05-04 20:28 [PATCH for-next v2] RDMA/rxe: Fix "RDMA/rxe: Cleanup rxe_mcast.c" Bob Pearson
@ 2022-05-05 0:32 ` Jason Gunthorpe
2022-05-05 1:44 ` Zhu Yanjun
1 sibling, 0 replies; 3+ messages in thread
From: Jason Gunthorpe @ 2022-05-05 0:32 UTC (permalink / raw)
To: Bob Pearson; +Cc: zyjzyj2000, linux-rdma
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH for-next v2] RDMA/rxe: Fix "RDMA/rxe: Cleanup rxe_mcast.c"
2022-05-04 20:28 [PATCH for-next v2] RDMA/rxe: Fix "RDMA/rxe: Cleanup rxe_mcast.c" Bob Pearson
2022-05-05 0:32 ` Jason Gunthorpe
@ 2022-05-05 1:44 ` Zhu Yanjun
1 sibling, 0 replies; 3+ messages in thread
From: Zhu Yanjun @ 2022-05-05 1:44 UTC (permalink / raw)
To: Bob Pearson; +Cc: Jason Gunthorpe, RDMA mailing list
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-05 1:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-04 20:28 [PATCH for-next v2] RDMA/rxe: Fix "RDMA/rxe: Cleanup rxe_mcast.c" Bob Pearson
2022-05-05 0:32 ` Jason Gunthorpe
2022-05-05 1:44 ` Zhu Yanjun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).