On Fri, Jan 04, 2019 at 06:34:47PM +0000, Jason Gunthorpe wrote:
> On Fri, Jan 04, 2019 at 07:09:55PM +0200, Leon Romanovsky wrote:
> > On Fri, Jan 04, 2019 at 04:24:24PM +0000, Jason Gunthorpe wrote:
> > > On Fri, Jan 04, 2019 at 08:12:50AM +0200, Leon Romanovsky wrote:
> > > > On Thu, Jan 03, 2019 at 10:27:52PM +0000, Jason Gunthorpe wrote:
> > > > > On Thu, Jan 03, 2019 at 10:05:57AM +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 | 13 +++++++++++++
> > > > > > include/rdma/restrack.h | 11 +++++++++++
> > > > > > 2 files changed, 24 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > > > > > index 473422da0856..d65758a3a7b6 100644
> > > > > > +++ b/drivers/infiniband/core/restrack.c
> > > > > > @@ -249,6 +249,19 @@ int __must_check rdma_restrack_get(struct rdma_restrack_entry *res)
> > > > > > }
> > > > > > EXPORT_SYMBOL(rdma_restrack_get);
> > > > > >
> > > > > > +struct rdma_restrack_entry *
> > > > > > +rdma_restrack_get_byid(struct rdma_restrack_root *rt,
> > > > > > + enum rdma_restrack_type type, u32 id)
> > > > > > +{
> > > > > > + struct rdma_restrack_entry *res;
> > > > > > +
> > > > > > + res = xa_load(&rt->xa[type], id);
> > > > >
> > > > > the previous patch did:
> > > > >
> > > > > + id = res_to_id(res);
> > > > > + ret = xa_insert(&dev->res.xa[res->type], id, res, GFP_KERNEL);
> > > > >
> > > > > Where res_to_id() returns a pointer casted to an unsigned long - so
> > > > > the use of 'u32 id' is wrong here.
> > > >
> > > > I "removed" from this RFC patches which added software ID generation for
> > > > drivers without relevant HW/FW and their conversion to use that ID.
> > > >
> > > > At the time of final submission res_to_id() won't return pointers.
> > > >
> > > > >
> > > > > Also, it seems to expose kernel pointers to user space, which is a
> > > > > security no-no.
> > > >
> > > > We are not, see introduction of ".id" field in nldev.c. It is added once
> > > > all drivers are capable to return some unique ID and not pointer.
> > > >
> > > > >
> > > > > I think this needs actual device-unique IDs for all objects, no use of
> > > > > pointers.
> > > >
> > > > It exists, I didn't post it as part of RFC.
> > > >
> > > > >
> > > > > Probably better to use the xarray in the IDR mode and have it generate
> > > > > unique ID's for all the cases where a HW ID is not already present.
> > > >
> > > > I'm attaching this patch for your convenience.
> > > >
> > > > From c3cca57ebf246c268b60be9ad364a0275cedb1de Mon Sep 17 00:00:00 2001
> > > > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > Date: Thu, 3 Jan 2019 18:19:43 +0200
> > > > Subject: [PATCH 24/25] RDMA/restrack: Implement software ID interface
> > > >
> > > > Provide basic functionality to safely get and put software IDs
> > > > for devices which don't do it in HW.
> > > >
> > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > drivers/infiniband/core/restrack.c | 94 +++++++++++++++++++++++++++++-
> > > > include/rdma/restrack.h | 39 +++++++++++++
> > > > 2 files changed, 132 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > > > index bd3c37ca7829..f687dffb042a 100644
> > > > +++ b/drivers/infiniband/core/restrack.c
> > > > @@ -12,6 +12,24 @@
> > > >
> > > > #include "cma_priv.h"
> > > >
> > > > +struct sw_id {
> > > > + /**
> > > > + * @id: Bitmap to keep SW genrated IDs for devices which
> > > > + * don't have relevant hW/FW to generate them on their own.
> > > > + * It is used for types without other unique identification
> > > > + * available, for example QPs have unique QPN built-in.
> > > > + */
> > > > + unsigned long *bitmap;
> > >
> > > Why? Just use the xarray to generate IDs.
> >
> > I wanted, but didn't find proper API for that in include/linux/xarray.h.
>
> Use xa_alloc
I considered it, but it didn't play well with current IB/core code.
We are performing allocation of IDs in the begging of all ib_alloc_XXX()
functions, while adding to restrack DB after successful return from
those functions in central place, applicable for whole stack.
It means that calling to xa_alloc() to get new ID will cause to double
allocation/access to xarray.
More on that, in current code, we are not tracking driver objects (e.g.
DRIVER_QPT) and not presenting them while doing xa_for_each() passage.
I'm really fine to push rdma_restrack_add() to drivers, so we won't need
double xa alloc and we will track and present ALL objects, including EFA's QP.
The question is "should we do it?"
>
> > Also, we are talking about objects which anyway backed by HW and limited
> > in size, at least for PD, almost all drivers implemented it with
> > bitmap, so IDR was overkill for them.
>
> If the drivers have bitmaps they should just have restrack do it
> instead and delete the bitmaps (which should be ida anyhow), something
> like restrack_assign_obj_id()
Yes, it more or less how I did it.
>
> If the drivers get the ID from someplace else then they can just do a
> restrack_set_obj_id() or somesuch similar.
>
> Jason
Attachment:
signature.asc
Description: PGP signature
