On 5/11/2020 7:37 AM, Leon Romanovsky wrote:
On Sun, May 10, 2020 at 05:55:54PM +0300, Yamin Friedman wrote:
A pre-step for adding shared CQs. Add the infra-structure to prevent
shared CQ users from altering the CQ configurations. For now all cqs are
marked as private (non-shared). The core driver should use the new force
functions to perform resize/destroy/moderation changes that are not
allowed for users of shared CQs.
Signed-off-by: Yamin Friedman <yaminf@xxxxxxxxxxxx>
Reviewed-by: Or Gerlitz <ogerlitz@xxxxxxxxxxxx>
---
drivers/infiniband/core/cq.c | 25 ++++++++++++++++++-------
drivers/infiniband/core/verbs.c | 37 ++++++++++++++++++++++++++++++++++---
include/rdma/ib_verbs.h | 20 +++++++++++++++++++-
3 files changed, 71 insertions(+), 11 deletions(-)
infiniband/core -> RDMA/core
Will fix.
diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
index 4f25b24..443a9cd 100644
--- a/drivers/infiniband/core/cq.c
+++ b/drivers/infiniband/core/cq.c
@@ -37,6 +37,7 @@ static void ib_cq_rdma_dim_work(struct work_struct *w)
{
struct dim *dim = container_of(w, struct dim, work);
struct ib_cq *cq = dim->priv;
+ int ret;
u16 usec = rdma_dim_prof[dim->profile_ix].usec;
u16 comps = rdma_dim_prof[dim->profile_ix].comps;
@@ -44,7 +45,10 @@ static void ib_cq_rdma_dim_work(struct work_struct *w)
dim->state = DIM_START_MEASURE;
trace_cq_modify(cq, comps, usec);
- cq->device->ops.modify_cq(cq, comps, usec);
+ ret = rdma_set_cq_moderation_force(cq, comps, usec);
+ if (ret)
+ WARN_ONCE(1, "Failed set moderation for CQ 0x%p\n", cq);
First WARN_ONCE(ret, ...), second no to pointer address print and third
this dump stack won't help, because CQ moderation will fail for many
reasons unrelated to the caller.
Would it be better to not include any warning for failed calls?
+
}
static void rdma_dim_init(struct ib_cq *cq)
@@ -218,6 +222,7 @@ struct ib_cq *__ib_alloc_cq_user(struct ib_device *dev, void *private,
cq->cq_context = private;
cq->poll_ctx = poll_ctx;
atomic_set(&cq->usecnt, 0);
+ cq->cq_type = IB_CQ_PRIVATE;
I would say it should be opposite, default is not shared CQ and only
pool sets something specific to mark that it is shared.
cq->wc = kmalloc_array(IB_POLL_BATCH, sizeof(*cq->wc), GFP_KERNEL);
if (!cq->wc)
@@ -300,12 +305,7 @@ struct ib_cq *__ib_alloc_cq_any(struct ib_device *dev, void *private,
}
EXPORT_SYMBOL(__ib_alloc_cq_any);
-/**
- * ib_free_cq_user - free a completion queue
- * @cq: completion queue to free.
- * @udata: User data or NULL for kernel object
- */
-void ib_free_cq_user(struct ib_cq *cq, struct ib_udata *udata)
+static void _ib_free_cq_user(struct ib_cq *cq, struct ib_udata *udata)
{
if (WARN_ON_ONCE(atomic_read(&cq->usecnt)))
return;
@@ -333,4 +333,15 @@ void ib_free_cq_user(struct ib_cq *cq, struct ib_udata *udata)
kfree(cq->wc);
kfree(cq);
}
+
+/**
+ * ib_free_cq_user - free a completion queue
+ * @cq: completion queue to free.
+ * @udata: User data or NULL for kernel object
+ */
+void ib_free_cq_user(struct ib_cq *cq, struct ib_udata *udata)
+{
+ if (!WARN_ON_ONCE(cq->cq_type != IB_CQ_PRIVATE))
+ _ib_free_cq_user(cq, udata);
+}
It is not preferable kernel style - not on WARN_ON_ONCE() and do
something later.
Should I remove the warning completly?
EXPORT_SYMBOL(ib_free_cq_user);
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index bf0249f..39c012f 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1988,15 +1988,29 @@ struct ib_cq *__ib_create_cq(struct ib_device *device,
}
EXPORT_SYMBOL(__ib_create_cq);
-int rdma_set_cq_moderation(struct ib_cq *cq, u16 cq_count, u16 cq_period)
+static int _rdma_set_cq_moderation(struct ib_cq *cq, u16 cq_count,
+ u16 cq_period)
{
return cq->device->ops.modify_cq ?
cq->device->ops.modify_cq(cq, cq_count,
cq_period) : -EOPNOTSUPP;
}
+
+int rdma_set_cq_moderation(struct ib_cq *cq, u16 cq_count, u16 cq_period)
+{
+ if (WARN_ON_ONCE(cq->cq_type != IB_CQ_PRIVATE))
+ return -EOPNOTSUPP;
+ else
+ return _rdma_set_cq_moderation(cq, cq_count, cq_period);
+}
EXPORT_SYMBOL(rdma_set_cq_moderation);
-int ib_destroy_cq_user(struct ib_cq *cq, struct ib_udata *udata)
+int rdma_set_cq_moderation_force(struct ib_cq *cq, u16 cq_count, u16 cq_period)
+{
+ return _rdma_set_cq_moderation(cq, cq_count, cq_period);
+}
All these one liners makes no sense, the call to
_rdma_set_cq_moderation() in this function and above is exactly the
same. It means there is no need in specific function.
+
+static int _ib_destroy_cq_user(struct ib_cq *cq, struct ib_udata *udata)
{
if (atomic_read(&cq->usecnt))
return -EBUSY;
@@ -2004,15 +2018,32 @@ int ib_destroy_cq_user(struct ib_cq *cq, struct ib_udata *udata)
rdma_restrack_del(&cq->res);
cq->device->ops.destroy_cq(cq, udata);
kfree(cq);
+
Not relevant
Will fix.
return 0;
}
+
+int ib_destroy_cq_user(struct ib_cq *cq, struct ib_udata *udata)
+{
+ if (WARN_ON_ONCE(cq->cq_type != IB_CQ_PRIVATE))
+ return -EOPNOTSUPP;
+ else
+ return _ib_destroy_cq_user(cq, udata);
+}
EXPORT_SYMBOL(ib_destroy_cq_user);
I would expect symmetric API, you can call to create_cq_user for your
pool, but can't call to destroy_cq_user, am I right?
The only reason I changed destroy_cq_user is because it is something
that could be done on a shared cq without realizing. Only the core
driver can create shared cqs so there is no reason to change
create_cq_user as it still only provides private cqs.
-int ib_resize_cq(struct ib_cq *cq, int cqe)
+static int _ib_resize_cq(struct ib_cq *cq, int cqe)
{
return cq->device->ops.resize_cq ?
cq->device->ops.resize_cq(cq, cqe, NULL) : -EOPNOTSUPP;
}
+
+int ib_resize_cq(struct ib_cq *cq, int cqe)
+{
+ if (WARN_ON_ONCE(cq->cq_type != IB_CQ_PRIVATE))
+ return -EOPNOTSUPP;
+ else
+ return _ib_resize_cq(cq, cqe);
+}
EXPORT_SYMBOL(ib_resize_cq);
It is not kernel style and probably dump_stack is not needed too.
Will change.
/* Memory regions */
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 4c488ca..c889415 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1557,6 +1557,10 @@ enum ib_poll_context {
IB_POLL_UNBOUND_WORKQUEUE, /* poll from unbound workqueue */
};
+enum ib_cq_type {
+ IB_CQ_PRIVATE, /* CQ will be used by only one user */
+};
Do you see another CQ types? If not it should not be a type but boolean.
If yes, PRIVATE is not really type but property.
Makes sense.
+
struct ib_cq {
struct ib_device *device;
struct ib_ucq_object *uobject;
@@ -1582,6 +1586,7 @@ struct ib_cq {
* Implementation details of the RDMA core, don't use in drivers:
*/
struct rdma_restrack_entry res;
+ enum ib_cq_type cq_type;
};
struct ib_srq {
@@ -3832,6 +3837,7 @@ static inline struct ib_cq *ib_alloc_cq_any(struct ib_device *dev,
* @cq: The CQ to free
*
* NOTE: for user cq use ib_free_cq_user with valid udata!
+ * NOTE: this will fail for shared cqs
*/
static inline void ib_free_cq(struct ib_cq *cq)
{
@@ -3881,7 +3887,19 @@ struct ib_cq *__ib_create_cq(struct ib_device *device,
int rdma_set_cq_moderation(struct ib_cq *cq, u16 cq_count, u16 cq_period);
/**
- * ib_destroy_cq_user - Destroys the specified CQ.
+ * rdma_set_cq_moderation_force - Modifies moderation params of the CQ.
+ * Meant for use in core driver to work for shared CQs.
+ * @cq: The CQ to modify.
+ * @cq_count: number of CQEs that will trigger an event
+ * @cq_period: max period of time in usec before triggering an event
+ *
+ */
+int rdma_set_cq_moderation_force(struct ib_cq *cq, u16 cq_count,
+ u16 cq_period);
+
+/**
+ * ib_destroy_cq_user - Destroys the specified CQ. If the CQ is not
+ * PRIVATE this function will fail.
It is not only fail, but print huge dump_stack.
I will deal with the superfluous dumps.
* @cq: The CQ to destroy.
* @udata: Valid user data or NULL for kernel objects
*/
--
1.8.3.1
