On Mon, Feb 04, 2019 at 08:14:31PM +0200, Leon Romanovsky wrote: > 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. Then don't write callers that need to use it.. Jason
