Re: [PATCH rdma-next v4 2/9] RDMA/restrack: Translate from ID to restrack object

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux