diff mbox

[V1,for-next,3/7] IB/core: Add idr based standard types

Message ID 1485952745-58476-4-git-send-email-matanb@mellanox.com (mailing list archive)
State Superseded
Headers show

Commit Message

Matan Barak Feb. 1, 2017, 12:39 p.m. UTC
This patch adds the standard idr based types. These types are
used in downstream patches in order to initialize, destroy and
lookup IB standard objects which are based on idr objects.

An idr object requires filling out several parameters. Its op pointer
should point to uverbs_idr_ops and its size should be at least the
size of ib_uobject. We add a macro to make the type declaration easier.

Signed-off-by: Matan Barak <matanb@mellanox.com>
Reviewed-by: Yishai Hadas <yishaih@mellanox.com>
---
 drivers/infiniband/core/Makefile           |   2 +-
 drivers/infiniband/core/uverbs.h           |   2 +
 drivers/infiniband/core/uverbs_main.c      |   4 +-
 drivers/infiniband/core/uverbs_std_types.c | 165 +++++++++++++++++++++++++++++
 include/rdma/uverbs_std_types.h            |  50 +++++++++
 include/rdma/uverbs_types.h                |  16 +++
 6 files changed, 236 insertions(+), 3 deletions(-)
 create mode 100644 drivers/infiniband/core/uverbs_std_types.c
 create mode 100644 include/rdma/uverbs_std_types.h

Comments

Jason Gunthorpe Feb. 10, 2017, 8:08 p.m. UTC | #1
On Wed, Feb 01, 2017 at 02:39:01PM +0200, Matan Barak wrote:

> +void uverbs_free_qp(struct ib_uobject *uobject)

This should be static...

Based on the last email I expect this to be

static int uverbs_destroy_qp(struct ib_uobject *uobject, enum rdma_remove_reason why)

> +{
> +	struct ib_qp *qp = uobject->object;
> +	struct ib_uqp_object *uqp =
> +		container_of(uobject, struct ib_uqp_object, uevent.uobject);
> +
> +	if (qp == qp->real_qp)
> +		ib_uverbs_detach_umcast(qp, uqp);
> +	ib_destroy_qp(qp);

This silently eats the error code from ib_destroy_qp, not OK. See
discussion in prior email. Lots of cases of this.

> +const struct uverbs_obj_idr_type uverbs_type_attrs_cq =
> +	UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0,
> +				 uverbs_free_cq);
>

It would be more readable if the struct decl was near the functions in
blocks.

Still don't like these macros. The open coded version would look like this:

const struct uverbs_obj_idr_type uverbs_type_attrs_cq = {
      .type = {
      	    .obj_size = sizeof(struct ib_ucq_object),
      	    .ops = &uverbs_idr_ops,
      },
      .destroy = uverbs_destroy_cq,
};

Whichs is more understandable and typical of the kernel style. If we
wish to add a new callback function for instance we don't need a new
macro or big churn.

You could do this I guess:

const struct uverbs_obj_idr_type uverbs_type_attrs_cq = {
      .type = UVERBS_IDR_TYPE(cq, 0),
      .destroy = uverbs_destroy_cq,
};

> +extern const struct uverbs_obj_type_ops uverbs_idr_ops;
> +
> +#define UVERBS_BUILD_BUG_ON(cond) (sizeof(char[1 - 2 * !!(cond)]) -	\
> +				   sizeof(char))

This probably doesn't add value, but you could make this a WARN_ON in
alloc_uobj().

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matan Barak Feb. 15, 2017, 1:44 p.m. UTC | #2
On Fri, Feb 10, 2017 at 10:08 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Feb 01, 2017 at 02:39:01PM +0200, Matan Barak wrote:
>
>> +void uverbs_free_qp(struct ib_uobject *uobject)
>
> This should be static...
>

Yeah

> Based on the last email I expect this to be
>
> static int uverbs_destroy_qp(struct ib_uobject *uobject, enum rdma_remove_reason why)
>

