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.
diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
index f77e5018dcb4..6e0ea878d78f 100644
--- a/drivers/infiniband/core/nldev.c
+++ b/drivers/infiniband/core/nldev.c
@@ -1157,7 +1157,7 @@ static int res_get_common_dumpit(struct sk_buff *skb,
has_cap_net_admin = netlink_capable(cb->skb, CAP_NET_ADMIN);
xa = rdma_dev_to_xa(device, res_type);
- rdma_rt_read_lock(device, res_type);
+ rdma_rt_read_lock(xa);
xa_for_each(xa, id, res) {
if (idx < start)
goto next;
@@ -1179,13 +1179,13 @@ static int res_get_common_dumpit(struct sk_buff *skb,
if (!entry_attr) {
ret = -EMSGSIZE;
rdma_restrack_put(res);
- rdma_rt_read_unlock(device, res_type);
+ rdma_rt_read_unlock(xa);
break;
}
- rdma_rt_read_unlock(device, res_type);
+ rdma_rt_read_unlock(xa);
ret = fe->fill_res_func(skb, has_cap_net_admin, res, port);
- rdma_rt_read_lock(device, res_type);
+ rdma_rt_read_lock(xa);
/*
* Return resource back, but it won't be released till
* the &device->res.rwsem will be released for write.
@@ -1206,7 +1206,7 @@ static int res_get_common_dumpit(struct sk_buff *skb,
nla_nest_end(skb, entry_attr);
next: idx++;
}
- rdma_rt_read_unlock(device, res_type);
+ rdma_rt_read_unlock(xa);
nla_nest_end(skb, table_attr);
nlmsg_end(skb, nlh);
@@ -1224,7 +1224,7 @@ next: idx++;
res_err:
nla_nest_cancel(skb, table_attr);
- rdma_rt_read_unlock(device, res_type);
+ rdma_rt_read_unlock(xa);
err:
nlmsg_cancel(skb, nlh);
diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
index 609af618a3e6..045016f2ab55 100644
--- a/drivers/infiniband/core/restrack.c
+++ b/drivers/infiniband/core/restrack.c
@@ -112,24 +112,28 @@ EXPORT_SYMBOL(rdma_dev_to_xa);
/**
* rdma_rt_read_lock() - Lock XArray for read, needed while iterating
* with xa_for_each()
- * @dev: IB device to work
- * @type: resource track type
+ * @xa: XArray to lock
*/
-void rdma_rt_read_lock(struct ib_device *dev, enum rdma_restrack_type type)
+void rdma_rt_read_lock(struct xarray *xa)
{
- down_read(&dev->res[type].rwsem);
+ struct rdma_restrack_root *rt =
+ container_of(xa, struct rdma_restrack_root, xa);
+
+ down_read(&rt->rwsem);
}
EXPORT_SYMBOL(rdma_rt_read_lock);
/**
* rdma_rt_read_unlock() - Unlock XArray for read, needed while iterating
* with xa_for_each()
- * @dev: IB device to work
- * @type: resource track type
+ * @xa: XArray to unlock
*/
-void rdma_rt_read_unlock(struct ib_device *dev, enum rdma_restrack_type type)
+void rdma_rt_read_unlock(struct xarray *xa)
{
- up_read(&dev->res[type].rwsem);
+ struct rdma_restrack_root *rt =
+ container_of(xa, struct rdma_restrack_root, xa);
+
+ up_read(&rt->rwsem);
}
EXPORT_SYMBOL(rdma_rt_read_unlock);
@@ -198,14 +202,14 @@ int rdma_restrack_count(struct ib_device *dev, enum rdma_restrack_type type,
unsigned long index = 0;
u32 cnt = 0;
- rdma_rt_read_lock(dev, type);
+ rdma_rt_read_lock(xa);
xa_for_each(xa, index, e) {
if (ns == &init_pid_ns ||
(!rdma_is_kernel_res(e) &&
ns == task_active_pid_ns(e->task)))
cnt++;
}
- rdma_rt_read_unlock(dev, type);
+ rdma_rt_read_unlock(xa);
return cnt;
}
EXPORT_SYMBOL(rdma_restrack_count);
diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
index c81e43d043cf..48705e3f0ed9 100644
--- a/include/rdma/restrack.h
+++ b/include/rdma/restrack.h
@@ -159,8 +159,8 @@ struct rdma_restrack_entry *rdma_restrack_get_byid(struct ib_device *dev,
u32 id);
struct xarray *rdma_dev_to_xa(struct ib_device *dev,
enum rdma_restrack_type type);
-void rdma_rt_read_lock(struct ib_device *dev, enum rdma_restrack_type type);
-void rdma_rt_read_unlock(struct ib_device *dev, enum rdma_restrack_type type);
+void rdma_rt_read_lock(struct xarray *xa);
+void rdma_rt_read_unlock(struct xarray *xa);
/**
* rdma_res_to_id() - Unique ID as seen by restrack
Please allow other opinions and coding preferences co-exist in RDMA subsystem.
Thanks
>
> Jason
Attachment:
signature.asc
Description: PGP signature
