linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock
@ 2021-09-13  8:04 Leon Romanovsky
  2021-09-13  8:17 ` Haakon Bugge
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Leon Romanovsky @ 2021-09-13  8:04 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe
  Cc: Leon Romanovsky, Christian Benvenuti, Dan Carpenter, linux-rdma,
	Nelson Escobar

From: Leon Romanovsky <leonro@nvidia.com>

Usnic VF doesn't need lock in atomic context to create QPs, so it is safe
to use mutex instead of spinlock. Such change fixes the following smatch
error.

Smatch static checker warning:

   lib/kobject.c:289 kobject_set_name_vargs()
    warn: sleeping in atomic context

Fixes: 514aee660df4 ("RDMA: Globally allocate and release QP memory")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/usnic/usnic_ib.h       |  2 +-
 drivers/infiniband/hw/usnic/usnic_ib_main.c  |  2 +-
 drivers/infiniband/hw/usnic/usnic_ib_verbs.c | 16 ++++++++--------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/usnic/usnic_ib.h b/drivers/infiniband/hw/usnic/usnic_ib.h
index 84dd682d2334..b350081aeb5a 100644
--- a/drivers/infiniband/hw/usnic/usnic_ib.h
+++ b/drivers/infiniband/hw/usnic/usnic_ib.h
@@ -90,7 +90,7 @@ struct usnic_ib_dev {
 
 struct usnic_ib_vf {
 	struct usnic_ib_dev		*pf;
-	spinlock_t			lock;
+	struct mutex			lock;
 	struct usnic_vnic		*vnic;
 	unsigned int			qp_grp_ref_cnt;
 	struct usnic_ib_pd		*pd;
diff --git a/drivers/infiniband/hw/usnic/usnic_ib_main.c b/drivers/infiniband/hw/usnic/usnic_ib_main.c
index 228e9a36dad0..d346dd48e731 100644
--- a/drivers/infiniband/hw/usnic/usnic_ib_main.c
+++ b/drivers/infiniband/hw/usnic/usnic_ib_main.c
@@ -572,7 +572,7 @@ static int usnic_ib_pci_probe(struct pci_dev *pdev,
 	}
 
 	vf->pf = pf;
-	spin_lock_init(&vf->lock);
+	mutex_init(&vf->lock);
 	mutex_lock(&pf->usdev_lock);
 	list_add_tail(&vf->link, &pf->vf_dev_list);
 	/*
diff --git a/drivers/infiniband/hw/usnic/usnic_ib_verbs.c b/drivers/infiniband/hw/usnic/usnic_ib_verbs.c
index 06a4e9d4545d..756a83bcff58 100644
--- a/drivers/infiniband/hw/usnic/usnic_ib_verbs.c
+++ b/drivers/infiniband/hw/usnic/usnic_ib_verbs.c
@@ -196,7 +196,7 @@ find_free_vf_and_create_qp_grp(struct ib_qp *qp,
 		for (i = 0; dev_list[i]; i++) {
 			dev = dev_list[i];
 			vf = dev_get_drvdata(dev);
-			spin_lock(&vf->lock);
+			mutex_lock(&vf->lock);
 			vnic = vf->vnic;
 			if (!usnic_vnic_check_room(vnic, res_spec)) {
 				usnic_dbg("Found used vnic %s from %s\n",
@@ -208,10 +208,10 @@ find_free_vf_and_create_qp_grp(struct ib_qp *qp,
 							     vf, pd, res_spec,
 							     trans_spec);
 
-				spin_unlock(&vf->lock);
+				mutex_unlock(&vf->lock);
 				goto qp_grp_check;
 			}
-			spin_unlock(&vf->lock);
+			mutex_unlock(&vf->lock);
 
 		}
 		usnic_uiom_free_dev_list(dev_list);
@@ -220,7 +220,7 @@ find_free_vf_and_create_qp_grp(struct ib_qp *qp,
 
 	/* Try to find resources on an unused vf */
 	list_for_each_entry(vf, &us_ibdev->vf_dev_list, link) {
-		spin_lock(&vf->lock);
+		mutex_lock(&vf->lock);
 		vnic = vf->vnic;
 		if (vf->qp_grp_ref_cnt == 0 &&
 		    usnic_vnic_check_room(vnic, res_spec) == 0) {
@@ -228,10 +228,10 @@ find_free_vf_and_create_qp_grp(struct ib_qp *qp,
 						     vf, pd, res_spec,
 						     trans_spec);
 
-			spin_unlock(&vf->lock);
+			mutex_unlock(&vf->lock);
 			goto qp_grp_check;
 		}
-		spin_unlock(&vf->lock);
+		mutex_unlock(&vf->lock);
 	}
 
 	usnic_info("No free qp grp found on %s\n",
@@ -253,9 +253,9 @@ static void qp_grp_destroy(struct usnic_ib_qp_grp *qp_grp)
 
 	WARN_ON(qp_grp->state != IB_QPS_RESET);
 
-	spin_lock(&vf->lock);
+	mutex_lock(&vf->lock);
 	usnic_ib_qp_grp_destroy(qp_grp);
-	spin_unlock(&vf->lock);
+	mutex_unlock(&vf->lock);
 }
 
 static int create_qp_validate_user_data(struct usnic_ib_create_qp_cmd cmd)
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock
  2021-09-13  8:04 [PATCH rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock Leon Romanovsky
@ 2021-09-13  8:17 ` Haakon Bugge
  2021-09-13  8:27   ` Leon Romanovsky
  2021-09-23  5:34 ` Leon Romanovsky
  2021-09-24 13:59 ` Jason Gunthorpe
  2 siblings, 1 reply; 6+ messages in thread
From: Haakon Bugge @ 2021-09-13  8:17 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Doug Ledford, Jason Gunthorpe, Leon Romanovsky,
	Christian Benvenuti, Dan Carpenter, OFED mailing list,
	Nelson Escobar



> On 13 Sep 2021, at 10:04, Leon Romanovsky <leon@kernel.org> wrote:
> 
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Usnic VF doesn't need lock in atomic context to create QPs, so it is safe
> to use mutex instead of spinlock. Such change fixes the following smatch
> error.

s/GFP_ATOMIC/GFP_KERNEL/ in find_free_vf_and_create_qp_grp() as well?


Thxs, Håkon
> 
> Smatch static checker warning:
> 
>   lib/kobject.c:289 kobject_set_name_vargs()
>    warn: sleeping in atomic context
> 
> Fixes: 514aee660df4 ("RDMA: Globally allocate and release QP memory")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
> drivers/infiniband/hw/usnic/usnic_ib.h       |  2 +-
> drivers/infiniband/hw/usnic/usnic_ib_main.c  |  2 +-
> drivers/infiniband/hw/usnic/usnic_ib_verbs.c | 16 ++++++++--------
> 3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/usnic/usnic_ib.h b/drivers/infiniband/hw/usnic/usnic_ib.h
> index 84dd682d2334..b350081aeb5a 100644
> --- a/drivers/infiniband/hw/usnic/usnic_ib.h
> +++ b/drivers/infiniband/hw/usnic/usnic_ib.h
> @@ -90,7 +90,7 @@ struct usnic_ib_dev {
> 
> struct usnic_ib_vf {
> 	struct usnic_ib_dev		*pf;
> -	spinlock_t			lock;
> +	struct mutex			lock;
> 	struct usnic_vnic		*vnic;
> 	unsigned int			qp_grp_ref_cnt;
> 	struct usnic_ib_pd		*pd;
> diff --git a/drivers/infiniband/hw/usnic/usnic_ib_main.c b/drivers/infiniband/hw/usnic/usnic_ib_main.c
> index 228e9a36dad0..d346dd48e731 100644
> --- a/drivers/infiniband/hw/usnic/usnic_ib_main.c
> +++ b/drivers/infiniband/hw/usnic/usnic_ib_main.c
> @@ -572,7 +572,7 @@ static int usnic_ib_pci_probe(struct pci_dev *pdev,
> 	}
> 
> 	vf->pf = pf;
> -	spin_lock_init(&vf->lock);
> +	mutex_init(&vf->lock);
> 	mutex_lock(&pf->usdev_lock);
> 	list_add_tail(&vf->link, &pf->vf_dev_list);
> 	/*
> diff --git a/drivers/infiniband/hw/usnic/usnic_ib_verbs.c b/drivers/infiniband/hw/usnic/usnic_ib_verbs.c
> index 06a4e9d4545d..756a83bcff58 100644
> --- a/drivers/infiniband/hw/usnic/usnic_ib_verbs.c
> +++ b/drivers/infiniband/hw/usnic/usnic_ib_verbs.c
> @@ -196,7 +196,7 @@ find_free_vf_and_create_qp_grp(struct ib_qp *qp,
> 		for (i = 0; dev_list[i]; i++) {
> 			dev = dev_list[i];
> 			vf = dev_get_drvdata(dev);
> -			spin_lock(&vf->lock);
> +			mutex_lock(&vf->lock);
> 			vnic = vf->vnic;
> 			if (!usnic_vnic_check_room(vnic, res_spec)) {
> 				usnic_dbg("Found used vnic %s from %s\n",
> @@ -208,10 +208,10 @@ find_free_vf_and_create_qp_grp(struct ib_qp *qp,
> 							     vf, pd, res_spec,
> 							     trans_spec);
> 
> -				spin_unlock(&vf->lock);
> +				mutex_unlock(&vf->lock);
> 				goto qp_grp_check;
> 			}
> -			spin_unlock(&vf->lock);
> +			mutex_unlock(&vf->lock);
> 
> 		}
> 		usnic_uiom_free_dev_list(dev_list);
> @@ -220,7 +220,7 @@ find_free_vf_and_create_qp_grp(struct ib_qp *qp,
> 
> 	/* Try to find resources on an unused vf */
> 	list_for_each_entry(vf, &us_ibdev->vf_dev_list, link) {
> -		spin_lock(&vf->lock);
> +		mutex_lock(&vf->lock);
> 		vnic = vf->vnic;
> 		if (vf->qp_grp_ref_cnt == 0 &&
> 		    usnic_vnic_check_room(vnic, res_spec) == 0) {
> @@ -228,10 +228,10 @@ find_free_vf_and_create_qp_grp(struct ib_qp *qp,
> 						     vf, pd, res_spec,
> 						     trans_spec);
> 
> -			spin_unlock(&vf->lock);
> +			mutex_unlock(&vf->lock);
> 			goto qp_grp_check;
> 		}
> -		spin_unlock(&vf->lock);
> +		mutex_unlock(&vf->lock);
> 	}
> 
> 	usnic_info("No free qp grp found on %s\n",
> @@ -253,9 +253,9 @@ static void qp_grp_destroy(struct usnic_ib_qp_grp *qp_grp)
> 
> 	WARN_ON(qp_grp->state != IB_QPS_RESET);
> 
> -	spin_lock(&vf->lock);
> +	mutex_lock(&vf->lock);
> 	usnic_ib_qp_grp_destroy(qp_grp);
> -	spin_unlock(&vf->lock);
> +	mutex_unlock(&vf->lock);
> }
> 
> static int create_qp_validate_user_data(struct usnic_ib_create_qp_cmd cmd)
> -- 
> 2.31.1
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock
  2021-09-13  8:17 ` Haakon Bugge
