| Message ID | 20181031124242.24239-2-shamir.rabinovitch@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | figure uverbs/kernel ib_pd w/o using ib_pd uobject | expand |
On Wed, Oct 31, 2018 at 02:42:38PM +0200, Shamir Rabinovitch wrote: > Bypassing ib_core leave the restrack and other fields uninitialized. > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > drivers/infiniband/hw/mlx5/main.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c > index be701d4..97b44e9 100644 > +++ b/drivers/infiniband/hw/mlx5/main.c > @@ -4565,26 +4565,19 @@ static int create_dev_resources(struct mlx5_ib_resources *devr) > > mutex_init(&devr->mutex); > > - devr->p0 = mlx5_ib_alloc_pd(&dev->ib_dev, NULL, NULL); > + /* must call ib_core here to initialize restrack! */ > + devr->p0 = ib_alloc_pd(&dev->ib_dev, 0); > if (IS_ERR(devr->p0)) { > ret = PTR_ERR(devr->p0); > goto error0; > } > - devr->p0->device = &dev->ib_dev; > - devr->p0->uobject = NULL; > - atomic_set(&devr->p0->usecnt, 0); > > - devr->c0 = mlx5_ib_create_cq(&dev->ib_dev, &cq_attr, NULL, NULL); > + /* must call ib_core here to initialize restrack! */ > + devr->c0 = ib_create_cq(&dev->ib_dev, NULL, NULL, NULL, &cq_attr); > if (IS_ERR(devr->c0)) { > ret = PTR_ERR(devr->c0); > goto error1; > } I think the issue here is that the device is not yet registered at this point. > @@ -4662,14 +4656,16 @@ static int create_dev_resources(struct mlx5_ib_resources *devr) > > error5: > mlx5_ib_destroy_srq(devr->s0); > + atomic_dec(&devr->p0->usecnt); > + atomic_dec(&devr->s0->ext.cq->usecnt); > error4: > mlx5_ib_dealloc_xrcd(devr->x1); > error3: > mlx5_ib_dealloc_xrcd(devr->x0); > error2: > - mlx5_ib_destroy_cq(devr->c0); > + ib_destroy_cq(devr->c0); > error1: > - mlx5_ib_dealloc_pd(devr->p0); > + ib_dealloc_pd(devr->p0); > error0: > return ret; > } > @@ -4681,11 +4677,15 @@ static void destroy_dev_resources(struct mlx5_ib_resources *devr) > int port; > > mlx5_ib_destroy_srq(devr->s1); > + atomic_dec(&devr->p0->usecnt); > + atomic_dec(&devr->s1->ext.cq->usecnt); > mlx5_ib_destroy_srq(devr->s0); > + atomic_dec(&devr->p0->usecnt); > + atomic_dec(&devr->s0->ext.cq->usecnt); > mlx5_ib_dealloc_xrcd(devr->x0); > mlx5_ib_dealloc_xrcd(devr->x1); > - mlx5_ib_destroy_cq(devr->c0); > - mlx5_ib_dealloc_pd(devr->p0); > + ib_destroy_cq(devr->c0); > + ib_dealloc_pd(devr->p0); and has been de-registered by this point. Calling ib_* functions on an unregistered device is not a good idea. Mark? Jason
> -----Original Message----- > From: linux-rdma-owner@vger.kernel.org <linux-rdma- > owner@vger.kernel.org> On Behalf Of Jason Gunthorpe > Sent: Wednesday, October 31, 2018 12:24 PM > To: Shamir Rabinovitch <shamir.rabinovitch@oracle.com>; Mark Bloch > <markb@mellanox.com> > Cc: linux-rdma@vger.kernel.org; dledford@redhat.com; leon@kernel.org; > santosh.shilimkar@oracle.com > Subject: Re: [PATCH v3 1/5] IB/mlx5: device resources must be created from > ib_core > > On Wed, Oct 31, 2018 at 02:42:38PM +0200, Shamir Rabinovitch wrote: > > Bypassing ib_core leave the restrack and other fields uninitialized. > > > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> > > drivers/infiniband/hw/mlx5/main.c | 30 +++++++++++++++--------------- > > 1 file changed, 15 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/infiniband/hw/mlx5/main.c > > b/drivers/infiniband/hw/mlx5/main.c > > index be701d4..97b44e9 100644 > > +++ b/drivers/infiniband/hw/mlx5/main.c > > @@ -4565,26 +4565,19 @@ static int create_dev_resources(struct > > mlx5_ib_resources *devr) > > > > mutex_init(&devr->mutex); > > > > - devr->p0 = mlx5_ib_alloc_pd(&dev->ib_dev, NULL, NULL); > > + /* must call ib_core here to initialize restrack! */ > > + devr->p0 = ib_alloc_pd(&dev->ib_dev, 0); > > if (IS_ERR(devr->p0)) { > > ret = PTR_ERR(devr->p0); > > goto error0; > > } > > - devr->p0->device = &dev->ib_dev; > > - devr->p0->uobject = NULL; > > - atomic_set(&devr->p0->usecnt, 0); > > > > - devr->c0 = mlx5_ib_create_cq(&dev->ib_dev, &cq_attr, NULL, NULL); > > + /* must call ib_core here to initialize restrack! */ > > + devr->c0 = ib_create_cq(&dev->ib_dev, NULL, NULL, NULL, &cq_attr); > > if (IS_ERR(devr->c0)) { > > ret = PTR_ERR(devr->c0); > > goto error1; > > } > > I think the issue here is that the device is not yet registered at this point. > > > @@ -4662,14 +4656,16 @@ static int create_dev_resources(struct > > mlx5_ib_resources *devr) > > > > error5: > > mlx5_ib_destroy_srq(devr->s0); > > + atomic_dec(&devr->p0->usecnt); > > + atomic_dec(&devr->s0->ext.cq->usecnt); > > error4: > > mlx5_ib_dealloc_xrcd(devr->x1); > > error3: > > mlx5_ib_dealloc_xrcd(devr->x0); > > error2: > > - mlx5_ib_destroy_cq(devr->c0); > > + ib_destroy_cq(devr->c0); > > error1: > > - mlx5_ib_dealloc_pd(devr->p0); > > + ib_dealloc_pd(devr->p0); > > error0: > > return ret; > > } > > @@ -4681,11 +4677,15 @@ static void destroy_dev_resources(struct > mlx5_ib_resources *devr) > > int port; > > > > mlx5_ib_destroy_srq(devr->s1); > > + atomic_dec(&devr->p0->usecnt); > > + atomic_dec(&devr->s1->ext.cq->usecnt); > > mlx5_ib_destroy_srq(devr->s0); > > + atomic_dec(&devr->p0->usecnt); > > + atomic_dec(&devr->s0->ext.cq->usecnt); > > mlx5_ib_dealloc_xrcd(devr->x0); > > mlx5_ib_dealloc_xrcd(devr->x1); > > - mlx5_ib_destroy_cq(devr->c0); > > - mlx5_ib_dealloc_pd(devr->p0); > > + ib_destroy_cq(devr->c0); > > + ib_dealloc_pd(devr->p0); > > and has been de-registered by this point. > Yes. This also conflicts with the other series that I am doing to unregister-register device in ib core. So existing code is more appropriate. > Calling ib_* functions on an unregistered device is not a good idea. > > Mark? > > Jason
On 10/31/2018 10:24 AM, Jason Gunthorpe wrote: > On Wed, Oct 31, 2018 at 02:42:38PM +0200, Shamir Rabinovitch wrote: >> Bypassing ib_core leave the restrack and other fields uninitialized. >> >> Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> >> drivers/infiniband/hw/mlx5/main.c | 30 +++++++++++++++--------------- >> 1 file changed, 15 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c >> index be701d4..97b44e9 100644 >> +++ b/drivers/infiniband/hw/mlx5/main.c >> @@ -4565,26 +4565,19 @@ static int create_dev_resources(struct mlx5_ib_resources *devr) >> >> mutex_init(&devr->mutex); >> >> - devr->p0 = mlx5_ib_alloc_pd(&dev->ib_dev, NULL, NULL); >> + /* must call ib_core here to initialize restrack! */ >> + devr->p0 = ib_alloc_pd(&dev->ib_dev, 0); >> if (IS_ERR(devr->p0)) { >> ret = PTR_ERR(devr->p0); >> goto error0; >> } >> - devr->p0->device = &dev->ib_dev; >> - devr->p0->uobject = NULL; >> - atomic_set(&devr->p0->usecnt, 0); >> >> - devr->c0 = mlx5_ib_create_cq(&dev->ib_dev, &cq_attr, NULL, NULL); >> + /* must call ib_core here to initialize restrack! */ >> + devr->c0 = ib_create_cq(&dev->ib_dev, NULL, NULL, NULL, &cq_attr); >> if (IS_ERR(devr->c0)) { >> ret = PTR_ERR(devr->c0); >> goto error1; >> } > > I think the issue here is that the device is not yet registered at > this point. > >> @@ -4662,14 +4656,16 @@ static int create_dev_resources(struct mlx5_ib_resources *devr) >> >> error5: >> mlx5_ib_destroy_srq(devr->s0); >> + atomic_dec(&devr->p0->usecnt); >> + atomic_dec(&devr->s0->ext.cq->usecnt); >> error4: >> mlx5_ib_dealloc_xrcd(devr->x1); >> error3: >> mlx5_ib_dealloc_xrcd(devr->x0); >> error2: >> - mlx5_ib_destroy_cq(devr->c0); >> + ib_destroy_cq(devr->c0); >> error1: >> - mlx5_ib_dealloc_pd(devr->p0); >> + ib_dealloc_pd(devr->p0); >> error0: >> return ret; >> } >> @@ -4681,11 +4677,15 @@ static void destroy_dev_resources(struct mlx5_ib_resources *devr) >> int port; >> >> mlx5_ib_destroy_srq(devr->s1); >> + atomic_dec(&devr->p0->usecnt); >> + atomic_dec(&devr->s1->ext.cq->usecnt); >> mlx5_ib_destroy_srq(devr->s0); >> + atomic_dec(&devr->p0->usecnt); >> + atomic_dec(&devr->s0->ext.cq->usecnt); >> mlx5_ib_dealloc_xrcd(devr->x0); >> mlx5_ib_dealloc_xrcd(devr->x1); >> - mlx5_ib_destroy_cq(devr->c0); >> - mlx5_ib_dealloc_pd(devr->p0); >> + ib_destroy_cq(devr->c0); >> + ib_dealloc_pd(devr->p0); > > and has been de-registered by this point. > > Calling ib_* functions on an unregistered device is not a good idea. > > Mark? Yep, I don't like it, any change to the create/destroy functions will now need to take into account that:"Don't play with struct ib_device too much as mlx5 does some funky stuff before the device is registered" Seems like a bad idea. > > Jason > Mark
On Wed, Oct 31, 2018 at 06:09:28PM +0000, Mark Bloch wrote: > [ ... ] > >> mlx5_ib_dealloc_xrcd(devr->x1); > >> - mlx5_ib_destroy_cq(devr->c0); > >> - mlx5_ib_dealloc_pd(devr->p0); > >> + ib_destroy_cq(devr->c0); > >> + ib_dealloc_pd(devr->p0); > > > > and has been de-registered by this point. > > > > Calling ib_* functions on an unregistered device is not a good idea. > > > > Mark? > > Yep, I don't like it, any change to the create/destroy functions > will now need to take into account that:"Don't play with struct ib_device too much > as mlx5 does some funky stuff before the device is registered" > > Seems like a bad idea. > > > > > Jason > > > > Mark Reason for this is that follow patches will start use the restrack to figure if pd was created by user or not and this is used even in drivers such as mlx5. So not having restrack initialized is bad idea also. I can initialize the restrack here and skip the calls to the ib_x functions. Will this be acceptable ? I do think that it's bad idea that we have objects that are partially initialized and that when accessed by driver, driver need to be careful not to touch the uninitialized parts.
On Thu, Nov 01, 2018 at 12:40:58PM +0200, Shamir Rabinovitch wrote: > On Wed, Oct 31, 2018 at 06:09:28PM +0000, Mark Bloch wrote: > > > [ ... ] > > >> mlx5_ib_dealloc_xrcd(devr->x1); > > >> - mlx5_ib_destroy_cq(devr->c0); > > >> - mlx5_ib_dealloc_pd(devr->p0); > > >> + ib_destroy_cq(devr->c0); > > >> + ib_dealloc_pd(devr->p0); > > > > > > and has been de-registered by this point. > > > > > > Calling ib_* functions on an unregistered device is not a good idea. > > > > > > Mark? > > > > Yep, I don't like it, any change to the create/destroy functions > > will now need to take into account that:"Don't play with struct ib_device too much > > as mlx5 does some funky stuff before the device is registered" > > > > Seems like a bad idea. > > > > > > > > Jason > > > > > > > Mark > > Reason for this is that follow patches will start use the restrack to > figure if pd was created by user or not and this is used even in drivers > such as mlx5. So not having restrack initialized is bad idea also. I can > initialize the restrack here and skip the calls to the ib_x functions. > > Will this be acceptable ? You will need to take into account that restrack DB is exposed through RDMA nldev to rdmatool, but for now, I don't see a problem with it, we have Steve's extension to return vendor specific data for already known objects. So yes, why not? > > I do think that it's bad idea that we have objects that are partially > initialized and that when accessed by driver, driver need to be > careful not to touch the uninitialized parts.
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c index be701d4..97b44e9 100644 --- a/drivers/infiniband/hw/mlx5/main.c +++ b/drivers/infiniband/hw/mlx5/main.c @@ -4565,26 +4565,19 @@ static int create_dev_resources(struct mlx5_ib_resources *devr) mutex_init(&devr->mutex); - devr->p0 = mlx5_ib_alloc_pd(&dev->ib_dev, NULL, NULL); + /* must call ib_core here to initialize restrack! */ + devr->p0 = ib_alloc_pd(&dev->ib_dev, 0); if (IS_ERR(devr->p0)) { ret = PTR_ERR(devr->p0); goto error0; } - devr->p0->device = &dev->ib_dev; - devr->p0->uobject = NULL; - atomic_set(&devr->p0->usecnt, 0); - devr->c0 = mlx5_ib_create_cq(&dev->ib_dev, &cq_attr, NULL, NULL); + /* must call ib_core here to initialize restrack! */ + devr->c0 = ib_create_cq(&dev->ib_dev, NULL, NULL, NULL, &cq_attr); if (IS_ERR(devr->c0)) { ret = PTR_ERR(devr->c0); goto error1; } - devr->c0->device = &dev->ib_dev; - devr->c0->uobject = NULL; - devr->c0->comp_handler = NULL; - devr->c0->event_handler = NULL; - devr->c0->cq_context = NULL; - atomic_set(&devr->c0->usecnt, 0); devr->x0 = mlx5_ib_alloc_xrcd(&dev->ib_dev, NULL, NULL); if (IS_ERR(devr->x0)) { @@ -4649,6 +4642,7 @@ static int create_dev_resources(struct mlx5_ib_resources *devr) devr->s1->srq_context = NULL; devr->s1->srq_type = IB_SRQT_BASIC; devr->s1->ext.cq = devr->c0; + atomic_inc(&devr->s1->ext.cq->usecnt); atomic_inc(&devr->p0->usecnt); atomic_set(&devr->s1->usecnt, 0); @@ -4662,14 +4656,16 @@ static int create_dev_resources(struct mlx5_ib_resources *devr) error5: mlx5_ib_destroy_srq(devr->s0); + atomic_dec(&devr->p0->usecnt); + atomic_dec(&devr->s0->ext.cq->usecnt); error4: mlx5_ib_dealloc_xrcd(devr->x1); error3: mlx5_ib_dealloc_xrcd(devr->x0); error2: - mlx5_ib_destroy_cq(devr->c0); + ib_destroy_cq(devr->c0); error1: - mlx5_ib_dealloc_pd(devr->p0); + ib_dealloc_pd(devr->p0); error0: return ret; } @@ -4681,11 +4677,15 @@ static void destroy_dev_resources(struct mlx5_ib_resources *devr) int port; mlx5_ib_destroy_srq(devr->s1); + atomic_dec(&devr->p0->usecnt); + atomic_dec(&devr->s1->ext.cq->usecnt); mlx5_ib_destroy_srq(devr->s0); + atomic_dec(&devr->p0->usecnt); + atomic_dec(&devr->s0->ext.cq->usecnt); mlx5_ib_dealloc_xrcd(devr->x0); mlx5_ib_dealloc_xrcd(devr->x1); - mlx5_ib_destroy_cq(devr->c0); - mlx5_ib_dealloc_pd(devr->p0); + ib_destroy_cq(devr->c0); + ib_dealloc_pd(devr->p0); /* Make sure no change P_Key work items are still executing */ for (port = 0; port < dev->num_ports; ++port)
Bypassing ib_core leave the restrack and other fields uninitialized. Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com> --- drivers/infiniband/hw/mlx5/main.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)