On Thu, Jan 31, 2019 at 07:58:02PM +0200, Leon Romanovsky wrote:
> On Thu, Jan 31, 2019 at 10:38:55AM -0700, Jason Gunthorpe wrote:
> > On Thu, Jan 31, 2019 at 07:30:56PM +0200, Leon Romanovsky wrote:
> > > On Wed, Jan 30, 2019 at 09:24:56PM -0700, Jason Gunthorpe wrote:
> > > > On Wed, Jan 30, 2019 at 12:49:04PM +0200, Leon Romanovsky wrote:
> > > > > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > >
> > > > > As a preparation to extension of rdma_restrack_root to provide
> > > > > software IDs, which will be per-type too. We convert the
> > > > > rdma_restrack_root from struct with arrays to array of structs.
> > > > >
> > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > drivers/infiniband/core/restrack.c | 35 +++++++++++++++---------------
> > > > > 1 file changed, 17 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > > > > index 284d087d7785..bcd6fc9d3225 100644
> > > > > +++ b/drivers/infiniband/core/restrack.c
> > > > > @@ -46,17 +46,13 @@ struct rdma_restrack_root {
> > > > > */
> > > > > struct rw_semaphore rwsem;
> > > > > /**
> > > > > - * @xa: Array of XArray structures to hold restrack entries.
> > > > > - * We want to use array of XArrays because insertion is type
> > > > > - * dependent. For types with xisiting unique ID (like QPN),
> > > > > - * we will insert to that unique index. For other types,
> > > > > - * we insert based on pointers and auto-allocate unique index.
> > > > > + * @xa: Array of XArray structure to hold restrack entries.
> > > > > */
> > > > > - struct xarray xa[RDMA_RESTRACK_MAX];
> > > > > + struct xarray xa;
> > > > > /**
> > > > > * #next_id: Next ID to support cyclic allocation
> > > > > */
> > > > > - u32 next_id[RDMA_RESTRACK_MAX];
> > > > > + u32 next_id;
> > > > > };
> > > > >
> > > > > /**
> > > > > @@ -70,15 +66,16 @@ int rdma_restrack_init(struct ib_device *dev)
> > > > > struct rdma_restrack_root *rt;
> > > > > int i;
> > > > >
> > > > > - dev->res = kzalloc(sizeof(*rt), GFP_KERNEL);
> > > > > + dev->res = kcalloc(RDMA_RESTRACK_MAX, sizeof(*rt), GFP_KERNEL);
> > > > > if (!dev->res)
> > > > > return -ENOMEM;
> > > > >
> > > > > rt = dev->res;
> > > > >
> > > > > - for (i = 0 ; i < RDMA_RESTRACK_MAX; i++)
> > > > > - xa_init_flags(&rt->xa[i], XA_FLAGS_ALLOC);
> > > > > - init_rwsem(&rt->rwsem);
> > > > > + for (i = 0 ; i < RDMA_RESTRACK_MAX; i++) {
> > > > > + init_rwsem(&rt[i].rwsem);
> > > > > + xa_init_flags(&rt[i].xa, XA_FLAGS_ALLOC);
> > > > > + }
> > > > >
> > > > > return 0;
> > > > > }
> > > > > @@ -107,7 +104,7 @@ static const char *type2str(enum rdma_restrack_type type)
> > > > > struct xarray *rdma_dev_to_xa(struct ib_device *dev,
> > > > > enum rdma_restrack_type type)
> > > > > {
> > > > > - return &dev->res->xa[type];
> > > > > + return &dev->res[type].xa;
> > > > >
> > > > > }
> > > > > EXPORT_SYMBOL(rdma_dev_to_xa);
> > > > > @@ -120,7 +117,7 @@ EXPORT_SYMBOL(rdma_dev_to_xa);
> > > > > */
> > > > > void rdma_rt_read_lock(struct ib_device *dev, enum rdma_restrack_type type)
> > > > > {
> > > > > - down_read(&dev->res->rwsem);
> > > > > + down_read(&dev->res[type].rwsem);
> > > > > }
> > > > > EXPORT_SYMBOL(rdma_rt_read_lock);
> > > > >
> > > > > @@ -132,7 +129,7 @@ EXPORT_SYMBOL(rdma_rt_read_lock);
> > > > > */
> > > > > void rdma_rt_read_unlock(struct ib_device *dev, enum rdma_restrack_type type)
> > > > > {
> > > > > - up_read(&dev->res->rwsem);
> > > > > + up_read(&dev->res[type].rwsem);
> > > > > }
> > > > > EXPORT_SYMBOL(rdma_rt_read_unlock);
> > > > >
> > > > > @@ -286,7 +283,7 @@ static void rdma_restrack_add(struct rdma_restrack_entry *res)
> > > > > if (!dev)
> > > > > return;
> > > > >
> > > > > - rt = dev->res;
> > > > > + rt = &dev->res[res->type];
> > > > > xa = rdma_dev_to_xa(dev, res->type);
> > > >
> > > > This whole rdma_dev_to_xa thing looks pretty pointless now that xa is
> > > > just rt->xa
> > > >
> > > > I feel like this patch should be pushed down to be right
> > > > after/before/or part of the xarray introduction patch
> > >
> > > I prefer to keep it as is, except painful rebase your proposal
> > > won't give us much, the outcome will be the same.
> >
> > It means you won't add a function in one patch and then just delete
> > the entire function in the next patch, which is fairly bad
> > practice. All the patch ordering in this series can be improved to
> > make it more understandable.
>
> I didn't delete rdma_dev_to_xa(), that function was introduced in
> "RDMA/restrack: Hide restrack DB from IB/core" patch.
My remark is that you should delete it now that everything trivially
has rt->xa
Jason
