[PATCH RFC rdma-core 0/5] Add thread domain support

[PATCH RFC rdma-core 0/5] Add thread domain support

[Yishai Hadas]

This RFC series is based on some initial discussion that was in the mailing
list to support resource domain.

The series includes detailed man pages of the suggested APIs and initial
patches in both verbs and mlx5 driver to demonstrate the expected usage.

The motivation behind the series is to allow an application finer grain control
over share hardware resources and locks when the resources are used by the same
thread.

Yishai

Yishai Hadas (5):
  verbs: Introduce thread domain and its related verbs
  verbs: Introduce parent domain and its related verbs
  mlx5: Add support for ibv_td object and its related verbs
  mlx5: Add support for ibv_parent domain and its related verbs
  mlx5: Handles QP creation with a given parent domain

 libibverbs/man/ibv_alloc_parent_domain.3 |  73 +++++++++++++++++++++
 libibverbs/man/ibv_alloc_td.3            |  52 +++++++++++++++
 libibverbs/verbs.h                       |  75 ++++++++++++++++++++++
 providers/mlx5/mlx5-abi.h                |   3 +-
 providers/mlx5/mlx5.c                    |   4 ++
 providers/mlx5/mlx5.h                    |  22 +++++++
 providers/mlx5/verbs.c                   | 107 +++++++++++++++++++++++++++++--
 7 files changed, 331 insertions(+), 5 deletions(-)
 create mode 100644 libibverbs/man/ibv_alloc_parent_domain.3
 create mode 100755 libibverbs/man/ibv_alloc_td.3

-- 
1.8.3.1

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

[PATCH RFC rdma-core 1/5] verbs: Introduce thread domain and its related verbs

[Yishai Hadas]

The ibv_thread_domain is a user space object that can be used by an
application to mark that few verbs resources will be used from single
thread.

As a result the driver can share some hardware resources for those
resources and can drop few locks when it accesses them.

This patch introduces this object and its related verbs, more details about
the expected usage are described as part of the man page of this commit.

Signed-off-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 libibverbs/man/ibv_alloc_td.3 | 52 +++++++++++++++++++++++++++++++++++++++++++
 libibverbs/verbs.h            | 35 +++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)
 create mode 100755 libibverbs/man/ibv_alloc_td.3

diff --git a/libibverbs/man/ibv_alloc_td.3 b/libibverbs/man/ibv_alloc_td.3
new file mode 100755
index 0000000..8fc2eef
--- /dev/null
+++ b/libibverbs/man/ibv_alloc_td.3
@@ -0,0 +1,52 @@
+.\" -*- nroff -*-
+.\" Licensed under the OpenIB.org BSD license (FreeBSD Variant) - See COPYING.md
+.\"
+.TH IBV_ALLOC_TD 3 2017-11-06 libibverbs "Libibverbs Programmer's Manual"
+.SH "NAME"
+ibv_alloc_td(), ibv_dealloc_td() \- allocate and deallocate thread domain object
+.SH "SYNOPSIS"
+.nf
+.B #include <infiniband/verbs.h>
+.sp
+.BI "struct ibv_td *ibv_alloc_td(struct ibv_context " "*context");
+.sp
+.BI "int ibv_dealloc_td(struct ibv_td " "*td");
+.fi
+.SH "DESCRIPTION"
+.B ibv_alloc_td()
+allocates a thread domain object for the RDMA device context
+.I context\fR.
+.sp
+The thread domain object defines how the verbs libraries will use locks and additional HW mapping to achieve best performance for handling multi-thread or single-thread protection.
+An application defines the serialization thread accesses scope by choosing which thread domain object will be used when creating verbs resources.
+.sp
+All verbs resources created with a unique thread domain object should be accessed in a serialized sequence by no more than single thread at a time.
+.sp
+Using verbs resources without a thread domain object means application can't commit to any threading access serialization model and the verbs library has to provide full thread safe access.
+.sp
+A
+.I struct ibv_td
+can be added to a parent domain via
+.B ibv_alloc_parent_domain()\fR.
+.sp
+.B ibv_dealloc_td()
+will deallocate the thread domain
+.I td\fR.
+All resources created with the
+.I td
+should be destroyed prior to deallocating the
+.I td\fR.
+.SH "RETURN VALUE"
+.B ibv_alloc_td()
+returns a pointer to the allocated struct
+.I ibv_td
+object, or NULL if the request fails (and sets errno to indicates the failure reason).
+.sp
+.B ibv_dealloc_td()
+returns 0 on success, or the value of errno on failure (which indicates the failure reason).
+.SH "SEE ALSO"
+.BR ibv_alloc_parent_domain (3),
+.SH "AUTHORS"
+.TP
+Alex Rosenbaum <rosenbaumalex-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+
diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
index 6465aa9..c6c536f 100644
--- a/libibverbs/verbs.h
+++ b/libibverbs/verbs.h
@@ -549,6 +549,10 @@ struct ibv_pd {
 	uint32_t		handle;
 };
 
