On Tue, Feb 05, 2019 at 03:07:13PM -0700, Jason Gunthorpe wrote:
> On Sat, Feb 02, 2019 at 01:42:46PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > Add new general helper to get restrack entry given by ID and
> > their respective type.
> >
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/core/restrack.c | 22 ++++++++++++++++++++++
> > include/rdma/restrack.h | 3 +++
> > 2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > index ef5f16669262..b10b9c3cbb06 100644
> > +++ b/drivers/infiniband/core/restrack.c
> > @@ -256,6 +256,28 @@ int __must_check rdma_restrack_get(struct rdma_restrack_entry *res)
> > }
> > EXPORT_SYMBOL(rdma_restrack_get);
> >
> > +/**
> > + * rdma_restrack_get_byid() - translate from ID to restrack object
> > + * @dev: IB device
> > + * @type: resource track type
> > + * @id: ID to take a look
> > + *
> > + * Return: Pointer to restrack entry or -ENOENT in case of error.
> > + */
> > +struct rdma_restrack_entry *
> > +rdma_restrack_get_byid(struct ib_device *dev,
> > + enum rdma_restrack_type type, u32 id)
> > +{
> > + struct rdma_restrack_root *rt = &dev->res;
> > + struct rdma_restrack_entry *res;
> > +
> > + res = xa_load(&rt->xa[type], id);
> > + if (!res || !rdma_restrack_get(res))
> > + return ERR_PTR(-ENOENT);
> > + return res;
>
> This is missing holding the read side of the rwsem
>
> And the order of xa_erase should be changed:
Agree, thanks
>
> void rdma_restrack_del(struct rdma_restrack_entry *res)
> {
> [..]
>
> down_write(&rt->rwsem);
> xa_erase(xa, res->id);
> up_write(&rt->rwsem);
>
> rdma_restrack_put(res);
>
> wait_for_completion(&res->comp);
>
> I'd also get rid of valid during the xarray conversion.
>
> The only place that sets it to true is after rt_xa_alloc_cyclic, so
> write it as:
>
> down_write(&rt->rwsem);
> add_has_been_done = xa_cmpxchg(xa, res->id, res, NULL, GFP_KERNEL) == res;
> up_write(&rt->rwsem);
>
> Ie if the entry was stored in it's own ID in the xarray then it is
> 'valid'.
what is the advantage over simple if (!ret) check?
>
> Jason
Attachment:
signature.asc
Description: PGP signature
