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. Jason
