[PATCH libmlx4 V3 0/2] Add support for UD QPs under RoCE IP addressing

[PATCH libmlx4 V3 0/2] Add support for UD QPs under RoCE IP addressing

[Or Gerlitz]

From: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Hi Yishai,

This is just a rebased-port of the series which adds support for Ethernet L2 address 
resolution for UD QPs, whose L2 address-handles, unlike RC/UC/XRC/etc QPs are set 
from user space without going through uverbs and the kernel IB core.

Matan and Or.

changes from V2:
 - Adapt the code for the latest changes in the related libibverbs series

changes from V1:
 - rebased against libmlx4  1.0.6

changes from V0:
 - better checking for invalid port numbers
 - create_ah_ex in libmlx4 conforms to the verbs extensions mechanism

Matan Barak (2):
  Add RoCE IP based addressing support for UD QPs
  Add ibv_query_port_ex support

 src/mlx4.c  |    4 ++
 src/mlx4.h  |   11 +++++
 src/verbs.c |  141 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 144 insertions(+), 12 deletions(-)

--
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 libmlx4 V3 1/2] Add RoCE IP based addressing support for UD QPs

[Or Gerlitz]

From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

In order to implement IP based addressing for UD QPs, we need a way to
resolve the addresses internally.
The L2 params are passed to the provider driver using an extension verbs
- drv_ibv_create_ah_ex.
libmlx4 gets the extra mac and vid params from libibverbs and sets
mlx4_ah relevant attributes.

Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 src/mlx4.c  |    1 +
 src/mlx4.h  |    2 +
 src/verbs.c |   85 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/src/mlx4.c b/src/mlx4.c
index 1ee0338..a9f3a98 100644
--- a/src/mlx4.c
+++ b/src/mlx4.c
@@ -205,6 +205,7 @@ static int mlx4_init_context(struct verbs_device *v_device,
 	verbs_set_ctx_op(verbs_ctx, open_qp, mlx4_open_qp);
 	verbs_set_ctx_op(verbs_ctx, create_flow, ibv_cmd_create_flow);
 	verbs_set_ctx_op(verbs_ctx, destroy_flow, ibv_cmd_destroy_flow);
+	verbs_set_ctx_op(verbs_ctx, create_ah_ex, mlx4_create_ah_ex);
 
 	return 0;
 
diff --git a/src/mlx4.h b/src/mlx4.h
index d71450f..3015357 100644
--- a/src/mlx4.h
+++ b/src/mlx4.h
@@ -431,6 +431,8 @@ struct mlx4_qp *mlx4_find_qp(struct mlx4_context *ctx, uint32_t qpn);
 int mlx4_store_qp(struct mlx4_context *ctx, uint32_t qpn, struct mlx4_qp *qp);
 void mlx4_clear_qp(struct mlx4_context *ctx, uint32_t qpn);
 struct ibv_ah *mlx4_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr);
+struct ibv_ah *mlx4_create_ah_ex(struct ibv_pd *pd,
+				 struct ibv_ah_attr_ex *attr_ex);
 int mlx4_destroy_ah(struct ibv_ah *ah);
 int mlx4_alloc_av(struct mlx4_pd *pd, struct ibv_ah_attr *attr,
 		   struct mlx4_ah *ah);
diff --git a/src/verbs.c b/src/verbs.c
index 623d576..ccfd678 100644
--- a/src/verbs.c
+++ b/src/verbs.c
@@ -40,6 +40,8 @@
 #include <pthread.h>
 #include <errno.h>
 #include <netinet/in.h>
+#include <net/if.h>
+#include <net/if_arp.h>
 
 #include "mlx4.h"
 #include "mlx4-abi.h"
@@ -783,13 +785,11 @@ static int mlx4_resolve_grh_to_l2(struct ibv_pd *pd, struct mlx4_ah *ah,
 	return 0;
 }
 
