| Message ID | 20190221002107.22625-28-willy@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Convert the Infiniband subsystem to XArray | expand |
On Wed, Feb 20, 2019 at 04:21:02PM -0800, Matthew Wilcox wrote: > This driver doesn't look up the pointer that it stores, so this can > just be an IDA and save some memory. > > Signed-off-by: Matthew Wilcox <willy@infradead.org> > --- > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_main.c b/drivers/infiniband/hw/ocrdma/ocrdma_main.c > index 1f393842453a..b014eeffbcce 100644 > --- a/drivers/infiniband/hw/ocrdma/ocrdma_main.c > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_main.c > @@ -62,7 +62,7 @@ MODULE_DESCRIPTION(OCRDMA_ROCE_DRV_DESC " " OCRDMA_ROCE_DRV_VERSION); > MODULE_AUTHOR("Emulex Corporation"); > MODULE_LICENSE("Dual BSD/GPL"); > > -static DEFINE_IDR(ocrdma_dev_id); > +static DEFINE_IDA(ocrdma_dev_id); > > void ocrdma_get_guid(struct ocrdma_dev *dev, u8 *guid) > { > @@ -302,12 +302,12 @@ static struct ocrdma_dev *ocrdma_add(struct be_dev_info *dev_info) > } > dev->mbx_cmd = kzalloc(sizeof(struct ocrdma_mqe_emb_cmd), GFP_KERNEL); > if (!dev->mbx_cmd) > - goto idr_err; > + goto ida_err; > > memcpy(&dev->nic_info, dev_info, sizeof(*dev_info)); > - dev->id = idr_alloc(&ocrdma_dev_id, NULL, 0, 0, GFP_KERNEL); > + dev->id = ida_alloc(&ocrdma_dev_id, GFP_KERNEL); WTF.. There are only three places where this id is used: - A whole bunch of pr_* calls which *really* should be dev_* calls with no ID (grrrr) - It is passed to userspace in the ucontext udata, and userpace ignores it (double grrrrr) - It is used to form a name for request_irq (use the pci BDF) We should delete this thing :( Jason
On Fri, Feb 22, 2019 at 4:55 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Wed, Feb 20, 2019 at 04:21:02PM -0800, Matthew Wilcox wrote: > > This driver doesn't look up the pointer that it stores, so this can > > just be an IDA and save some memory. > > > > Signed-off-by: Matthew Wilcox <willy@infradead.org> > > --- > > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_main.c b/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > index 1f393842453a..b014eeffbcce 100644 > > --- a/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > @@ -62,7 +62,7 @@ MODULE_DESCRIPTION(OCRDMA_ROCE_DRV_DESC " " OCRDMA_ROCE_DRV_VERSION); > > MODULE_AUTHOR("Emulex Corporation"); > > MODULE_LICENSE("Dual BSD/GPL"); > > > > -static DEFINE_IDR(ocrdma_dev_id); > > +static DEFINE_IDA(ocrdma_dev_id); > > > > void ocrdma_get_guid(struct ocrdma_dev *dev, u8 *guid) > > { > > @@ -302,12 +302,12 @@ static struct ocrdma_dev *ocrdma_add(struct be_dev_info *dev_info) > > } > > dev->mbx_cmd = kzalloc(sizeof(struct ocrdma_mqe_emb_cmd), GFP_KERNEL); > > if (!dev->mbx_cmd) > > - goto idr_err; > > + goto ida_err; > > > > memcpy(&dev->nic_info, dev_info, sizeof(*dev_info)); > > - dev->id = idr_alloc(&ocrdma_dev_id, NULL, 0, 0, GFP_KERNEL); > > + dev->id = ida_alloc(&ocrdma_dev_id, GFP_KERNEL); > > WTF.. There are only three places where this id is used: > - A whole bunch of pr_* calls which *really* should be dev_* calls > with no ID (grrrr) > - It is passed to userspace in the ucontext udata, and userpace > ignores it (double grrrrr) > - It is used to form a name for request_irq (use the pci BDF) > > We should delete this thing :( > Will send a patch to remove this. > Jason
On Wed, Feb 27, 2019 at 4:45 AM Devesh Sharma <devesh.sharma@broadcom.com> wrote: > > On Fri, Feb 22, 2019 at 4:55 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Wed, Feb 20, 2019 at 04:21:02PM -0800, Matthew Wilcox wrote: > > > This driver doesn't look up the pointer that it stores, so this can > > > just be an IDA and save some memory. > > > > > > Signed-off-by: Matthew Wilcox <willy@infradead.org> > > > --- > > > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 16 +++++++--------- > > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_main.c b/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > > index 1f393842453a..b014eeffbcce 100644 > > > --- a/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > > @@ -62,7 +62,7 @@ MODULE_DESCRIPTION(OCRDMA_ROCE_DRV_DESC " " OCRDMA_ROCE_DRV_VERSION); > > > MODULE_AUTHOR("Emulex Corporation"); > > > MODULE_LICENSE("Dual BSD/GPL"); > > > > > > -static DEFINE_IDR(ocrdma_dev_id); > > > +static DEFINE_IDA(ocrdma_dev_id); > > > > > > void ocrdma_get_guid(struct ocrdma_dev *dev, u8 *guid) > > > { > > > @@ -302,12 +302,12 @@ static struct ocrdma_dev *ocrdma_add(struct be_dev_info *dev_info) > > > } > > > dev->mbx_cmd = kzalloc(sizeof(struct ocrdma_mqe_emb_cmd), GFP_KERNEL); > > > if (!dev->mbx_cmd) > > > - goto idr_err; > > > + goto ida_err; > > > > > > memcpy(&dev->nic_info, dev_info, sizeof(*dev_info)); > > > - dev->id = idr_alloc(&ocrdma_dev_id, NULL, 0, 0, GFP_KERNEL); > > > + dev->id = ida_alloc(&ocrdma_dev_id, GFP_KERNEL); > > > > WTF.. There are only three places where this id is used: > > - A whole bunch of pr_* calls which *really* should be dev_* calls > > with no ID (grrrr) > > - It is passed to userspace in the ucontext udata, and userpace > > ignores it (double grrrrr) > > - It is used to form a name for request_irq (use the pci BDF) > > > > We should delete this thing :( > > > > Will send a patch to remove this. Devesh, will a patch be coming? Thanks, Jason
On Fri, Mar 29, 2019 at 11:27 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Wed, Feb 27, 2019 at 4:45 AM Devesh Sharma > <devesh.sharma@broadcom.com> wrote: > > > > On Fri, Feb 22, 2019 at 4:55 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > On Wed, Feb 20, 2019 at 04:21:02PM -0800, Matthew Wilcox wrote: > > > > This driver doesn't look up the pointer that it stores, so this can > > > > just be an IDA and save some memory. > > > > > > > > Signed-off-by: Matthew Wilcox <willy@infradead.org> > > > > --- > > > > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 16 +++++++--------- > > > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_main.c b/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > > > index 1f393842453a..b014eeffbcce 100644 > > > > --- a/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > > > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > > > @@ -62,7 +62,7 @@ MODULE_DESCRIPTION(OCRDMA_ROCE_DRV_DESC " " OCRDMA_ROCE_DRV_VERSION); > > > > MODULE_AUTHOR("Emulex Corporation"); > > > > MODULE_LICENSE("Dual BSD/GPL"); > > > > > > > > -static DEFINE_IDR(ocrdma_dev_id); > > > > +static DEFINE_IDA(ocrdma_dev_id); > > > > > > > > void ocrdma_get_guid(struct ocrdma_dev *dev, u8 *guid) > > > > { > > > > @@ -302,12 +302,12 @@ static struct ocrdma_dev *ocrdma_add(struct be_dev_info *dev_info) > > > > } > > > > dev->mbx_cmd = kzalloc(sizeof(struct ocrdma_mqe_emb_cmd), GFP_KERNEL); > > > > if (!dev->mbx_cmd) > > > > - goto idr_err; > > > > + goto ida_err; > > > > > > > > memcpy(&dev->nic_info, dev_info, sizeof(*dev_info)); > > > > - dev->id = idr_alloc(&ocrdma_dev_id, NULL, 0, 0, GFP_KERNEL); > > > > + dev->id = ida_alloc(&ocrdma_dev_id, GFP_KERNEL); > > > > > > WTF.. There are only three places where this id is used: > > > - A whole bunch of pr_* calls which *really* should be dev_* calls > > > with no ID (grrrr) > > > - It is passed to userspace in the ucontext udata, and userpace > > > ignores it (double grrrrr) > > > - It is used to form a name for request_irq (use the pci BDF) > > > > > > We should delete this thing :( > > > > > > > Will send a patch to remove this. > > Devesh, will a patch be coming? > Yeah Jason, I will send it this week. > Thanks, > Jason
On Mon, Apr 1, 2019 at 11:41 PM Devesh Sharma <devesh.sharma@broadcom.com> wrote: > > On Fri, Mar 29, 2019 at 11:27 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Wed, Feb 27, 2019 at 4:45 AM Devesh Sharma > > <devesh.sharma@broadcom.com> wrote: > > > > > > On Fri, Feb 22, 2019 at 4:55 AM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > > > On Wed, Feb 20, 2019 at 04:21:02PM -0800, Matthew Wilcox wrote: > > > > > This driver doesn't look up the pointer that it stores, so this can > > > > > just be an IDA and save some memory. > > > > > > > > > > Signed-off-by: Matthew Wilcox <willy@infradead.org> > > > > > --- > > > > > drivers/infiniband/hw/ocrdma/ocrdma_main.c | 16 +++++++--------- > > > > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_main.c b/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > > > > index 1f393842453a..b014eeffbcce 100644 > > > > > --- a/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > > > > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_main.c > > > > > @@ -62,7 +62,7 @@ MODULE_DESCRIPTION(OCRDMA_ROCE_DRV_DESC " " OCRDMA_ROCE_DRV_VERSION); > > > > > MODULE_AUTHOR("Emulex Corporation"); > > > > > MODULE_LICENSE("Dual BSD/GPL"); > > > > > > > > > > -static DEFINE_IDR(ocrdma_dev_id); > > > > > +static DEFINE_IDA(ocrdma_dev_id); > > > > > > > > > > void ocrdma_get_guid(struct ocrdma_dev *dev, u8 *guid) > > > > > { > > > > > @@ -302,12 +302,12 @@ static struct ocrdma_dev *ocrdma_add(struct be_dev_info *dev_info) > > > > > } > > > > > dev->mbx_cmd = kzalloc(sizeof(struct ocrdma_mqe_emb_cmd), GFP_KERNEL); > > > > > if (!dev->mbx_cmd) > > > > > - goto idr_err; > > > > > + goto ida_err; > > > > > > > > > > memcpy(&dev->nic_info, dev_info, sizeof(*dev_info)); > > > > > - dev->id = idr_alloc(&ocrdma_dev_id, NULL, 0, 0, GFP_KERNEL); > > > > > + dev->id = ida_alloc(&ocrdma_dev_id, GFP_KERNEL); > > > > > > > > WTF.. There are only three places where this id is used: > > > > - A whole bunch of pr_* calls which *really* should be dev_* calls > > > > with no ID (grrrr) > > > > - It is passed to userspace in the ucontext udata, and userpace > > > > ignores it (double grrrrr) > > > > - It is used to form a name for request_irq (use the pci BDF) > > > > > > > > We should delete this thing :( > > > > > > > > > > Will send a patch to remove this. > > > > Devesh, will a patch be coming? > > > Yeah Jason, I will send it this week. > > Thanks, > > Jason Hey Jason, still trying to test the patch, I will be sending this in a day or two.
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_main.c b/drivers/infiniband/hw/ocrdma/ocrdma_main.c index 1f393842453a..b014eeffbcce 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_main.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_main.c @@ -62,7 +62,7 @@ MODULE_DESCRIPTION(OCRDMA_ROCE_DRV_DESC " " OCRDMA_ROCE_DRV_VERSION); MODULE_AUTHOR("Emulex Corporation"); MODULE_LICENSE("Dual BSD/GPL"); -static DEFINE_IDR(ocrdma_dev_id); +static DEFINE_IDA(ocrdma_dev_id); void ocrdma_get_guid(struct ocrdma_dev *dev, u8 *guid) { @@ -302,12 +302,12 @@ static struct ocrdma_dev *ocrdma_add(struct be_dev_info *dev_info) } dev->mbx_cmd = kzalloc(sizeof(struct ocrdma_mqe_emb_cmd), GFP_KERNEL); if (!dev->mbx_cmd) - goto idr_err; + goto ida_err; memcpy(&dev->nic_info, dev_info, sizeof(*dev_info)); - dev->id = idr_alloc(&ocrdma_dev_id, NULL, 0, 0, GFP_KERNEL); + dev->id = ida_alloc(&ocrdma_dev_id, GFP_KERNEL); if (dev->id < 0) - goto idr_err; + goto ida_err; status = ocrdma_init_hw(dev); if (status) @@ -345,8 +345,8 @@ static struct ocrdma_dev *ocrdma_add(struct be_dev_info *dev_info) ocrdma_free_resources(dev); ocrdma_cleanup_hw(dev); init_err: - idr_remove(&ocrdma_dev_id, dev->id); -idr_err: + ida_free(&ocrdma_dev_id, dev->id); +ida_err: kfree(dev->mbx_cmd); ib_dealloc_device(&dev->ibdev); pr_err("%s() leaving. ret=%d\n", __func__, status); @@ -355,8 +355,7 @@ static struct ocrdma_dev *ocrdma_add(struct be_dev_info *dev_info) static void ocrdma_remove_free(struct ocrdma_dev *dev) { - - idr_remove(&ocrdma_dev_id, dev->id); + ida_free(&ocrdma_dev_id, dev->id); kfree(dev->mbx_cmd); ib_dealloc_device(&dev->ibdev); } @@ -461,7 +460,6 @@ static void __exit ocrdma_exit_module(void) { be_roce_unregister_driver(&ocrdma_drv); ocrdma_rem_debugfs(); - idr_destroy(&ocrdma_dev_id); } module_init(ocrdma_init_module);
This driver doesn't look up the pointer that it stores, so this can just be an IDA and save some memory. Signed-off-by: Matthew Wilcox <willy@infradead.org> --- drivers/infiniband/hw/ocrdma/ocrdma_main.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)