On Mon, Jan 14, 2019 at 08:43:18PM +0000, Jason Gunthorpe wrote:
> On Mon, Jan 14, 2019 at 04:18:24PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > Generate unique resource ID for SW and HW capable devices.
> > RES_VALID marker is introduced as a temporal measure till
> > all drivers are converted to use restrask IDs. After that,
> > it will be safe to remove RES_VISIBLE and expose all objects,
> > including internal ones.
> >
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/core/nldev.c | 2 +-
> > drivers/infiniband/core/restrack.c | 83 +++++++++++++++++++-----------
> > include/rdma/restrack.h | 19 +++++++
> > 3 files changed, 74 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
> > index 7fa64bd43c40..de3e90d6fb5a 100644
> > +++ b/drivers/infiniband/core/nldev.c
> > @@ -1121,7 +1121,7 @@ static int res_get_common_dumpit(struct sk_buff *skb,
> >
> > xa = rdma_dev_to_xa(device, res_type);
> > rdma_rt_read_lock(device, res_type);
> > - xa_for_each(xa, res, id, ULONG_MAX, XA_PRESENT) {
> > + xa_for_each(xa, res, id, ULONG_MAX, RES_VISIBLE) {
> > if (idx < start)
> > goto next;
> >
> > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > index a3c94fe910a2..ff1581347f66 100644
> > +++ b/drivers/infiniband/core/restrack.c
> > @@ -51,6 +51,7 @@ int rdma_restrack_init(struct ib_device *dev)
> > for (i = 0 ; i < RDMA_RESTRACK_MAX; i++) {
> > init_rwsem(&rt[i].rwsem);
> > xa_init_flags(&rt[i].xa, XA_FLAGS_ALLOC);
> > + rt[i].max = U32_MAX;
> > }
> >
> > return 0;
> > @@ -146,7 +147,7 @@ int rdma_restrack_count(struct ib_device *dev, enum rdma_restrack_type type,
> > u32 cnt = 0;
> >
> > rdma_rt_read_lock(dev, type);
> > - xa_for_each(xa, e, index, ULONG_MAX, XA_PRESENT) {
> > + xa_for_each(xa, e, index, ULONG_MAX, RES_VISIBLE) {
>
> visible is a good use for a mark..
>
> > -static void rdma_restrack_add(struct rdma_restrack_entry *res)
> > +int rdma_restrack_add(struct rdma_restrack_entry *res)
> > {
> > struct ib_device *dev = res_to_dev(res);
> > struct xarray *xa = rdma_dev_to_xa(dev, res->type);
> > + struct rdma_restrack_root *rt = dev->res;
> > int ret;
> >
> > + /*
> > + * Once all drivers are converted, we can remove this check
> > + * and remove call to rdma_restrack_add() from rdma_restrack_kadd()
> > + * and rdma_restrack_uadd()
> > + */
> > + if (xa_load(xa, res->id))
> > + return 0;
> > +
> > kref_init(&res->kref);
> > init_completion(&res->comp);
> > res->valid = true;
> >
> > - ret = xa_insert(xa, res_to_id(res), res, GFP_KERNEL);
> > - WARN_ONCE(ret == -EEXIST, "Tried to add non-unique type %d entry\n",
> > - res->type);
> > + if (rt[res->type].max) {
> > + /* Not HW-capable device */
> > + res->id = rt[res->type].reserved;
> > + ret = xa_alloc(xa, &res->id, rt[res->type].max, res,
> > + GFP_KERNEL);
>
> I feel like the SW ids should by allocated cyclicly?
It was my initial thought too, but after I saw that most of the drivers
are using find_first_bit()/find_first_zero_bit() in various bitmap,
I realized that adding complexity for cycling allocation doesn't worth it.
https://elixir.bootlin.com/linux/v5.0-rc2/source/drivers/infiniband/hw/bnxt_re/qplib_res.c#L567
https://elixir.bootlin.com/linux/v5.0-rc2/source/drivers/net/ethernet/qlogic/qed/qed_rdma.c#L83
>
> > void rdma_restrack_kadd(struct rdma_restrack_entry *res)
> > {
> > + struct ib_device *dev = res_to_dev(res);
> > + struct xarray *xa = rdma_dev_to_xa(dev, res->type);
> > +
> > res->task = NULL;
> > set_kern_name(res);
> > + /*
> > + * Temporaly, we are not intested in return value,
> > + * once conversion will be finished, it will be checked by drivers.
> > + */
> > rdma_restrack_add(res);
> > + xa_set_mark(xa, res->id, RES_VISIBLE);
>
> But if it failed to add we shouldn't set a mark on a entry that may
> not exist in the xarray.
I'll add, however I'm relying on WARN_ONCE in rdam_restrack_add() and my
hope that users won't see it, because drivers authors will fix those
places.
>
> Jason
Attachment:
signature.asc
Description: PGP signature