Since we don't care about the reason here and it's called from the
destroy callback,
we could leave the deceleration as is.

>> +{
>> +     struct ib_qp *qp = uobject->object;
>> +     struct ib_uqp_object *uqp =
>> +             container_of(uobject, struct ib_uqp_object, uevent.uobject);
>> +
>> +     if (qp == qp->real_qp)
>> +             ib_uverbs_detach_umcast(qp, uqp);
>> +     ib_destroy_qp(qp);
>
> This silently eats the error code from ib_destroy_qp, not OK. See
> discussion in prior email. Lots of cases of this.
>

I can add a WARN_ON, but this is essentially the same as ib_destroy_qp
fails when a context is destroyed (either because of hot unplug or process
termination). You won't get here because of regular destroy handler.
This is the current behavior as well.

>> +const struct uverbs_obj_idr_type uverbs_type_attrs_cq =
>> +     UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0,
>> +                              uverbs_free_cq);
>>
>
> It would be more readable if the struct decl was near the functions in
> blocks.
>

But then it can't be used by the hardware drivers to declare new types.

> Still don't like these macros. The open coded version would look like this:
>
> const struct uverbs_obj_idr_type uverbs_type_attrs_cq = {
>       .type = {
>             .obj_size = sizeof(struct ib_ucq_object),
>             .ops = &uverbs_idr_ops,
>       },
>       .destroy = uverbs_destroy_cq,
> };
>
> Whichs is more understandable and typical of the kernel style. If we
> wish to add a new callback function for instance we don't need a new
> macro or big churn.
>
> You could do this I guess:
>
> const struct uverbs_obj_idr_type uverbs_type_attrs_cq = {
>       .type = UVERBS_IDR_TYPE(cq, 0),
>       .destroy = uverbs_destroy_cq,
> };
>

Yeah, seems reasonable - a good trade off.

>> +extern const struct uverbs_obj_type_ops uverbs_idr_ops;
>> +
>> +#define UVERBS_BUILD_BUG_ON(cond) (sizeof(char[1 - 2 * !!(cond)]) -  \
>> +                                sizeof(char))
>
> This probably doesn't add value, but you could make this a WARN_ON in
> alloc_uobj().
>

Why should we calculate this on runtime if we could know this in compile time?

> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Feb. 15, 2017, 6:09 p.m. UTC | #3
On Wed, Feb 15, 2017 at 03:44:22PM +0200, Matan Barak wrote:

> > static int uverbs_destroy_qp(struct ib_uobject *uobject, enum rdma_remove_reason why)
> >
> 
> Since we don't care about the reason here and it's called from the
> destroy callback,
> we could leave the deceleration as is.

The point is to ultimately push the reason down to the driver so the
driver can do the proper action. Eg force-destroy a HCA object.

> I can add a WARN_ON, but this is essentially the same as ib_destroy_qp
> fails when a context is destroyed (either because of hot unplug or process
> termination). You won't get here because of regular destroy handler.
> This is the current behavior as well.

I don't think the current behavior is very good. We've had driver
submissions that include random failures inside their destroy routines
- the current situation is a mess.
 
> Why should we calculate this on runtime if we could know this in compile time?

Because then you need the ugly macro :)

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/infiniband/core/Makefile b/drivers/infiniband/core/Makefile
index 1819623..c336d9c 100644
--- a/drivers/infiniband/core/Makefile
+++ b/drivers/infiniband/core/Makefile
@@ -29,4 +29,4 @@  ib_umad-y :=			user_mad.o
 ib_ucm-y :=			ucm.o
 
 ib_uverbs-y :=			uverbs_main.o uverbs_cmd.o uverbs_marshall.o \
-				rdma_core.o
+				rdma_core.o uverbs_std_types.o
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 662a592..dd8048f 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -204,6 +204,8 @@  void ib_uverbs_event_handler(struct ib_event_handler *handler,
 void ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev, struct ib_xrcd *xrcd);
 
 int uverbs_dealloc_mw(struct ib_mw *mw);
