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 Jason