-struct ibv_ah *mlx4_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
+static struct ibv_ah *mlx4_create_ah_common(struct ibv_pd *pd,
+					    struct ibv_ah_attr *attr,
+					    uint8_t link_layer)
 {
 	struct mlx4_ah *ah;
-	struct ibv_port_attr port_attr;
-
-	if (ibv_query_port(pd->context, attr->port_num, &port_attr))
-		return NULL;
 
 	ah = malloc(sizeof *ah);
 	if (!ah)
@@ -799,7 +799,7 @@ struct ibv_ah *mlx4_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
 
 	ah->av.port_pd   = htonl(to_mpd(pd)->pdn | (attr->port_num << 24));
 
-	if (port_attr.link_layer != IBV_LINK_LAYER_ETHERNET) {
+	if (link_layer != IBV_LINK_LAYER_ETHERNET) {
 		ah->av.g_slid = attr->src_path_bits;
 		ah->av.dlid   = htons(attr->dlid);
 		ah->av.sl_tclass_flowlabel = htonl(attr->sl << 28);
@@ -820,13 +820,76 @@ struct ibv_ah *mlx4_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
 		memcpy(ah->av.dgid, attr->grh.dgid.raw, 16);
 	}
 
-	if (port_attr.link_layer == IBV_LINK_LAYER_ETHERNET)
-		if (mlx4_resolve_grh_to_l2(pd, ah, attr)) {
-			free(ah);
-			return NULL;
+	return &ah->ibv_ah;
+}
+
+struct ibv_ah *mlx4_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
+{
+	struct ibv_ah *ah;
+	struct ibv_port_attr port_attr;
+
+	if (ibv_query_port(pd->context, attr->port_num, &port_attr))
+		return NULL;
+
+	ah = mlx4_create_ah_common(pd, attr, port_attr.link_layer);
+	if (NULL != ah &&
+	    (port_attr.link_layer != IBV_LINK_LAYER_ETHERNET ||
+	    !mlx4_resolve_grh_to_l2(pd, to_mah(ah), attr)))
+		return ah;
+
+	if (ah)
+		free(ah);
+	return NULL;
+}
+
+struct ibv_ah *mlx4_create_ah_ex(struct ibv_pd *pd,
+				 struct ibv_ah_attr_ex *attr_ex)
+{
+	struct ibv_port_attr port_attr;
+	struct ibv_ah *ah;
+	struct mlx4_ah *mah;
+
+	if (ibv_query_port(pd->context, attr_ex->port_num, &port_attr))
+		return NULL;
+
+	ah = mlx4_create_ah_common(pd, (struct ibv_ah_attr *)attr_ex,
+				   port_attr.link_layer);
+
+	if (NULL == ah)
+		return NULL;
+
+	mah = to_mah(ah);
+
+	/* If vlan was given, check that we could use it */
+	if (attr_ex->comp_mask & IBV_AH_ATTR_EX_VID &&
+	    attr_ex->vid <= 0xfff &&
+	    !(attr_ex->comp_mask & IBV_AH_ATTR_EX_LL))
+		goto err;
+
+	/* check that ll is provided and valid */
+	if (attr_ex->comp_mask & IBV_AH_ATTR_EX_LL) {
+		if (ARPHRD_ETHER != attr_ex->ll.sa.sa_family ||
+		    port_attr.link_layer != IBV_LINK_LAYER_ETHERNET)
+			/* mlx4 provider currently only support ethernet
+			 * extensions */
+			goto err;
+
+		memcpy(mah->mac, attr_ex->ll.sa.sa_data,
+		       IFHWADDRLEN);
+
+		if (attr_ex->comp_mask & IBV_AH_ATTR_EX_VID &&
+		    attr_ex->vid <= 0xfff) {
+				mah->av.port_pd |= htonl(1 << 29);
+				mah->vlan = attr_ex->vid |
+					((attr_ex->sl & 7) << 13);
 		}
+	}
 
-	return &ah->ibv_ah;
+	return ah;
+
+err:
+	free(ah);
+	return NULL;
 }
 
 int mlx4_destroy_ah(struct ibv_ah *ah)
-- 
1.7.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 libmlx4 V3 2/2] Add ibv_query_port_ex support

[Or Gerlitz]

From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This patch adds the new extended support for query_port.
The purpose of this is:
1. Request fields that aren't availible by today's ibv_query_port
2. Don't fetch fields that the user doesn't need. Hence, there is
   more chance to optimize.
3. Cache link layer's type in mlx4_context.
   Caching will allow us to avoid ibv_query_port calls and save time
   in ibv_create_ah.

Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 src/mlx4.c  |    3 ++
 src/mlx4.h  |    9 ++++++++
 src/verbs.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/src/mlx4.c b/src/mlx4.c
index a9f3a98..1d42270 100644
--- a/src/mlx4.c
+++ b/src/mlx4.c
@@ -157,6 +157,8 @@ static int mlx4_init_context(struct verbs_device *v_device,
 
 	context->qp_table_shift = ffs(context->num_qps) - 1 - MLX4_QP_TABLE_BITS;
 	context->qp_table_mask	= (1 << context->qp_table_shift) - 1;
+	for (i = 0; i < MLX4_PORTS_NUM; ++i)
+		context->port_query_cache[i].valid = 0;
 
 	pthread_mutex_init(&context->qp_table_mutex, NULL);
 	for (i = 0; i < MLX4_QP_TABLE_SIZE; ++i)
@@ -206,6 +208,7 @@ static int mlx4_init_context(struct verbs_device *v_device,
 	verbs_set_ctx_op(verbs_ctx, create_flow, ibv_cmd_create_flow);
 	verbs_set_ctx_op(verbs_ctx, destroy_flow, ibv_cmd_destroy_flow);
 	verbs_set_ctx_op(verbs_ctx, create_ah_ex, mlx4_create_ah_ex);
+	verbs_set_ctx_op(verbs_ctx, query_port_ex, mlx4_query_port_ex);
 
 	return 0;
 
diff --git a/src/mlx4.h b/src/mlx4.h
index 3015357..06fd2ba 100644
--- a/src/mlx4.h
+++ b/src/mlx4.h
@@ -40,6 +40,8 @@
 #include <infiniband/arch.h>
 #include <infiniband/verbs.h>
 
+#define MLX4_PORTS_NUM 2
+
 #ifdef HAVE_VALGRIND_MEMCHECK_H
 
 #  include <valgrind/memcheck.h>
@@ -189,6 +191,11 @@ struct mlx4_context {
 	pthread_mutex_t			db_list_mutex;
 	int				cqe_size;
 	struct mlx4_xsrq_table		xsrq_table;
+	struct {
+		uint8_t			valid;
+		uint8_t			link_layer;
+		enum ibv_port_cap_flags	caps;
+	} port_query_cache[MLX4_PORTS_NUM];
 };
 
 struct mlx4_buf {
@@ -354,6 +361,8 @@ int mlx4_query_device(struct ibv_context *context,
 		       struct ibv_device_attr *attr);
 int mlx4_query_port(struct ibv_context *context, uint8_t port,
 		     struct ibv_port_attr *attr);
+int mlx4_query_port_ex(struct ibv_context *context, uint8_t port_num,
+		       struct ibv_port_attr_ex *port_attr);
 
 struct ibv_pd *mlx4_alloc_pd(struct ibv_context *context);
 int mlx4_free_pd(struct ibv_pd *pd);
diff --git a/src/verbs.c b/src/verbs.c
index ccfd678..b2c71e7 100644
--- a/src/verbs.c
+++ b/src/verbs.c
@@ -72,8 +72,58 @@ int mlx4_query_port(struct ibv_context *context, uint8_t port,
 		     struct ibv_port_attr *attr)
 {
 	struct ibv_query_port cmd;
+	int err;
+
+	err = ibv_cmd_query_port(context, port, attr, &cmd, sizeof(cmd));
+	if (!err && port <= MLX4_PORTS_NUM && port > 0) {
+		struct mlx4_context *mctx = to_mctx(context);
+		if (!mctx->port_query_cache[port - 1].valid) {
+			mctx->port_query_cache[port - 1].link_layer =
+				attr->link_layer;
+			mctx->port_query_cache[port - 1].caps =
+				attr->port_cap_flags;
+			mctx->port_query_cache[port - 1].valid = 1;
+		}
+	}
+
+	return err;
+}
+
+int mlx4_query_port_ex(struct ibv_context *context, uint8_t port_num,
+		       struct ibv_port_attr_ex *port_attr)
+{
+	/* Check that only valid flags were given */
+	if (port_attr->comp_mask & ~IBV_QUERY_PORT_EX_MASK)
+		return -EINVAL;
+
+	/* Optimize the link type query */
+	if (!(port_attr->comp_mask & ~(IBV_QUERY_PORT_EX_LINK_LAYER |
+				       IBV_QUERY_PORT_EX_CAP_FLAGS))) {
+		struct mlx4_context *mctx = to_mctx(context);
+		if (port_num <= 0 || port_num > MLX4_PORTS_NUM)
+			return -EINVAL;
+		if (mctx->port_query_cache[port_num - 1].valid) {
+			if (port_attr->comp_mask &
+			    IBV_QUERY_PORT_EX_LINK_LAYER)
+				port_attr->link_layer =
+					mctx->
+					port_query_cache[port_num - 1].
+					link_layer;
+			if (port_attr->comp_mask &
+			    IBV_QUERY_PORT_EX_CAP_FLAGS)
+				port_attr->port_cap_flags =
+					mctx->
+					port_query_cache[port_num - 1].
+					caps;
+			return 0;
+		}
+	}
+	if (port_attr->comp_mask & IBV_QUERY_PORT_EX_STD_MASK) {
+		return mlx4_query_port(context, port_num,
+				       (struct ibv_port_attr *)port_attr);
+	}
 
-	return ibv_cmd_query_port(context, port, attr, &cmd, sizeof cmd);
+	return -EOPNOTSUPP;
 }
 
 struct ibv_pd *mlx4_alloc_pd(struct ibv_context *context)
@@ -826,15 +876,17 @@ static struct ibv_ah *mlx4_create_ah_common(struct ibv_pd *pd,
 struct ibv_ah *mlx4_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
 {
 	struct ibv_ah *ah;
-	struct ibv_port_attr port_attr;
+	struct ibv_port_attr_ex port_attr;
+
+	port_attr.comp_mask = IBV_QUERY_PORT_EX_LINK_LAYER;
 
-	if (ibv_query_port(pd->context, attr->port_num, &port_attr))
+	if (ibv_query_port_ex(pd->context, attr->port_num, &port_attr))
 		return NULL;
 
 	ah = mlx4_create_ah_common(pd, attr, port_attr.link_layer);
 	if (NULL != ah &&
 	    (port_attr.link_layer != IBV_LINK_LAYER_ETHERNET ||
-	    !mlx4_resolve_grh_to_l2(pd, to_mah(ah), attr)))
+	     !mlx4_resolve_grh_to_l2(pd, to_mah(ah), attr)))
 		return ah;
 
 	if (ah)
@@ -845,11 +897,13 @@ struct ibv_ah *mlx4_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
 struct ibv_ah *mlx4_create_ah_ex(struct ibv_pd *pd,
 				 struct ibv_ah_attr_ex *attr_ex)
 {
-	struct ibv_port_attr port_attr;
+	struct ibv_port_attr_ex port_attr;
 	struct ibv_ah *ah;
 	struct mlx4_ah *mah;
 
-	if (ibv_query_port(pd->context, attr_ex->port_num, &port_attr))
+	port_attr.comp_mask = IBV_QUERY_PORT_EX_LINK_LAYER;
+
+	if (ibv_query_port_ex(pd->context, attr_ex->port_num, &port_attr))
 		return NULL;
 
 	ah = mlx4_create_ah_common(pd, (struct ibv_ah_attr *)attr_ex,
-- 
1.7.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 libmlx4 V3 1/2] Add RoCE IP based addressing support for UD QPs

[Jason Gunthorpe]

On Sun, May 18, 2014 at 12:39:11PM +0300, Or Gerlitz wrote:
> From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> In order to implement IP based addressing for UD QPs, we need a way to
> resolve the addresses internally.
> The L2 params are passed to the provider driver using an extension verbs
> - drv_ibv_create_ah_ex.
   ^^^^^^
The name changed

> +struct ibv_ah *mlx4_create_ah_ex(struct ibv_pd *pd,
> +				 struct ibv_ah_attr_ex *attr_ex)
> +{
> +	struct ibv_port_attr port_attr;
> +	struct ibv_ah *ah;
> +	struct mlx4_ah *mah;
> +
> +	if (ibv_query_port(pd->context, attr_ex->port_num, &port_attr))
> +		return NULL;
> +
> +	ah = mlx4_create_ah_common(pd, (struct ibv_ah_attr *)attr_ex,
> +				   port_attr.link_layer);
> +
> +	if (NULL == ah)
> +		return NULL;

I'm seeing a real lack of error reporting here.

My prior question was never answered: Should we be consistently using
errno for functions that return pointers? If yes, you need to make
sure errno is set on these failure paths, and document the possible
values errno can take.

> +	/* check that ll is provided and valid */
> +	if (attr_ex->comp_mask & IBV_AH_ATTR_EX_LL) {
> +		if (ARPHRD_ETHER != attr_ex->ll.sa.sa_family ||
                  ^^^^^^^^^^^
As I mentioned before, ARPHDR_ETHER is not correct for sa_family.

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 libmlx4 V3 1/2] Add RoCE IP based addressing support for UD QPs

[Matan Barak]

On 21/5/2014 11:00 PM, Jason Gunthorpe wrote:
> On Sun, May 18, 2014 at 12:39:11PM +0300, Or Gerlitz wrote:
>> From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>
>> In order to implement IP based addressing for UD QPs, we need a way to
>> resolve the addresses internally.
>> The L2 params are passed to the provider driver using an extension verbs
>> - drv_ibv_create_ah_ex.
>     ^^^^^^
> The name changed
>

Thanks!

>> +struct ibv_ah *mlx4_create_ah_ex(struct ibv_pd *pd,
>> +				 struct ibv_ah_attr_ex *attr_ex)
>> +{
>> +	struct ibv_port_attr port_attr;
>> +	struct ibv_ah *ah;
>> +	struct mlx4_ah *mah;
>> +
>> +	if (ibv_query_port(pd->context, attr_ex->port_num, &port_attr))
>> +		return NULL;
>> +
>> +	ah = mlx4_create_ah_common(pd, (struct ibv_ah_attr *)attr_ex,
>> +				   port_attr.link_layer);
>> +
>> +	if (NULL == ah)
>> +		return NULL;
>
> I'm seeing a real lack of error reporting here.
>

Correct, ibv_query_port error should be captured to errno,
In addition, we need to add an error for an incorrect link layer type.

> My prior question was never answered: Should we be consistently using
> errno for functions that return pointers? If yes, you need to make
> sure errno is set on these failure paths, and document the possible
> values errno can take.
>
>> +	/* check that ll is provided and valid */
>> +	if (attr_ex->comp_mask & IBV_AH_ATTR_EX_LL) {
>> +		if (ARPHRD_ETHER != attr_ex->ll.sa.sa_family ||
>                    ^^^^^^^^^^^
> As I mentioned before, ARPHDR_ETHER is not correct for sa_family.
>

I don't want to tie this mechanism to Ethernet. If you prefer in-lining 
the data (rather than using a pointer), we might just introduce a new 
type of a big sockaddr_storage like buffer with a link_layer type enum.
What do you think?

> Jason
>

Matan
--
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