On Fri, Jan 04, 2019 at 05:24:21PM +0000, Jason Gunthorpe wrote:
> On Fri, Jan 04, 2019 at 07:12:54PM +0200, Leon Romanovsky wrote:
> > On Fri, Jan 04, 2019 at 04:23:49PM +0000, Jason Gunthorpe wrote:
> > > On Fri, Jan 04, 2019 at 08:05:06AM +0200, Leon Romanovsky wrote:
> > > > On Thu, Jan 03, 2019 at 10:30:26PM +0000, Jason Gunthorpe wrote:
> > > > > On Thu, Jan 03, 2019 at 10:05:59AM +0200, Leon Romanovsky wrote:
> > > > > > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > >
> > > > > > XArray uses internal lock for updates to XArray. This means that our
> > > > > > external RW lock is not needed anymore and we can delete it.
> > > > >
> > > > > The xarray lock only protects the array itself - not the things it
> > > > > points to.
> > > > >
> > > > > > - down_read(&res->rwsem);
> > > > > > xa_for_each(&res->xa[type], e, index, ULONG_MAX, XA_PRESENT) {
> > > > > > if (ns == &init_pid_ns ||
> > > > > > (!rdma_is_kernel_res(e) &&
> > > > >
> > > > > So eg here, 'e' is still needs the locking to protect against
> > > > > concurrent restrack_del causing e to be freed.
> > > > >
> > > > > Most likely the internal xarray locking doesn't save anything in this
> > > > > usage..
> > > >
> > > > The "rwsem" protects hash lists insert/delete and we are still using
> > > > kref to protect specific entries. And those xarray insert/deletion are
> > > > handled by xa_lock() now.
> > >
> > > This doesn't explain where the locking for 'e' comes from above
> >
> > According to the documentation, xa_for_each takes RCU lock while
> > iterating, so I assume that "e" is safe once it returned from xarray,
>
> The documentation means it internally takes/releases the RCU lock (in
> contrast to radix tree that requires callers to hold the RCU lock to
> protect the internals of the radix tree).
>
> See the implementation, the loop body is run with RCU unlocked.
>
> The xarray interfaces only protect its insides, the caller is
> responsible to protect the returned data.
Thanks for the pointer.
Because XArray ensure that iteration is safe, we can grab kref on entry
to ensure that it is not released.
diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
index f687dffb042a..6db28e3d6c53 100644
--- a/drivers/infiniband/core/restrack.c
+++ b/drivers/infiniband/core/restrack.c
@@ -116,10 +116,15 @@ int rdma_restrack_count(struct rdma_restrack_root *res,
u32 cnt = 0;
xa_for_each(&res->xa[type], e, index, ULONG_MAX, XA_PRESENT) {
+ if (!rdma_restrack_get(e))
+ continue;
+
if (ns == &init_pid_ns ||
(!rdma_is_kernel_res(e) &&
ns == task_active_pid_ns(e->task)))
cnt++;
+
+ rdma_restrack_put(e);
}
return cnt;
}
>
> Jason
Attachment:
signature.asc
Description: PGP signature