@ 2021-09-13  8:27   ` Leon Romanovsky
  2021-09-13 12:50     ` Haakon Bugge
  0 siblings, 1 reply; 6+ messages in thread
From: Leon Romanovsky @ 2021-09-13  8:27 UTC (permalink / raw)
  To: Haakon Bugge
  Cc: Doug Ledford, Jason Gunthorpe, Christian Benvenuti,
	Dan Carpenter, OFED mailing list, Nelson Escobar

On Mon, Sep 13, 2021 at 08:17:00AM +0000, Haakon Bugge wrote:
> 
> 
> > On 13 Sep 2021, at 10:04, Leon Romanovsky <leon@kernel.org> wrote:
> > 
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Usnic VF doesn't need lock in atomic context to create QPs, so it is safe
> > to use mutex instead of spinlock. Such change fixes the following smatch
> > error.
> 
> s/GFP_ATOMIC/GFP_KERNEL/ in find_free_vf_and_create_qp_grp() as well?

Do you mean in usnic_uiom_get_dev_list()?
That GFP_ATOMIC existed before my patch while we are holding usdev_lock mutex.

Anyway, I prefer to touch that driver as less as possible.

The allocations can continue to be with GFP_ATOMIC while we use mutex.
It is bad thing, but not a necessary to fix bug. We just wasting atomic
memory and instruct kernel do not sleep while
doing allocations.

