On Tue, Jan 15, 2019 at 09:12:53PM +0200, Leon Romanovsky wrote:
> On Tue, Jan 15, 2019 at 08:17:37PM +0200, Leon Romanovsky wrote:
> > On Tue, Jan 15, 2019 at 09:53:13AM -0700, Jason Gunthorpe wrote:
> > > On Tue, Jan 15, 2019 at 12:56:10PM +0200, Leon Romanovsky wrote:
> > > > > > - 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.
> > >
> > > Those drivers didn't contemplate exposing these numbers of net link to
> > > an agent that isn't part of the create/destroy cycle.
> > >
> > > Using cyclic is the best way to avoid various races and confusion for
> > > the netlink side, and it is straightforward to do.
> >
> > No problem, I'll change it and will drop that patch with "user" field.
>
> Actually after thinking more, in order to implement this cyclic
> allocator, I will need to store next index and update it after every
> xa_alloc call and it means that we will need to add lock to maintain
> that index.
>
> Are you sure that we really need it?
>
It will be something like that:
diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
index 58a60730e835..262e6499d40d 100644
--- a/drivers/infiniband/core/restrack.c
+++ b/drivers/infiniband/core/restrack.c
@@ -10,6 +10,8 @@
#include <linux/sched/task.h>
#include <linux/pid_namespace.h>
#include <linux/rwsem.h>
+#include <linux/spinlock.h>
+#include <linux/overflow.h>
#include "cma_priv.h"
@@ -35,6 +37,14 @@ struct rdma_restrack_root {
* @max: Maximal supported number of indexes
*/
u32 max;
+ /**
+ * @next: Next ID to support cyclic allocation
+ */
+ u32 next_id;
+ /**
+ * @next_lock: Lock to protect next-ID generation
+ */
+ spinlock_t next_lock;
};
/**
@@ -58,6 +68,7 @@ int rdma_restrack_init(struct ib_device *dev)
init_rwsem(&rt[i].rwsem);
xa_init_flags(&rt[i].xa, XA_FLAGS_ALLOC);
rt[i].max = U32_MAX;
+ spin_lock_init(&rt[i].next_lock);
}
return 0;
@@ -282,14 +293,28 @@ int rdma_restrack_add(struct rdma_restrack_entry *res)
res->valid = true;
if (rt[res->type].max) {
+ u32 a = 1, b;
+
/* Not HW-capable device */
- res->id = rt[res->type].reserved;
+ spin_lock(&rt[res->type].next_lock);
+ res->id = rt[res->type].next_id;
ret = xa_alloc(xa, &res->id, rt[res->type].max, res,
GFP_KERNEL);
+ if (ret) {
+ spin_unlock(&rt[res->type].next_lock);
+ goto out;
+ }
+
+ if (check_add_overflow(res->id, a, &b))
+ rt[res->type].next_id = rt[res->type].reserved;
+ else
+ rt[res->type].next_id = res->id + 1;
+ spin_unlock(&rt[res->type].next_lock);
} else {
ret = xa_insert(xa, res->id, res, GFP_KERNEL);
}
- /*
+
+out: /*
* WARNs below indicates an error in driver code.
* The check of -EEXIST is never occuried and added here
* to allow simple removal of xa_load above.
@@ -445,6 +470,7 @@ void rdma_rt_set_id_range(struct ib_device *dev, enum rdma_restrack_type type,
rt[type].max = max;
rt[type].reserved = reserved;
+ rt[type].next_id = reserved;
}
EXPORT_SYMBOL(rdma_rt_set_id_range);
>
> >
> > >
> > > Jason
>
>
Attachment:
signature.asc
Description: PGP signature
