| Message ID | 2a0e295786c127e518ebee8bb7cafcb819a625f6.1631520231.git.leonro@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [rdma-rc] RDMA/usnic: Lock VF with mutex instead of spinlock | expand |
> 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 >
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
> 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
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
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
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)