Re: [PATCH rdma-next v3 12/19] RDMA/restrack: Prepare restrack_root to addition of extra fields per-type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Feb 04, 2019 at 11:09:35AM -0700, Jason Gunthorpe wrote:
> On Sat, Feb 02, 2019 at 11:32:19AM +0200, Leon Romanovsky wrote:
> > On Fri, Feb 01, 2019 at 09:05:06AM -0700, Jason Gunthorpe wrote:
> > > On Fri, Feb 01, 2019 at 09:16:29AM +0200, Leon Romanovsky wrote:
> > > > On Thu, Jan 31, 2019 at 03:29:00PM -0700, Jason Gunthorpe wrote:
> > > > > On Fri, Feb 01, 2019 at 12:10:55AM +0200, Leon Romanovsky wrote:
> > > > > > > > > > > > -	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
> > > > > >
> > > > > > Is this "must"? I really like this helper and it is easier
> > > > > > for me than "&dev->res[type].xa"
> > > > >
> > > > > You should use this approach everywhere:
> > > > >
> > > > > 	rt = &dev->res[res->type];
> > > > >         down_read(&rt->rwsem);
> > > > >         xa_load(&rt->xa);
> > > > >
> > > > > Instead of building a whole bunch of wakky wrapper APIs that take type
> > > > > as an argument.
> > > >
> > > >
> > > > rdma_dev_to_xa() is used outside of restrack.c (e.g. nldev.c) and it is
> > > > designed to hide restrack_root.
> > >
> > > Since you can't really use the xa without the lock that goes with it,
> > > you should still stick with the struct instead of a bunch of one line
> > > wrapper accessors
> >
> > What about this change? It removes "type" argument from lock/unlock
> > function. I really don't want to expose restrack internals outside
> > of restrack.c. This change makes me one step closer from hiding "type"
> > and rdma_restrack_add()/rdma_restrack_del() will work automatically
> > without need to provide type at all.
>
> That is even less type-safe..
>
> This is the kernel, you shouldn't be afraid to expose a struct if it
> makes sense to do so. If places outside the core are going to be
> locking and using this xarray, then expose it via struct not with
> function wrappers.

This is the difference between us, I don't think that it makes sense to
expose this lock.

Thanks

>
> Jason

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux