diff mbox series

[for-next,v2] RDMA/rxe: Fix "RDMA/rxe: Cleanup rxe_mcast.c"

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

Commit Message

Bob Pearson May 4, 2022, 8:28 p.m. UTC
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(-)

Comments

Jason Gunthorpe May 5, 2022, 12:32 a.m. UTC | #1
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
Zhu Yanjun May 5, 2022, 1:44 a.m. UTC | #2
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 mbox series

Patch

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;
 }