+struct ibv_td {
+	struct ibv_context     *context;
+};
+
 enum ibv_xrcd_init_attr_mask {
 	IBV_XRCD_INIT_ATTR_FD	    = 1 << 0,
 	IBV_XRCD_INIT_ATTR_OFLAGS   = 1 << 1,
@@ -1637,6 +1641,8 @@ enum verbs_context_mask {
 
 struct verbs_context {
 	/*  "grows up" - new fields go here */
+	int (*dealloc_td)(struct ibv_td *td);
+	struct ibv_td *(*alloc_td)(struct ibv_context *context);
 	int (*post_srq_ops)(struct ibv_srq *srq,
 			    struct ibv_ops_wr *op,
 			    struct ibv_ops_wr **bad_op);
@@ -2177,6 +2183,35 @@ ibv_create_qp_ex(struct ibv_context *context, struct ibv_qp_init_attr_ex *qp_ini
 }
 
 /**
+ * ibv_alloc_td - Allocate a thread domain
+ */
+static inline struct ibv_td *ibv_alloc_td(struct ibv_context *context)
+{
+	struct verbs_context *vctx;
+
+	vctx = verbs_get_ctx_op(context, alloc_td);
+	if (!vctx) {
+		errno = ENOSYS;
+		return NULL;
+	}
+	return vctx->alloc_td(context);
+}
+
+/**
+ * ibv_dealloc_td - Free a thread domain
+ */
+static inline int ibv_dealloc_td(struct ibv_td *td)
+{
+	struct verbs_context *vctx;
+
+	vctx = verbs_get_ctx_op(td->context, dealloc_td);
+	if (!vctx)
+		return ENOSYS;
+
+	return vctx->dealloc_td(td);
+}
+
+/**
  * ibv_query_rt_values_ex - Get current real time @values of a device.
  * @values - in/out - defines the attributes we need to query/queried.
  * (Or's bits of enum ibv_values_mask on values->comp_mask field)
-- 
1.8.3.1

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

[PATCH RFC rdma-core 2/5] verbs: Introduce parent domain and its related verbs

[Yishai Hadas]

This patch introduces the ibv_parent_domain user space object and its
related create and destroy verbs.

This object may include few verbs domains as of protection domain (i.e.
ibv_pd), thread domain (i.e. ibv_td) and in the future some others as of
loopback domain etc.

The main purpose of introducing it, is to enable an application setting
the required domains in one object and pass them all at once upon
resource creation.

To prevent the need to change/extend current verbs APIs this parent
object is defined as ibv_pd outside the libraries so that verbs that
already get a PD can now get this object without any change.

It's the responsibility of the driver to wrap it internally as already
done today for other objects (e.g. QP, SRQ) and have some indication
whether the given ibv_pd is some legacy object or this is the new parent
object which includes some other domains.

This with the ibv_td object that was introduced in the previous patch
will enable driver that will implement them to share hardware resources
and drop locks for verbs objects that will be created with a parent
domain with same thread domain.

Extra details appeared in the man page which is part of this patch.

Signed-off-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 libibverbs/man/ibv_alloc_parent_domain.3 | 73 ++++++++++++++++++++++++++++++++
 libibverbs/verbs.h                       | 40 +++++++++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 libibverbs/man/ibv_alloc_parent_domain.3

diff --git a/libibverbs/man/ibv_alloc_parent_domain.3 b/libibverbs/man/ibv_alloc_parent_domain.3
new file mode 100644
index 0000000..45287f8
--- /dev/null
+++ b/libibverbs/man/ibv_alloc_parent_domain.3
@@ -0,0 +1,73 @@
+.\" -*- nroff -*-
+.\" Licensed under the OpenIB.org BSD license (FreeBSD Variant) - See COPYING.md
+.\"
+.TH IBV_ALLOC_PARENT_DOMAIN 3 2017-11-06 libibverbs "Libibverbs Programmer's Manual"
+.SH "NAME"
+ibv_alloc_parent_domain(), ibv_dealloc_parent_domain() \- allocate and deallocate the parent domain object
+.SH "SYNOPSIS"
+.nf
+.B #include <infiniband/verbs.h>
+.sp
+.BI "struct ibv_pd *ibv_alloc_parent_domain(struct ibv_context "*context" ", struct ibv_domain_init_attr " "*attr");
+.sp
+.BI "int ibv_dealloc_parent_domain(struct ibv_pd " "*pd");
+.fi
+.SH "DESCRIPTION"
+.B ibv_alloc_parent_domain()
+allocates a parent domain object for the RDMA device context
+.I context\fR.
+.sp
+The parent domain object is a collection of specific domain objects of different types, e.g. protection domain and thread domain.
+.sp
+A parent domain can be used as input parameter interchangeable where protection domain (ibv_pd) is currently used.
+Note that
+.I struct ibv_pd
+acts as handle for legacy protection domain and the new parent domain.
+.sp
+The
+.I attr
+argument specifies the following:
+.PP
+.nf
+struct ibv_parent_domain_init_attr {
+.in +8
+struct ibv_pd *pd; /* referance to a protection domain, or NULL */
+struct ibv_td *td; /* referance to a thread domain, or NULL */
+uint32_t comp_mask;
+.in -8
+};
+.fi
+.PP
+.sp
+Creating a parent domain object has to include at least one specific type domain object:
+.I pd\fR,
+.I td\fR,
+or both.
+.sp
+When creating a QP, SRQ, WQ, the legacy or new ibv_pd can be used, depending on the scope of domains required to be covered. When using the parent domain, it must include a reference to a protection domain else the verbs object creation will fail.
+.sp
+When creating a CQ, only the parent domain object will be a valid input. The protection domain value is disregarded in the CQ creation.
+.sp
+.B ibv_dealloc_parent_domain()
+will deallocate the parent domain
+.I pd\fR.
+All resources created with the
+.I pd
+should be destroyed prior to deallocating the
+.I pd\fR.
+.SH "RETURN VALUE"
+.B ibv_alloc_parent_domain()
+returns a pointer to the allocated struct
+.I ibv_pd
+object, or NULL if the request fails (and sets errno to indicates the failure reason).
+.sp
+.B ibv_dealloc_parent_domain()
+returns 0 on success, or the value of errno on failure (which indicates the failure reason).
+.SH "SEE ALSO"
+.BR ibv_alloc_parent_domain (3),
+.BR ibv_alloc_pd (3),
+.BR ibv_alloc_td (3),
+.SH "AUTHORS"
+.TP
+Alex Rosenbaum <rosenbaumalex-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+
diff --git a/libibverbs/verbs.h b/libibverbs/verbs.h
index c6c536f..db88705 100644
--- a/libibverbs/verbs.h
+++ b/libibverbs/verbs.h
@@ -1620,6 +1620,12 @@ struct ibv_cq_init_attr_ex {
 	uint32_t		flags;
 };
 
+struct ibv_parent_domain_init_attr {
+	struct ibv_pd *pd; /* referance to a protection domain object, or NULL */
+	struct ibv_td *td; /* referance to a thread domain object, or NULL */
+	uint32_t comp_mask;
+};
+
 enum ibv_values_mask {
 	IBV_VALUES_MASK_RAW_CLOCK	= 1 << 0,
 	IBV_VALUES_MASK_RESERVED	= 1 << 1
@@ -1641,6 +1647,9 @@ enum verbs_context_mask {
 
 struct verbs_context {
 	/*  "grows up" - new fields go here */
+	int (*dealloc_parent_domain)(struct ibv_pd *domain);
+	struct ibv_pd *(*alloc_parent_domain)(struct ibv_context *context,
+					       struct ibv_parent_domain_init_attr *attr);
 	int (*dealloc_td)(struct ibv_td *td);
 	struct ibv_td *(*alloc_td)(struct ibv_context *context);
 	int (*post_srq_ops)(struct ibv_srq *srq,
@@ -2212,6 +2221,37 @@ static inline int ibv_dealloc_td(struct ibv_td *td)
 }
 
 /**
+ * ibv_alloc_parent_domain - Allocate a parent domain
+ */
+static inline struct ibv_pd *ibv_alloc_parent_domain(struct ibv_context *context,
+						struct ibv_parent_domain_init_attr *attr)
+{
+	struct verbs_context *vctx;
+
+	vctx = verbs_get_ctx_op(context, alloc_parent_domain);
+	if (!vctx) {
+		errno = ENOSYS;
+		return NULL;
+	}
+
+	return vctx->alloc_parent_domain(context, attr);
+}
+
+/**
+ * ibv_dealloc_parent_domain - Free a parent domain
+ */
+static inline int ibv_dealloc_parent_domain(struct ibv_pd *domain)
+{
+	struct verbs_context *vctx;
+
+	vctx = verbs_get_ctx_op(domain->context, dealloc_parent_domain);
+	if (!vctx)
+		return ENOSYS;
+
+	return vctx->dealloc_parent_domain(domain);
+}
+
+/**
  * ibv_query_rt_values_ex - Get current real time @values of a device.
  * @values - in/out - defines the attributes we need to query/queried.
  * (Or's bits of enum ibv_values_mask on values->comp_mask field)
-- 
1.8.3.1

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

[PATCH RFC rdma-core 3/5] mlx5: Add support for ibv_td object and its related verbs

[Yishai Hadas]

This patch introduces the initial implementation of the
ibv_alloc/dealloc_td verbs.

Upon td creation a dedicated UAR should be attached to this thread
domain object and upon destruction the UAR is detached.

The motivation behind this is to let an application supply this ibv_td
as part of QP creation (by using the ibv_parent_domain) so that this
internal UAR will be used by both QPs, dropping any need to take a lock
upon post_send when those QPs will be used.

A full implementation of the attach/detach dedicated UAR will be given
post the acceptance of the RFC API as part of the final series.

Signed-off-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 providers/mlx5/mlx5.c  |  2 ++
 providers/mlx5/mlx5.h  | 13 +++++++++++++
 providers/mlx5/verbs.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/providers/mlx5/mlx5.c b/providers/mlx5/mlx5.c
index cbdfc40..0c073e2 100644
--- a/providers/mlx5/mlx5.c
+++ b/providers/mlx5/mlx5.c
@@ -1009,6 +1009,8 @@ static int mlx5_init_context(struct verbs_device *vdev,
 	verbs_set_ctx_op(v_ctx, create_rwq_ind_table, mlx5_create_rwq_ind_table);
 	verbs_set_ctx_op(v_ctx, destroy_rwq_ind_table, mlx5_destroy_rwq_ind_table);
 	verbs_set_ctx_op(v_ctx, post_srq_ops, mlx5_post_srq_ops);
+	verbs_set_ctx_op(v_ctx, alloc_td, mlx5_alloc_td);
+	verbs_set_ctx_op(v_ctx, dealloc_td, mlx5_dealloc_td);
 
 	memset(&device_attr, 0, sizeof(device_attr));
 	if (!mlx5_query_device_ex(ctx, NULL, &device_attr,
diff --git a/providers/mlx5/mlx5.h b/providers/mlx5/mlx5.h
index 5978e2e..da0aa91 100644
--- a/providers/mlx5/mlx5.h
+++ b/providers/mlx5/mlx5.h
@@ -321,6 +321,11 @@ struct mlx5_pd {
 	uint32_t			pdn;
 };
 
+struct mlx5_td {
+	struct ibv_td			ibv_td;
+	struct mlx5_bf			*bf;
+};
+
 enum {
 	MLX5_CQ_SET_CI	= 0,
 	MLX5_CQ_ARM_DB	= 1,
@@ -569,6 +574,11 @@ static inline struct mlx5_srq *to_msrq(struct ibv_srq *ibsrq)
 	return container_of(vsrq, struct mlx5_srq, vsrq);
 }
 
+static inline struct mlx5_td *to_mtd(struct ibv_td *ibtd)
+{
+	return to_mxxx(td, td);
+}
+
 static inline struct mlx5_qp *to_mqp(struct ibv_qp *ibqp)
 {
 	struct verbs_qp *vqp = (struct verbs_qp *)ibqp;
@@ -753,6 +763,9 @@ int mlx5_post_srq_ops(struct ibv_srq *srq,
 		      struct ibv_ops_wr *wr,
 		      struct ibv_ops_wr **bad_wr);
 
+struct ibv_td *mlx5_alloc_td(struct ibv_context *context);
+int mlx5_dealloc_td(struct ibv_td *td);
+
 static inline void *mlx5_find_uidx(struct mlx5_context *ctx, uint32_t uidx)
 {
 	int tind = uidx >> MLX5_UIDX_TABLE_SHIFT;
diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
index fea81f9..8cf14c0 100644
--- a/providers/mlx5/verbs.c
+++ b/providers/mlx5/verbs.c
@@ -153,6 +153,49 @@ struct ibv_pd *mlx5_alloc_pd(struct ibv_context *context)
 	return &pd->ibv_pd;
 }
 
+static struct mlx5_bf *mlx5_attach_dedicated_bf(struct ibv_context *context)
+{
+	/* TBD: allocate a dedciated BF to be used by that thread domain */
+	return NULL;
+}
+
+static int mlx5_detach_dedicated_bf(struct mlx5_bf *bf)
+{
+	/* TBD */
+	return 0;
+}
+
+struct ibv_td *mlx5_alloc_td(struct ibv_context *context)
+{
+	struct mlx5_td	*td;
+
+	td = calloc(1, sizeof *td);
+	if (!td)
+		return NULL;
+
+	td->bf = mlx5_attach_dedicated_bf(context);
+	if (td->bf)
+		return NULL;
+
+	td->ibv_td.context = context;
+	return &td->ibv_td;
+}
+
+int mlx5_dealloc_td(struct ibv_td *ib_td)
+{
+	struct mlx5_td	*td;
+	int err;
+
+	td = to_mtd(ib_td);
+	err = mlx5_detach_dedicated_bf(td->bf);
+
+	if (err)
+		return err;
+
+	free(td);
+	return 0;
+}
+
 int mlx5_free_pd(struct ibv_pd *pd)
 {
 	int ret;
-- 
1.8.3.1

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

[PATCH RFC rdma-core 4/5] mlx5: Add support for ibv_parent domain and its related verbs

[Yishai Hadas]

This patch is the initial implementation of ibv_alloc/dealloc_parent
domain verbs in the mlx5 driver.

The driver uses internally mlx5_pd structure which its prefix holds
the legacy ibv_pd so that the return ibv_pd can be used with any verb
around that gets a protection domain.

In addition, the driver saves an indication whether this ibv_pd is a
parent domain and as such may hold a thread domain and a pointer to a
previously allocated protection domain.

In downstream patches the mlx5 driver will consider upon getting an
ibv_pd which fields should be used based on the above indicator.

Signed-off-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 providers/mlx5/mlx5.c  |  2 ++
 providers/mlx5/mlx5.h  |  9 +++++++++
 providers/mlx5/verbs.c | 23 +++++++++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/providers/mlx5/mlx5.c b/providers/mlx5/mlx5.c
index 0c073e2..b160373 100644
--- a/providers/mlx5/mlx5.c
+++ b/providers/mlx5/mlx5.c
@@ -1011,6 +1011,8 @@ static int mlx5_init_context(struct verbs_device *vdev,
 	verbs_set_ctx_op(v_ctx, post_srq_ops, mlx5_post_srq_ops);
 	verbs_set_ctx_op(v_ctx, alloc_td, mlx5_alloc_td);
 	verbs_set_ctx_op(v_ctx, dealloc_td, mlx5_dealloc_td);
+	verbs_set_ctx_op(v_ctx, alloc_parent_domain, mlx5_alloc_parent_domain);
+	verbs_set_ctx_op(v_ctx, dealloc_parent_domain, mlx5_dealloc_parent_domain);
 
 	memset(&device_attr, 0, sizeof(device_attr));
 	if (!mlx5_query_device_ex(ctx, NULL, &device_attr,
diff --git a/providers/mlx5/mlx5.h b/providers/mlx5/mlx5.h
index da0aa91..f9b83ad 100644
--- a/providers/mlx5/mlx5.h
+++ b/providers/mlx5/mlx5.h
@@ -319,6 +319,9 @@ struct mlx5_buf {
 struct mlx5_pd {
 	struct ibv_pd			ibv_pd;
 	uint32_t			pdn;
+	int 				is_parent_domain;
+	struct ibv_td			*td;
+	struct ibv_pd			*protection_domain;
 };
 
 struct mlx5_td {
@@ -766,6 +769,12 @@ int mlx5_post_srq_ops(struct ibv_srq *srq,
 struct ibv_td *mlx5_alloc_td(struct ibv_context *context);
 int mlx5_dealloc_td(struct ibv_td *td);
 
+struct ibv_pd *mlx5_alloc_parent_domain(struct ibv_context *context,
+					struct ibv_parent_domain_init_attr *attr);
+
+int mlx5_dealloc_parent_domain(struct ibv_pd *parent_domain);
+
+
 static inline void *mlx5_find_uidx(struct mlx5_context *ctx, uint32_t uidx)
 {
 	int tind = uidx >> MLX5_UIDX_TABLE_SHIFT;
diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
index 8cf14c0..13844b3 100644
--- a/providers/mlx5/verbs.c
+++ b/providers/mlx5/verbs.c
@@ -196,6 +196,29 @@ int mlx5_dealloc_td(struct ibv_td *ib_td)
 	return 0;
 }
 
+struct ibv_pd *mlx5_alloc_parent_domain(struct ibv_context *context,
+			struct ibv_parent_domain_init_attr *attr)
+{
+	struct mlx5_pd	*pd;
+
+	pd = calloc(1, sizeof *pd);
+	if (!pd)
+		return NULL;
+
+	pd->is_parent_domain = 1;
+	pd->td = attr->td;
+	pd->protection_domain= attr->pd;
+
+	pd->ibv_pd.context = context;
+	return &pd->ibv_pd;
+}
+
+int mlx5_dealloc_parent_domain(struct ibv_pd *parent_domain)
+{
+	free(to_mpd(parent_domain));
+	return 0;
+}
+
 int mlx5_free_pd(struct ibv_pd *pd)
 {
 	int ret;
-- 
1.8.3.1

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

[PATCH RFC rdma-core 5/5] mlx5: Handles QP creation with a given parent domain

[Yishai Hadas]

This patch comes to demonstrate the expected usage of parent domain
and its internal thread domain as part of QP creation.

In case a parent domain was set, its internal protection domain (i.e.
ibv_pd) will be used for the PD usage and if a thread domain exists its
dedicated UAR will be used by passing its index to the mlx5
kernel driver. In that way application can control the UAR that this QP
will use and share it with other QPs upon their creation by suppling the
same thread domain.

A full patch will be supplied as part the final series post this RFC.

Signed-off-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 providers/mlx5/mlx5-abi.h |  3 ++-
 providers/mlx5/verbs.c    | 41 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/providers/mlx5/mlx5-abi.h b/providers/mlx5/mlx5-abi.h
index d1e8b9d..1c6116c 100644
--- a/providers/mlx5/mlx5-abi.h
+++ b/providers/mlx5/mlx5-abi.h
@@ -43,6 +43,7 @@
 enum {
 	MLX5_QP_FLAG_SIGNATURE		= 1 << 0,
 	MLX5_QP_FLAG_SCATTER_CQE	= 1 << 1,
+	MLX5_QP_FLAG_UAR_INDEX		= 1 << 2,
 };
 
 enum {
@@ -196,7 +197,7 @@ struct mlx5_create_qp {
 	__u32				rq_wqe_shift;
 	__u32				flags;
 	__u32                           uidx;
-	__u32                           reserved;
+	__u32                           uar_user_index;
 	/* SQ buffer address - used for Raw Packet QP */
 	__u64                           sq_buf_addr;
 };
diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
index 13844b3..85e3841 100644
--- a/providers/mlx5/verbs.c
+++ b/providers/mlx5/verbs.c
@@ -1094,11 +1094,14 @@ static int mlx5_calc_wq_size(struct mlx5_context *ctx,
 }
 
 static void map_uuar(struct ibv_context *context, struct mlx5_qp *qp,
-		     int uuar_index)
+		     int uuar_index, struct mlx5_bf *dyn_bf)
 {
 	struct mlx5_context *ctx = to_mctx(context);
 
-	qp->bf = &ctx->bfs[uuar_index];
+	if (!dyn_bf)
+		qp->bf = &ctx->bfs[uuar_index];
+	else
+		qp->bf = dyn_bf;
 }
 
 static const char *qptype2key(enum ibv_qp_type type)
@@ -1309,6 +1312,19 @@ enum {
 					IBV_QP_INIT_ATTR_RX_HASH),
 };
 
+static void mlx5_get_domains(struct ibv_pd *pd, struct ibv_pd **protection_domain,
+				struct ibv_td **td)
+{
+	struct mlx5_pd *mpd = to_mpd(pd);
+	if (mpd->is_parent_domain) {
+		*protection_domain = mpd->protection_domain;
+		*td  = mpd->td;
+	} else {
+		*protection_domain = pd;
+		*td = NULL;
+	}
+}
+
 static struct ibv_qp *create_qp(struct ibv_context *context,
 			 struct ibv_qp_init_attr_ex *attr)
 {
@@ -1322,6 +1338,10 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
 	int32_t				usr_idx = 0;
 	uint32_t			uuar_index;
 	FILE *fp = ctx->dbg_fp;
+	struct ibv_pd *pd;
+	struct ibv_td *td;
+	struct mlx5_td *mtd = NULL;
+	struct ibv_pd *attr_pd = attr->pd;
 
 	if (attr->comp_mask & ~MLX5_CREATE_QP_SUP_COMP_MASK)
 		return NULL;
@@ -1335,6 +1355,13 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
 		mlx5_dbg(fp, MLX5_DBG_QP, "\n");
 		return NULL;
 	}
+
+	mlx5_get_domains(attr->pd, &pd, &td);
+	if (!pd)
+		goto err;
+	attr->pd = pd;
+	if (td)
+		mtd = to_mtd(td);
 	ibqp = (struct ibv_qp *)&qp->verbs_qp;
 	qp->ibv_qp = ibqp;
 
@@ -1440,6 +1467,11 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
 		cmd.uidx = usr_idx;
 	}
 
+	if (mtd) {
+		cmd.uar_user_index = mtd->bf->uuarn;
+		cmd.flags |= MLX5_QP_FLAG_UAR_INDEX;
+	}
+
 	if (attr->comp_mask & MLX5_CREATE_QP_EX2_COMP_MASK)
 		ret = mlx5_cmd_create_qp_ex(context, attr, &cmd, qp, &resp_ex);
 	else
@@ -1465,7 +1497,7 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
 		pthread_mutex_unlock(&ctx->qp_table_mutex);
 	}
 
-	map_uuar(context, qp, uuar_index);
+	map_uuar(context, qp, uuar_index, mtd ? mtd->bf : NULL);
 
 	qp->rq.max_post = qp->rq.wqe_cnt;
 	if (attr->sq_sig_all)
@@ -1481,6 +1513,7 @@ static struct ibv_qp *create_qp(struct ibv_context *context,
 	qp->rsc.rsn = (ctx->cqe_version && !is_xrc_tgt(attr->qp_type)) ?
 		      usr_idx : ibqp->qp_num;
 
+	attr->pd = attr_pd;
 	return ibqp;
 
 err_destroy:
@@ -1500,7 +1533,7 @@ err_free_qp_buf:
 
 err:
 	free(qp);
-
+	attr->pd = attr_pd;
 	return NULL;
 }
 
-- 
1.8.3.1

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

Re: [PATCH RFC rdma-core 2/5] verbs: Introduce parent domain and its related verbs

[Jason Gunthorpe]

On Sun, Nov 12, 2017 at 11:41:40PM +0200, Yishai Hadas wrote:

> +.BI "struct ibv_pd *ibv_alloc_parent_domain(struct ibv_context "*context" ", struct ibv_domain_init_attr " "*attr");
> +.sp
> +.BI "int ibv_dealloc_parent_domain(struct ibv_pd " "*pd");

I don't think we should have a dealloc for this, since it is ibv_pd it
should go through the normal ibv_dealloc_pd. Having two deletors for
the same type is too hard to use.

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

Re: [PATCH RFC rdma-core 1/5] verbs: Introduce thread domain and its related verbs

[Jason Gunthorpe]

On Sun, Nov 12, 2017 at 11:41:39PM +0200, Yishai Hadas wrote:

> +struct ibv_td {
> +	struct ibv_context     *context;
> +};

As much as possible, I would like to see any new objects be 'opaque'
to the application, so this should just be

struct ibv_td;

And ibv_td should be defined in driver.h or something

This avoids leaking internal details and means we don't have to commit
to an ABI for the insides of these structs.

It was a mistake that so many structs were entirely exposed in the
original verbs :(

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

Re: [PATCH RFC rdma-core 4/5] mlx5: Add support for ibv_parent domain and its related verbs

[Jason Gunthorpe]

On Sun, Nov 12, 2017 at 11:41:42PM +0200, Yishai Hadas wrote:

> @@ -319,6 +319,9 @@ struct mlx5_buf {
>  struct mlx5_pd {
>  	struct ibv_pd			ibv_pd;
>  	uint32_t			pdn;
> +	int 				is_parent_domain;
> +	struct ibv_td			*td;
> +	struct ibv_pd			*protection_domain;

is_parent_domain can just be protection_domain != NULL

> +struct ibv_pd *mlx5_alloc_parent_domain(struct ibv_context *context,
> +			struct ibv_parent_domain_init_attr *attr)
> +{
> +	struct mlx5_pd	*pd;
> +
> +	pd = calloc(1, sizeof *pd);
> +	if (!pd)
> +		return NULL;
> +
> +	pd->is_parent_domain = 1;
> +	pd->td = attr->td;
> +	pd->protection_domain= attr->pd;

Don't we need some kind of ref counting here?

What is the intention for the final version of this patch?

Are you going to do
  pd->ibv_pd = *attr->pd;

During create?

Or change every call site very roughly like like..

inline struct mlx5_pd *resolve_pd(struct ibv_pd *pd)
{
   if (pd->protection_domain)
      return pd->protection_domain;
   return pd;
}

mlx5_foo(struct ibv_pd *arg_pd)
{
    struct mlx5_pd *pd = resolve_pd(arg_pd);

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

Re: [PATCH RFC rdma-core 5/5] mlx5: Handles QP creation with a given parent domain

[Jason Gunthorpe]

On Sun, Nov 12, 2017 at 11:41:43PM +0200, Yishai Hadas wrote:
> This patch comes to demonstrate the expected usage of parent domain
> and its internal thread domain as part of QP creation.
> 
> In case a parent domain was set, its internal protection domain (i.e.
> ibv_pd) will be used for the PD usage and if a thread domain exists its
> dedicated UAR will be used by passing its index to the mlx5
> kernel driver. In that way application can control the UAR that this QP
> will use and share it with other QPs upon their creation by suppling the
> same thread domain.
> 
> A full patch will be supplied as part the final series post this RFC.

I thought the plan was to have API entry points under mlx5dv to
access and set the UAR on the TD? Is that still the case?

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

Re: [PATCH RFC rdma-core 0/5] Add thread domain support

[Jason Gunthorpe]

On Sun, Nov 12, 2017 at 11:41:38PM +0200, Yishai Hadas wrote:
> This RFC series is based on some initial discussion that was in the mailing
> list to support resource domain.
> 
> The series includes detailed man pages of the suggested APIs and initial
> patches in both verbs and mlx5 driver to demonstrate the expected usage.
> 
> The motivation behind the series is to allow an application finer
> grain control over share hardware resources and locks when the
> resources are used by the same thread.

I think I'm basically fine with this approach.

It is weird trade off to have a ibv_pd have two roles, but this seems
like the least bad option when compared with revising every single
ABI.

I assume you'll also add a create_cq_ex to accept a PD instead of a
device?

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

Re: [PATCH RFC rdma-core 5/5] mlx5: Handles QP creation with a given parent domain

[Alex Rosenbaum]

On Mon, Nov 13, 2017 at 10:05 PM, Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:
> On Sun, Nov 12, 2017 at 11:41:43PM +0200, Yishai Hadas wrote:
>
> I thought the plan was to have API entry points under mlx5dv to
> access and set the UAR on the TD? Is that still the case?

It is not needed for now. the UAR index maps nicely to the td object.
the uar index hint was needed for cases where there are very few uar's
in the HW to allocate for potential too many threads.

we will expose the MAX uar's available for a context via DV API.

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

Re: [PATCH RFC rdma-core 2/5] verbs: Introduce parent domain and its related verbs

[Alex Rosenbaum]

On Mon, Nov 13, 2017 at 9:53 PM, Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:
> On Sun, Nov 12, 2017 at 11:41:40PM +0200, Yishai Hadas wrote:
>
>> +.BI "struct ibv_pd *ibv_alloc_parent_domain(struct ibv_context "*context" ", struct ibv_domain_init_attr " "*attr");
>> +.sp
>> +.BI "int ibv_dealloc_parent_domain(struct ibv_pd " "*pd");
>
> I don't think we should have a dealloc for this, since it is ibv_pd it
> should go through the normal ibv_dealloc_pd. Having two deletors for
> the same type is too hard to use.

good idea.

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

Re: [PATCH RFC rdma-core 1/5] verbs: Introduce thread domain and its related verbs

[Yishai Hadas]

On 11/13/2017 9:58 PM, Jason Gunthorpe wrote:
> On Sun, Nov 12, 2017 at 11:41:39PM +0200, Yishai Hadas wrote:
> 
>> +struct ibv_td {
>> +	struct ibv_context     *context;
>> +};
> 
> As much as possible, I would like to see any new objects be 'opaque'
> to the application, so this should just be
> 
> struct ibv_td;
> 
> And ibv_td should be defined in driver.h or something
> 
> This avoids leaking internal details and means we don't have to commit
> to an ABI for the insides of these structs.
> 

ibv_td should expose in verbs.h the 'context' as the inline function 
ibv_alloc_td() needs it to get the verbs_context and call the driver 
function if was set. Further extensions if will come should be 'opaque' 
as you pointed.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC rdma-core 2/5] verbs: Introduce parent domain and its related verbs

[Yishai Hadas]

On 11/13/2017 9:53 PM, Jason Gunthorpe wrote:
> On Sun, Nov 12, 2017 at 11:41:40PM +0200, Yishai Hadas wrote:
> 
>> +.BI "struct ibv_pd *ibv_alloc_parent_domain(struct ibv_context "*context" ", struct ibv_domain_init_attr " "*attr");
>> +.sp
>> +.BI "int ibv_dealloc_parent_domain(struct ibv_pd " "*pd");
> 
> I don't think we should have a dealloc for this, since it is ibv_pd it
> should go through the normal ibv_dealloc_pd. Having two deletors for
> the same type is too hard to use.
> 

I'm fine with that, just to be clear here, the application will still 
need to call twice for ibv_dealloc_pd() once to free the parent domain 
collection and later to free the protection name, this will make it 
fully symmetric from allocation/free point of view.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC rdma-core 4/5] mlx5: Add support for ibv_parent domain and its related verbs

[Yishai Hadas]

On 11/13/2017 10:03 PM, Jason Gunthorpe wrote:
> On Sun, Nov 12, 2017 at 11:41:42PM +0200, Yishai Hadas wrote:
> 
>> @@ -319,6 +319,9 @@ struct mlx5_buf {
>>   struct mlx5_pd {
>>   	struct ibv_pd			ibv_pd;
>>   	uint32_t			pdn;
>> +	int 				is_parent_domain;
>> +	struct ibv_td			*td;
>> +	struct ibv_pd			*protection_domain;
> 
> is_parent_domain can just be protection_domain != NULL

It's not enough, there might be cases that protection_domain is NULL but 
still the object is a parent domain as of in the CQ case where td may be 
applicable but not the protection_domain. Better having here clear 
indication which is set upon the creation API rather than some semantic.

>> +struct ibv_pd *mlx5_alloc_parent_domain(struct ibv_context *context,
>> +			struct ibv_parent_domain_init_attr *attr)
>> +{
>> +	struct mlx5_pd	*pd;
>> +
>> +	pd = calloc(1, sizeof *pd);
>> +	if (!pd)
>> +		return NULL;
>> +
>> +	pd->is_parent_domain = 1;
>> +	pd->td = attr->td;
>> +	pd->protection_domain= attr->pd;
> 
> Don't we need some kind of ref counting here?
> 

We thought about it as well, however reference counting can't prevent 
race conditions when more than one thread uses the API.
We expect the application to work as the API is documented. (e.g. 
dealloc_parent_domain() first and then its internal protection domain 
and thread domain.

Similar behavior exists also in current code which puts the 
responsibility on the application to correctly use the API instead of 
using some reference counting /locks inside the libraries.

For example if one thread is calling alloc_pd() and then create_qp() and 
second thread will call in parallel to dealloc_pd() and it will run 
before the create_qp() will ended the application might oops in the user 
area when ibv_pd will be used post its free as part of the command.

> What is the intention for the final version of this patch?

The next patch of this RFC around QP creation introduces the expected usage.

Look at:
static void mlx5_get_domains(struct ibv_pd *pd, struct ibv_pd
                             **protection_domain, struct ibv_td **td)

It gets an ibv_pd and returns both the protection domain and the thread 
domain. This helper function will be used in other mlx5 verbs around 
that gets an ibv_pd. In the final series there may be a separate patch 
for that post this patch that introduces the parent domain.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC rdma-core 5/5] mlx5: Handles QP creation with a given parent domain

[Yishai Hadas]

On 11/13/2017 10:24 PM, Alex Rosenbaum wrote:
> On Mon, Nov 13, 2017 at 10:05 PM, Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:
>> On Sun, Nov 12, 2017 at 11:41:43PM +0200, Yishai Hadas wrote:
>>
>> I thought the plan was to have API entry points under mlx5dv to
>> access and set the UAR on the TD? Is that still the case?
> 
> It is not needed for now. the UAR index maps nicely to the td object.

Correct, see patch #3 around TD creation which maps UAR to a TD. (i.e 
mlx5_attach_dedicated_bf()). Application will not control which UAR 
index will be used but will control the option to share same UAR between 
QPs by using same TD object upon QP creation.

> the uar index hint was needed for cases where there are very few uar's
> in the HW to allocate for potential too many threads.
> 
> we will expose the MAX uar's available for a context via DV API.
>

Correct, this will give the application an hint what is the max 
ibv_td(s) that can be created as each of is mapped to a dedicated UAR 
under the cover. As this is some mlx5 specification implementation 
detail it will be exposed by the DV API.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC rdma-core 0/5] Add thread domain support

[Yishai Hadas]

On 11/13/2017 10:06 PM, Jason Gunthorpe wrote:
> On Sun, Nov 12, 2017 at 11:41:38PM +0200, Yishai Hadas wrote:
>> This RFC series is based on some initial discussion that was in the mailing
>> list to support resource domain.
>>
>> The series includes detailed man pages of the suggested APIs and initial
>> patches in both verbs and mlx5 driver to demonstrate the expected usage.
>>
>> The motivation behind the series is to allow an application finer
>> grain control over share hardware resources and locks when the
>> resources are used by the same thread.
> 
> I think I'm basically fine with this approach.

Fine, good news.

> I assume you'll also add a create_cq_ex to accept a PD instead of a
> device?
> 

Future looking:
1) ibv_cq_init_attr_ex may be extended to get also parent_domain when 
there will be some driver code that will implement it, prefer not to 
introduce an un-supported API.

2) As TD may include some other properties as of core/numa information, 
need to have its creation API extended from day one by having a 
comp_mask in place.

struct ibv_td_init_attr {	
	uint32_t comp_mask;
};

struct ibv_td *ibv_alloc_td(struct ibv_context *context,
                               struct ibv_td_init_attr *attr)

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

Re: [PATCH RFC rdma-core 2/5] verbs: Introduce parent domain and its related verbs

[Jason Gunthorpe]

On Tue, Nov 14, 2017 at 10:34:25AM +0200, Yishai Hadas wrote:

> >I don't think we should have a dealloc for this, since it is ibv_pd it
> >should go through the normal ibv_dealloc_pd. Having two deletors for
> >the same type is too hard to use.
> 
> I'm fine with that, just to be clear here, the application will still need
> to call twice for ibv_dealloc_pd() once to free the parent domain collection
> and later to free the protection name, this will make it fully symmetric
> from allocation/free point of view.

Yes

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

Re: [PATCH RFC rdma-core 4/5] mlx5: Add support for ibv_parent domain and its related verbs

[Jason Gunthorpe]

On Tue, Nov 14, 2017 at 12:29:14PM +0200, Yishai Hadas wrote:

> It's not enough, there might be cases that protection_domain is NULL but
> still the object is a parent domain as of in the CQ case where td may be
> applicable but not the protection_domain. Better having here clear
> indication which is set upon the creation API rather than some semantic.

Ugh, the idea of a ibv_pd without a ibv_pd is a ugly. That is a whole
bunch more checking and failure scenarios.

I think I'd rather have the CQ receive a PD it doesn't need than do
that..

> >>+struct ibv_pd *mlx5_alloc_parent_domain(struct ibv_context *context,
> >>+			struct ibv_parent_domain_init_attr *attr)

> >>+	pd->protection_domain= attr->pd;
> >
> >Don't we need some kind of ref counting here?
> >
> 
> We thought about it as well, however reference counting can't prevent race
> conditions when more than one thread uses the API.

Not really concerned about races. More concerned about helping the
user use it properly..

> Similar behavior exists also in current code which puts the responsibility
> on the application to correctly use the API instead of using some reference
> counting /locks inside the libraries.

We have refcounts in the kernel side for many things and generate
errors if the user does stuff in the wrong order.

> >What is the intention for the final version of this patch?
> 
> The next patch of this RFC around QP creation introduces the
> expected usage.

Well, this transformation would have to ultimately be done in this
patch or before.

Looking more, this isn't good enough, at the very least I think you
need to memcpy the struct ibv_pd portion so context and handle are
set, as those are public ABIs.

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

Re: [PATCH RFC rdma-core 1/5] verbs: Introduce thread domain and its related verbs

[Jason Gunthorpe]

On Tue, Nov 14, 2017 at 10:26:16AM +0200, Yishai Hadas wrote:
> On 11/13/2017 9:58 PM, Jason Gunthorpe wrote:
> >On Sun, Nov 12, 2017 at 11:41:39PM +0200, Yishai Hadas wrote:
> >
> >>+struct ibv_td {
> >>+	struct ibv_context     *context;
> >>+};
> >
> >As much as possible, I would like to see any new objects be 'opaque'
> >to the application, so this should just be
> >
> >struct ibv_td;
> >
> >And ibv_td should be defined in driver.h or something
> >
> >This avoids leaking internal details and means we don't have to commit
> >to an ABI for the insides of these structs.
> >
> 
> ibv_td should expose in verbs.h the 'context' as the inline function
> ibv_alloc_td() needs it to get the verbs_context and call the driver
> function if was set.

You mean ibv_dealloc_td, OK..

Related to my other point, ibv_alloc_td should probably accept a
ibv_pd as an argument instead.

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

Re: [PATCH RFC rdma-core 0/5] Add thread domain support

[Jason Gunthorpe]

On Tue, Nov 14, 2017 at 02:09:35PM +0200, Yishai Hadas wrote:
> On 11/13/2017 10:06 PM, Jason Gunthorpe wrote:

> 2) As TD may include some other properties as of core/numa information, need
> to have its creation API extended from day one by having a comp_mask in
> place.
> 
> struct ibv_td_init_attr {	
> 	uint32_t comp_mask;
> };

Yes OK, let's do that

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

Re: [PATCH RFC rdma-core 0/5] Add thread domain support

[Alex Rosenbaum]

oops, I did not reply-all...  adding the ML back
although Yishai replied/answered most of this anyway.


On Tue, Nov 14, 2017 at 4:57 AM, Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:
>
> On Mon, Nov 13, 2017 at 10:32:06PM +0200, Alex Rosenbaum wrote:
>
> > > I assume you'll also add a create_cq_ex to accept a PD instead of a
> > > device?
>
> > currently we're mainly focusing on improving the inter-QP locks.
> > we did not plan to improve the CQ related locking at this point so we
> > did not plan to add parent domain to the create CQ in this series.
> > once Mellanox (or someone else) will improve the CQ related locks it
> > will definitely be the first step.
>
> I would like to see the evolve into a more complete solution, not just
> for simple locking but also NUMA issues as well.
>
> Eg I would like to see it replace the rather limited comp_vector in the
> CQ.
>
> Adding APIs like
>
> ibv_td_set_cpuset()
>  * Tell the library the application has bound the calling threads to
>  certain CPU(s). This may help the library & kernel make a NUMA
>  decision, eg by choosing the right MSI-X or memory allocation strategy.
>
> ibv_td_set_locking_mode()
>  * Disable locking inside the library assuming the caller guarentees
>    external locking or single-threaded operation.
>
> etc
>
> And given Doug's preference to keep the API style, lets use some
> comp_mask approach and a ib_td_modify & create whatsit.
>
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC rdma-core 1/5] verbs: Introduce thread domain and its related verbs

[Yishai Hadas]

On 11/14/2017 9:17 PM, Jason Gunthorpe wrote:
> On Tue, Nov 14, 2017 at 10:26:16AM +0200, Yishai Hadas wrote:
>> On 11/13/2017 9:58 PM, Jason Gunthorpe wrote:
>>> On Sun, Nov 12, 2017 at 11:41:39PM +0200, Yishai Hadas wrote:
>>>
>>>> +struct ibv_td {
>>>> +	struct ibv_context     *context;
>>>> +};
>>>
>>> As much as possible, I would like to see any new objects be 'opaque'
>>> to the application, so this should just be
>>>
>>> struct ibv_td;
>>>
>>> And ibv_td should be defined in driver.h or something
>>>
>>> This avoids leaking internal details and means we don't have to commit
>>> to an ABI for the insides of these structs.
>>>
>>
>> ibv_td should expose in verbs.h the 'context' as the inline function
>> ibv_alloc_td() needs it to get the verbs_context and call the driver
>> function if was set.
> 
> You mean ibv_dealloc_td, OK..
> 
> Related to my other point, ibv_alloc_td should probably accept a
> ibv_pd as an argument instead.
> 

Why ibv_alloc_td() should get an ibv_pd ? do you have a typo here ?

The expected API for ibv_alloc_td() as was previously discussed expects 
to be as follows:

struct ibv_td {
	struct ibv_context     *context;
};

struct ibv_td_init_attr {	
	uint32_t comp_mask;
};

struct ibv_td *ibv_alloc_td(struct ibv_context *context,
                             struct ibv_td_init_attr *attr);
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC rdma-core 1/5] verbs: Introduce thread domain and its related verbs

[Jason Gunthorpe]

On Wed, Nov 15, 2017 at 11:34:01AM +0200, Yishai Hadas wrote:

> >>ibv_td should expose in verbs.h the 'context' as the inline function
> >>ibv_alloc_td() needs it to get the verbs_context and call the driver
> >>function if was set.
> >
> >You mean ibv_dealloc_td, OK..
> >
> >Related to my other point, ibv_alloc_td should probably accept a
> >ibv_pd as an argument instead.
> >
> 
> Why ibv_alloc_td() should get an ibv_pd ? do you have a typo here ?

No..

Since ibv_alloc_td returns a 'ibv_pd' with additional information, it
makes no sense to have an API that can return an 'ibv_pd' that is not
actually a PD.

So, all TD's have to be created from a PD, and the concept of a TD
without a PD should be entirely removed from the API.

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

Re: [PATCH RFC rdma-core 1/5] verbs: Introduce thread domain and its related verbs

[Alex Rosenbaum]

On Fri, Nov 17, 2017 at 8:42 PM, Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:
> On Wed, Nov 15, 2017 at 11:34:01AM +0200, Yishai Hadas wrote:
>
>> >>ibv_td should expose in verbs.h the 'context' as the inline function
>> >>ibv_alloc_td() needs it to get the verbs_context and call the driver
>> >>function if was set.
>> >
>> >You mean ibv_dealloc_td, OK..
>> >
>> >Related to my other point, ibv_alloc_td should probably accept a
>> >ibv_pd as an argument instead.
>> >
>>
>> Why ibv_alloc_td() should get an ibv_pd ? do you have a typo here ?
>
> No..
>
> Since ibv_alloc_td returns a 'ibv_pd' with additional information, it
> makes no sense to have an API that can return an 'ibv_pd' that is not
> actually a PD.

This new ibv_alloc_td() returns a ibv_td (Thread Domain), and it only
manages the Thread related information

The new ibv_alloc_parent_domain() returns a ibv_pd.
https://www.spinics.net/lists/linux-rdma/msg56891.html
The Parent Domain gets as input the protection domain (i.e. ibv_pd),
thread domain (i.e. ibv_td) and potential other domains in the future
(e.g. loopback).
Here you can claim you want to enforce that a parent domain 'ibv_pd'
will always include a protection domain 'ibv_pd', so that the struct
ibv_pd values are always valid. Just need to update the man page for
that.
Maybe that's the source of the confusion?

> So, all TD's have to be created from a PD, and the concept of a TD
> without a PD should be entirely removed from the API.

I can definitely see an application that wants to do something smart
with it's CQ's, like per thread or NUMA aware logic. While it has many
QPs' which it can't commit to any thread logic.
Here we'll force the user to provide some protection domain while
creating the CQ's with specific TD's. It doesn't have to be so.

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

Re: [PATCH RFC rdma-core 1/5] verbs: Introduce thread domain and its related verbs

[Jason Gunthorpe]

On Sun, Nov 19, 2017 at 08:43:16AM +0200, Alex Rosenbaum wrote:

> This new ibv_alloc_td() returns a ibv_td (Thread Domain), and it only
> manages the Thread related information

Yes, sorry, my bad. I didn't read this carefully, you are right, it is
this the API I was thinking about:

> The new ibv_alloc_parent_domain() returns a ibv_pd.
> https://www.spinics.net/lists/linux-rdma/msg56891.html

So I'd like to see:

  struct ibv_parent_domain_init_attr {
-         struct ibv_pd *pd; /* referance to a protection domain, or NULL */
+         struct ibv_pd *pd; /* referance to a protection domain, can not be NULL */

> > So, all TD's have to be created from a PD, and the concept of a TD
> > without a PD should be entirely removed from the API.
> 
> I can definitely see an application that wants to do something smart
> with it's CQ's, like per thread or NUMA aware logic. While it has many
> QPs' which it can't commit to any thread logic.
> Here we'll force the user to provide some protection domain while
> creating the CQ's with specific TD's. It doesn't have to be so.

When we revise the CQ api it can directly accept a TD object, so there
is no need for it to use the 'parent domain' version of the PD.

And even if we did use the PD for consistency, it doesn't hurt at all
to pass in a PD&TD to the CQ object and then have the CQ ignore the
PD portion.

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

Re: [PATCH RFC rdma-core 1/5] verbs: Introduce thread domain and its related verbs

[Alex Rosenbaum]

On Sun, Nov 19, 2017 at 5:32 PM, Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:
> On Sun, Nov 19, 2017 at 08:43:16AM +0200, Alex Rosenbaum wrote:
>
> So I'd like to see:
>
>   struct ibv_parent_domain_init_attr {
> -         struct ibv_pd *pd; /* referance to a protection domain, or NULL */
> +         struct ibv_pd *pd; /* referance to a protection domain, can not be NULL */
>
> And even if we did use the PD for consistency, it doesn't hurt at all
> to pass in a PD&TD to the CQ object and then have the CQ ignore the
> PD portion.

I see no problem with this direction.
We'll take it to next step now.

Thanks,
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html