+void ib_uverbs_detach_umcast(struct ib_qp *qp,
+			     struct ib_uqp_object *uobj);
 
 struct ib_uverbs_flow_spec {
 	union {
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 8344afd..ba8b1eb 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -200,8 +200,8 @@  void ib_uverbs_release_uevent(struct ib_uverbs_file *file,
 	spin_unlock_irq(&file->async_file->lock);
 }
 
-static void ib_uverbs_detach_umcast(struct ib_qp *qp,
-				    struct ib_uqp_object *uobj)
+void ib_uverbs_detach_umcast(struct ib_qp *qp,
+			     struct ib_uqp_object *uobj)
 {
 	struct ib_uverbs_mcast_entry *mcast, *tmp;
 
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
new file mode 100644
index 0000000..9bbc4eb
--- /dev/null
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -0,0 +1,165 @@ 
+/*
+ * Copyright (c) 2017, Mellanox Technologies inc.  All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <rdma/uverbs_std_types.h>
+#include <rdma/ib_user_verbs.h>
+#include <rdma/ib_verbs.h>
+#include <linux/bug.h>
+#include <linux/file.h>
+#include "rdma_core.h"
+#include "uverbs.h"
+
+void uverbs_free_ah(struct ib_uobject *uobject)
+{
+	ib_destroy_ah((struct ib_ah *)uobject->object);
+}
+
+void uverbs_free_flow(struct ib_uobject *uobject)
+{
+	ib_destroy_flow((struct ib_flow *)uobject->object);
+}
+
+void uverbs_free_mw(struct ib_uobject *uobject)
+{
+	uverbs_dealloc_mw((struct ib_mw *)uobject->object);
+}
+
+void uverbs_free_qp(struct ib_uobject *uobject)
+{
+	struct ib_qp *qp = uobject->object;
+	struct ib_uqp_object *uqp =
+		container_of(uobject, struct ib_uqp_object, uevent.uobject);
+
+	if (qp == qp->real_qp)
+		ib_uverbs_detach_umcast(qp, uqp);
+	ib_destroy_qp(qp);
+	ib_uverbs_release_uevent(uobject->context->ufile, &uqp->uevent);
+}
+
+void uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject)
+{
+	struct ib_rwq_ind_table *rwq_ind_tbl = uobject->object;
+	struct ib_wq **ind_tbl = rwq_ind_tbl->ind_tbl;
+
+	ib_destroy_rwq_ind_table(rwq_ind_tbl);
+	kfree(ind_tbl);
+}
+
+void uverbs_free_wq(struct ib_uobject *uobject)
+{
+	struct ib_wq *wq = uobject->object;
+	struct ib_uwq_object *uwq =
+		container_of(uobject, struct ib_uwq_object, uevent.uobject);
+
+	ib_destroy_wq(wq);
+	ib_uverbs_release_uevent(uobject->context->ufile, &uwq->uevent);
+}
+
+void uverbs_free_srq(struct ib_uobject *uobject)
+{
+	struct ib_srq *srq = uobject->object;
+	struct ib_uevent_object *uevent =
+		container_of(uobject, struct ib_uevent_object, uobject);
+
+	ib_destroy_srq(srq);
+	ib_uverbs_release_uevent(uobject->context->ufile, uevent);
+}
+
+void uverbs_free_cq(struct ib_uobject *uobject)
+{
+	struct ib_cq *cq = uobject->object;
+	struct ib_uverbs_event_file *ev_file = cq->cq_context;
+	struct ib_ucq_object *ucq =
+		container_of(uobject, struct ib_ucq_object, uobject);
+
+	ib_destroy_cq(cq);
+	ib_uverbs_release_ucq(uobject->context->ufile, ev_file, ucq);
+}
+
+void uverbs_free_mr(struct ib_uobject *uobject)
+{
+	ib_dereg_mr((struct ib_mr *)uobject->object);
+}
+
+void uverbs_free_xrcd(struct ib_uobject *uobject)
+{
+	struct ib_xrcd *xrcd = uobject->object;
+
+	mutex_lock(&uobject->context->ufile->device->xrcd_tree_mutex);
+	ib_uverbs_dealloc_xrcd(uobject->context->ufile->device, xrcd);
+	mutex_unlock(&uobject->context->ufile->device->xrcd_tree_mutex);
+}
+
+void uverbs_free_pd(struct ib_uobject *uobject)
+{
+	ib_dealloc_pd((struct ib_pd *)uobject->object);
+}
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_cq =
+	UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0,
+				 uverbs_free_cq);
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_qp =
+	UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uqp_object), 0,
+				 uverbs_free_qp);
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_mw =
+	UVERBS_TYPE_ALLOC_IDR(0, uverbs_free_mw);
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_mr =
+	/* 1 is used in order to free the MR after all the MWs */
+	UVERBS_TYPE_ALLOC_IDR(1, uverbs_free_mr);
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_srq =
+	UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_usrq_object), 0,
+				 uverbs_free_srq);
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_ah =
+	UVERBS_TYPE_ALLOC_IDR(0, uverbs_free_ah);
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_flow =
+	UVERBS_TYPE_ALLOC_IDR(0, uverbs_free_flow);
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_wq =
+	UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uwq_object), 0,
+				 uverbs_free_wq);
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_rwq_ind_table =
+	UVERBS_TYPE_ALLOC_IDR(0, uverbs_free_rwq_ind_tbl);
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_xrcd =
+	UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uxrcd_object), 0,
+				 uverbs_free_xrcd);
+
+const struct uverbs_obj_idr_type uverbs_type_attrs_pd =
+	/* 2 is used in order to free the PD after MRs */
+	UVERBS_TYPE_ALLOC_IDR(2, uverbs_free_pd);
diff --git a/include/rdma/uverbs_std_types.h b/include/rdma/uverbs_std_types.h
new file mode 100644
index 0000000..2edb776
--- /dev/null
+++ b/include/rdma/uverbs_std_types.h
@@ -0,0 +1,50 @@ 
+/*
+ * Copyright (c) 2017, Mellanox Technologies inc.  All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef _UVERBS_STD_TYPES__
+#define _UVERBS_STD_TYPES__
+
+#include <rdma/uverbs_types.h>
+
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_cq;
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_qp;
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_rwq_ind_table;
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_wq;
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_srq;
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_ah;
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_flow;
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_mr;
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_mw;
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_pd;
+extern const struct uverbs_obj_idr_type uverbs_type_attrs_xrcd;
+#endif
+
diff --git a/include/rdma/uverbs_types.h b/include/rdma/uverbs_types.h
index bb263f0..cdbf352 100644
--- a/include/rdma/uverbs_types.h
+++ b/include/rdma/uverbs_types.h
@@ -101,4 +101,20 @@  struct uverbs_obj_idr_type {
 	void (*hot_unplug)(struct ib_uobject *uobj);
 };
 
+extern const struct uverbs_obj_type_ops uverbs_idr_ops;
+
+#define UVERBS_BUILD_BUG_ON(cond) (sizeof(char[1 - 2 * !!(cond)]) -	\
+				   sizeof(char))
+#define UVERBS_TYPE_ALLOC_IDR_SZ(_size, _order, _hot_unplug)		\
+	 {.type = {							\
+		.destroy_order = _order,				\
+		.ops = &uverbs_idr_ops,					\
+	 },								\
+	 .hot_unplug = _hot_unplug,					\
+	 .obj_size = (_size) +						\
+		UVERBS_BUILD_BUG_ON((_size) < sizeof(struct		\
+						     ib_uobject))}
+#define UVERBS_TYPE_ALLOC_IDR(_order, _hot_unplug)			\
+	 UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_uobject), _order,	\
+				  _hot_unplug)
 #endif