Thanks

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock
  2021-09-13  8:27   ` Leon Romanovsky
@ 2021-09-13 12:50     ` Haakon Bugge
  0 siblings, 0 replies; 6+ messages in thread
From: Haakon Bugge @ 2021-09-13 12:50 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Doug Ledford, Jason Gunthorpe, Christian Benvenuti,
	Dan Carpenter, OFED mailing list, Nelson Escobar



> On 13 Sep 2021, at 10:27, Leon Romanovsky <leon@kernel.org> wrote:
> 
> On Mon, Sep 13, 2021 at 08:17:00AM +0000, Haakon Bugge wrote:
>> 
>> 
>>> On 13 Sep 2021, at 10:04, Leon Romanovsky <leon@kernel.org> wrote:
>>> 
>>> From: Leon Romanovsky <leonro@nvidia.com>
>>> 
>>> Usnic VF doesn't need lock in atomic context to create QPs, so it is safe
>>> to use mutex instead of spinlock. Such change fixes the following smatch
>>> error.
>> 
>> s/GFP_ATOMIC/GFP_KERNEL/ in find_free_vf_and_create_qp_grp() as well?
> 
> Do you mean in usnic_uiom_get_dev_list()?

Sorry, my comments was v5.14.1 centric. For latest upstream, its alloc_res_chunk_list(), 

find_free_vf_and_create_qp_grp() 
   -> usnic_ib_qp_grp_create
      -> alloc_res_chunk_list()



> That GFP_ATOMIC existed before my patch while we are holding usdev_lock mutex.

True.

> Anyway, I prefer to touch that driver as less as possible.
> 
> The allocations can continue to be with GFP_ATOMIC while we use mutex.
> It is bad thing, but not a necessary to fix bug. We just wasting atomic
> memory and instruct kernel do not sleep while
> doing allocations.

I assume this driver is not very much maintained by Cisco, hence, I agree with your rationale above,

Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>


Thxs, Håkon


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock
  2021-09-13  8:04 [PATCH rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock Leon Romanovsky
  2021-09-13  8:17 ` Haakon Bugge
@ 2021-09-23  5:34 ` Leon Romanovsky
  2021-09-24 13:59 ` Jason Gunthorpe
  2 siblings, 0 replies; 6+ messages in thread
From: Leon Romanovsky @ 2021-09-23  5:34 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe
  Cc: Christian Benvenuti, Dan Carpenter, linux-rdma, Nelson Escobar

On Mon, Sep 13, 2021 at 11:04:42AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Usnic VF doesn't need lock in atomic context to create QPs, so it is safe
> to use mutex instead of spinlock. Such change fixes the following smatch
> error.
> 
> Smatch static checker warning:
> 
>    lib/kobject.c:289 kobject_set_name_vargs()
>     warn: sleeping in atomic context
> 
> Fixes: 514aee660df4 ("RDMA: Globally allocate and release QP memory")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/hw/usnic/usnic_ib.h       |  2 +-
>  drivers/infiniband/hw/usnic/usnic_ib_main.c  |  2 +-
>  drivers/infiniband/hw/usnic/usnic_ib_verbs.c | 16 ++++++++--------
>  3 files changed, 10 insertions(+), 10 deletions(-)

Jason?

Thanks

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock
  2021-09-13  8:04 [PATCH rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock Leon Romanovsky
  2021-09-13  8:17 ` Haakon Bugge
  2021-09-23  5:34 ` Leon Romanovsky
@ 2021-09-24 13:59 ` Jason Gunthorpe
  2 siblings, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2021-09-24 13:59 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Doug Ledford, Leon Romanovsky, Christian Benvenuti,
	Dan Carpenter, linux-rdma, Nelson Escobar

On Mon, Sep 13, 2021 at 11:04:42AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Usnic VF doesn't need lock in atomic context to create QPs, so it is safe
> to use mutex instead of spinlock. Such change fixes the following smatch
> error.
> 
> Smatch static checker warning:
> 
>    lib/kobject.c:289 kobject_set_name_vargs()
>     warn: sleeping in atomic context
> 
> Fixes: 514aee660df4 ("RDMA: Globally allocate and release QP memory")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> Reviewed-by: Håkon Bugge <haakon.bugge@oracle.com>
> ---
>  drivers/infiniband/hw/usnic/usnic_ib.h       |  2 +-
>  drivers/infiniband/hw/usnic/usnic_ib_main.c  |  2 +-
>  drivers/infiniband/hw/usnic/usnic_ib_verbs.c | 16 ++++++++--------
>  3 files changed, 10 insertions(+), 10 deletions(-)

Applied to for-rc

Thanks,
Jason

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-24 13:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-13  8:04 [PATCH rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock Leon Romanovsky
2021-09-13  8:17 ` Haakon Bugge
2021-09-13  8:27   ` Leon Romanovsky
2021-09-13 12:50     ` Haakon Bugge
2021-09-23  5:34 ` Leon Romanovsky
2021-09-24 13:59 ` Jason Gunthorpe

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).