[PATCH rdma-core 0/2] Optimize RoCE address handle creation for userspace

[PATCH rdma-core 0/2] Optimize RoCE address handle creation for userspace

[Yishai Hadas]

This patch set from Moni is the supplementary part of the kernel series that
was lastly sent upstream.

Creating a UD address handler when link layer is Ethernet requires resolving
the remote L3 address (GID) to a L2 address (MAC/VLAN).

Until now the way to resolve GID (which is the remote IP or a function of it)
to a MAC was with an interface supplied by libnl. The implementation of this
interface is heavy and fails on large load of requests to create an address
handle.

This series of patches enables user drivers that care for it to optimize the
resolution of L3 to L2 addresses with uverbs interface.

The series was tested successfully with CX4 in both kernel which doesn't have
this support and with a kernel which includes the above support.

Pull request was sent:
https://github.com/linux-rdma/rdma-core/pull/42

Yishai

Moni Shoua (2):
  ibverbs: Allow vendor data response in create_ah command
  mlx5: Create and destroy address handle with kernel assistance

 libibverbs/cmd.c                | 11 ++++++-----
 libibverbs/driver.h             |  4 +++-
 providers/hfi1verbs/verbs.c     |  4 +++-
 providers/ipathverbs/verbs.c    |  4 +++-
 providers/mlx5/mlx5-abi.h       |  7 +++++++
 providers/mlx5/mlx5.h           |  2 ++
 providers/mlx5/verbs.c          | 28 +++++++++++++++++++++++-----
 providers/ocrdma/ocrdma_verbs.c |  4 +++-
 providers/rxe/rxe.c             |  4 +++-
 9 files changed, 53 insertions(+), 15 deletions(-)

-- 
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 rdma-core 1/2] ibverbs: Allow vendor data response in create_ah command

[Yishai Hadas]

From: Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Let caller pass response buffer and response size to ibv_cmd_create_ah
to enable kernel driver to return some private vendor data.

Signed-off-by: Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 libibverbs/cmd.c                | 11 ++++++-----
 libibverbs/driver.h             |  4 +++-
 providers/hfi1verbs/verbs.c     |  4 +++-
 providers/ipathverbs/verbs.c    |  4 +++-
 providers/ocrdma/ocrdma_verbs.c |  4 +++-
 providers/rxe/rxe.c             |  4 +++-
 6 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/libibverbs/cmd.c b/libibverbs/cmd.c
index 06a017b..eaf7164 100644
--- a/libibverbs/cmd.c
+++ b/libibverbs/cmd.c
@@ -1492,12 +1492,13 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
 }
 
 int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
-		      struct ibv_ah_attr *attr)
+		      struct ibv_ah_attr *attr,
+		      struct ibv_create_ah_resp *resp,
+		      size_t resp_size)
 {
 	struct ibv_create_ah      cmd;
-	struct ibv_create_ah_resp resp;
 
-	IBV_INIT_CMD_RESP(&cmd, sizeof cmd, CREATE_AH, &resp, sizeof resp);
+	IBV_INIT_CMD_RESP(&cmd, sizeof cmd, CREATE_AH, resp, resp_size);
 	cmd.user_handle            = (uintptr_t) ah;
 	cmd.pd_handle              = pd->handle;
 	cmd.attr.dlid              = attr->dlid;
@@ -1515,9 +1516,9 @@ int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
 	if (write(pd->context->cmd_fd, &cmd, sizeof cmd) != sizeof cmd)
 		return errno;
 
-	(void) VALGRIND_MAKE_MEM_DEFINED(&resp, sizeof resp);
+	(void) VALGRIND_MAKE_MEM_DEFINED(resp, resp_size);
 
-	ah->handle  = resp.handle;
+	ah->handle  = resp->handle;
 	ah->context = pd->context;
 
 	return 0;
diff --git a/libibverbs/driver.h b/libibverbs/driver.h
index ea3dade..c170169 100644
--- a/libibverbs/driver.h
+++ b/libibverbs/driver.h
@@ -233,7 +233,9 @@ int ibv_cmd_post_recv(struct ibv_qp *ibqp, struct ibv_recv_wr *wr,
 int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
 			  struct ibv_recv_wr **bad_wr);
 int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
-		      struct ibv_ah_attr *attr);
+		      struct ibv_ah_attr *attr,
+		      struct ibv_create_ah_resp *resp,
+		      size_t resp_size);
 int ibv_cmd_destroy_ah(struct ibv_ah *ah);
 int ibv_cmd_attach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
 int ibv_cmd_detach_mcast(struct ibv_qp *qp, const union ibv_gid *gid, uint16_t lid);
diff --git a/providers/hfi1verbs/verbs.c b/providers/hfi1verbs/verbs.c
index 06ddbb7..d39a4d0 100644
--- a/providers/hfi1verbs/verbs.c
+++ b/providers/hfi1verbs/verbs.c
@@ -678,12 +678,14 @@ int hfi1_post_srq_recv(struct ibv_srq *ibsrq, struct ibv_recv_wr *wr,
 struct ibv_ah *hfi1_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
 {
 	struct ibv_ah *ah;
+	struct ibv_create_ah_resp resp;
 
 	ah = malloc(sizeof *ah);
 	if (ah == NULL)
 		return NULL;
 
-	if (ibv_cmd_create_ah(pd, ah, attr)) {
+	memset(&resp, 0, sizeof(resp));
+	if (ibv_cmd_create_ah(pd, ah, attr, &resp, sizeof(resp))) {
 		free(ah);
 		return NULL;
 	}
diff --git a/providers/ipathverbs/verbs.c b/providers/ipathverbs/verbs.c
index 35b2162..2145b15 100644
--- a/providers/ipathverbs/verbs.c
+++ b/providers/ipathverbs/verbs.c
@@ -653,12 +653,14 @@ int ipath_post_srq_recv(struct ibv_srq *ibsrq, struct ibv_recv_wr *wr,
 struct ibv_ah *ipath_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
 {
 	struct ibv_ah *ah;
+	struct ibv_create_ah_resp resp;
 
 	ah = malloc(sizeof *ah);
 	if (ah == NULL)
 		return NULL;
 
-	if (ibv_cmd_create_ah(pd, ah, attr)) {
+	memset(&resp, 0, sizeof(resp));
+	if (ibv_cmd_create_ah(pd, ah, attr, &resp, sizeof(resp))) {
 		free(ah);
 		return NULL;
 	}
diff --git a/providers/ocrdma/ocrdma_verbs.c b/providers/ocrdma/ocrdma_verbs.c
index 7e066dc..dce833b 100644
--- a/providers/ocrdma/ocrdma_verbs.c
+++ b/providers/ocrdma/ocrdma_verbs.c
@@ -2099,6 +2099,7 @@ struct ibv_ah *ocrdma_create_ah(struct ibv_pd *ibpd, struct ibv_ah_attr *attr)
 	int ahtbl_idx;
 	struct ocrdma_pd *pd;
 	struct ocrdma_ah *ah;
+	struct ibv_create_ah_resp resp;
 
 	pd = get_ocrdma_pd(ibpd);
 	ah = malloc(sizeof *ah);
@@ -2111,7 +2112,8 @@ struct ibv_ah *ocrdma_create_ah(struct ibv_pd *ibpd, struct ibv_ah_attr *attr)
 	if (ahtbl_idx < 0)
 		goto tbl_err;
 	attr->dlid = ahtbl_idx;
-	status = ibv_cmd_create_ah(ibpd, &ah->ibv_ah, attr);
+	memset(&resp, 0, sizeof(resp));
+	status = ibv_cmd_create_ah(ibpd, &ah->ibv_ah, attr, &resp, sizeof(resp));
 	if (status)
 		goto cmd_err;
 
diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
index d23ef3d..1dd30ea 100644
--- a/providers/rxe/rxe.c
+++ b/providers/rxe/rxe.c
@@ -781,6 +781,7 @@ static struct ibv_ah *rxe_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
 	struct rxe_ah *ah;
 	struct rxe_av *av;
 	union ibv_gid sgid;
+	struct ibv_create_ah_resp resp;
 
 	err = ibv_query_gid(pd->context, attr->port_num, attr->grh.sgid_index,
 			    &sgid);
@@ -803,7 +804,8 @@ static struct ibv_ah *rxe_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
 	rdma_gid2ip(&av->sgid_addr._sockaddr, &sgid);
 	rdma_gid2ip(&av->dgid_addr._sockaddr, &attr->grh.dgid);
 
-	if (ibv_cmd_create_ah(pd, &ah->ibv_ah, attr)) {
+	memset(&resp, 0, sizeof(resp));
+	if (ibv_cmd_create_ah(pd, &ah->ibv_ah, attr, &resp, sizeof(resp))) {
 		free(ah);
 		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 rdma-core 1/2] ibverbs: Allow vendor data response in create_ah command

[Jason Gunthorpe]

On Thu, Dec 01, 2016 at 04:25:52PM +0200, Yishai Hadas wrote:
> From: Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Let caller pass response buffer and response size to ibv_cmd_create_ah
> to enable kernel driver to return some private vendor data.
> 
> Signed-off-by: Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>  libibverbs/cmd.c                | 11 ++++++-----
>  libibverbs/driver.h             |  4 +++-
>  providers/hfi1verbs/verbs.c     |  4 +++-
>  providers/ipathverbs/verbs.c    |  4 +++-
>  providers/ocrdma/ocrdma_verbs.c |  4 +++-
>  providers/rxe/rxe.c             |  4 +++-
>  6 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/libibverbs/cmd.c b/libibverbs/cmd.c
> index 06a017b..eaf7164 100644
> +++ b/libibverbs/cmd.c
> @@ -1492,12 +1492,13 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
>  }
>  
>  int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> -		      struct ibv_ah_attr *attr)
> +		      struct ibv_ah_attr *attr,
> +		      struct ibv_create_ah_resp *resp,
> +		      size_t resp_size)

NAK without changing the name of this function..

We need to decide if we want to have comapt for ibv_cmd_ driver facing
entry points, or discard that idea now that we only have one tree.

In this case it would be easy to provide two symbol versions for this
change so maybe that is what you should do..

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 rdma-core 1/2] ibverbs: Allow vendor data response in create_ah command

[Yishai Hadas]

On 12/1/2016 6:00 PM, Jason Gunthorpe wrote:
> On Thu, Dec 01, 2016 at 04:25:52PM +0200, Yishai Hadas wrote:
>> From: Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>
>> Let caller pass response buffer and response size to ibv_cmd_create_ah
>> to enable kernel driver to return some private vendor data.
>>
>> Signed-off-by: Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>  libibverbs/cmd.c                | 11 ++++++-----
>>  libibverbs/driver.h             |  4 +++-
>>  providers/hfi1verbs/verbs.c     |  4 +++-
>>  providers/ipathverbs/verbs.c    |  4 +++-
>>  providers/ocrdma/ocrdma_verbs.c |  4 +++-
>>  providers/rxe/rxe.c             |  4 +++-
>>  6 files changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/libibverbs/cmd.c b/libibverbs/cmd.c
>> index 06a017b..eaf7164 100644
>> +++ b/libibverbs/cmd.c
>> @@ -1492,12 +1492,13 @@ int ibv_cmd_post_srq_recv(struct ibv_srq *srq, struct ibv_recv_wr *wr,
>>  }
>>
>>  int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
>> -		      struct ibv_ah_attr *attr)
>> +		      struct ibv_ah_attr *attr,
>> +		      struct ibv_create_ah_resp *resp,
>> +		      size_t resp_size)
>
> NAK without changing the name of this function..
>
> We need to decide if we want to have comapt for ibv_cmd_ driver facing
> entry points, or discard that idea now that we only have one tree.
>

The idea was that moving to one tree can drop the need to maintain 2 
different symbols, all vendors around where changed to use the new 
command. As this API is not used directly by an applications I don't see 
a real need for 2 APIs.

I don't believe that we should support legacy provider library *not* 
from this tree to run with libibverbs *from* that tree, all expect to 
come in one package.

Maybe let Doug and other people comment on as well.

> In this case it would be easy to provide two symbol versions for this
> change so maybe that is what you should do..

It can be done, however as pointed above it seems redundant, going in 
that approach won't give us the chance to enjoy from the consolidate 
rdma user space in those areas.
.
--
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 rdma-core 1/2] ibverbs: Allow vendor data response in create_ah command

[Jason Gunthorpe]

On Thu, Dec 01, 2016 at 06:30:05PM +0200, Yishai Hadas wrote:
> >> int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
> >>-		      struct ibv_ah_attr *attr)
> >>+		      struct ibv_ah_attr *attr,
> >>+		      struct ibv_create_ah_resp *resp,
> >>+		      size_t resp_size)
> >
> >NAK without changing the name of this function..
> >
> >We need to decide if we want to have comapt for ibv_cmd_ driver facing
> >entry points, or discard that idea now that we only have one tree.
> 
> The idea was that moving to one tree can drop the need to maintain 2
> different symbols, all vendors around where changed to use the new command.
> As this API is not used directly by an applications I don't see a real need
> for 2 APIs.

I'm fine with that, but we still need to change the dynamic link
symbol name when the signature is changed to ensure dynamlic linking
of wrong providers fails.

This might just be as simple as moving ibv_cmd_create_ah from
IBVERBS_1.0 to IBVERBS_1.4 in the .map file. Please test and confirm.

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 rdma-core 1/2] ibverbs: Allow vendor data response in create_ah command

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1350 bytes --]

On 12/1/2016 11:40 AM, Jason Gunthorpe wrote:
> On Thu, Dec 01, 2016 at 06:30:05PM +0200, Yishai Hadas wrote:
>>>> int ibv_cmd_create_ah(struct ibv_pd *pd, struct ibv_ah *ah,
>>>> -		      struct ibv_ah_attr *attr)
>>>> +		      struct ibv_ah_attr *attr,
>>>> +		      struct ibv_create_ah_resp *resp,
>>>> +		      size_t resp_size)
>>>
>>> NAK without changing the name of this function..
>>>
>>> We need to decide if we want to have comapt for ibv_cmd_ driver facing
>>> entry points, or discard that idea now that we only have one tree.
>>
>> The idea was that moving to one tree can drop the need to maintain 2
>> different symbols, all vendors around where changed to use the new command.
>> As this API is not used directly by an applications I don't see a real need
>> for 2 APIs.

Agreed.

> I'm fine with that, but we still need to change the dynamic link
> symbol name when the signature is changed to ensure dynamlic linking
> of wrong providers fails.

Right.

> This might just be as simple as moving ibv_cmd_create_ah from
> IBVERBS_1.0 to IBVERBS_1.4 in the .map file. Please test and confirm.

I'm pretty positive a symbol version change should be sufficient, but I
agree a test to confirm can't hurt.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

[PATCH rdma-core 2/2] mlx5: Create and destroy address handle with kernel assistance

[Yishai Hadas]

From: Moni Shoua <monis-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Depending on kernel support and when link layer is Ethernet, call kernel
when asked to create an address handle. Kernel will return destination
mac which is required to create the address handle. The alternative,
when kernel support is missing, is to use netlink which is less
efficient.  Destroying an address handle also needs kernel assistance to
clean resources that were allocated during address handle creation.

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

diff --git a/providers/mlx5/mlx5-abi.h b/providers/mlx5/mlx5-abi.h
index 4b5b577..800915d 100644
--- a/providers/mlx5/mlx5-abi.h
+++ b/providers/mlx5/mlx5-abi.h
@@ -91,6 +91,13 @@ struct mlx5_alloc_ucontext_resp {
 	__u64				hca_core_clock_offset;
 };
 
+struct mlx5_create_ah_resp {
+	struct ibv_create_ah_resp	ibv_resp;
+	__u32				response_length;
+	__u8				dmac[ETHERNET_LL_SIZE];
+	__u8				reserved[6];
+};
+
 struct mlx5_alloc_pd_resp {
 	struct ibv_alloc_pd_resp	ibv_resp;
 	__u32				pdn;
diff --git a/providers/mlx5/mlx5.h b/providers/mlx5/mlx5.h
index cb65429..cf314fc 100644
--- a/providers/mlx5/mlx5.h
+++ b/providers/mlx5/mlx5.h
@@ -217,6 +217,7 @@ enum mlx5_rsc_type {
 
 enum {
 	MLX5_USER_CMDS_SUPP_UHW_QUERY_DEVICE = 1 << 0,
+	MLX5_USER_CMDS_SUPP_UHW_CREATE_AH    = 1 << 1,
 };
 
 struct mlx5_resource {
@@ -470,6 +471,7 @@ struct mlx5_av {
 struct mlx5_ah {
 	struct ibv_ah			ibv_ah;
 	struct mlx5_av			av;
+	bool				kern_ah;
 };
 
 struct mlx5_rwq {
diff --git a/providers/mlx5/verbs.c b/providers/mlx5/verbs.c
index e288ebf..7107a5b 100644
--- a/providers/mlx5/verbs.c
+++ b/providers/mlx5/verbs.c
@@ -1636,11 +1636,21 @@ struct ibv_ah *mlx5_create_ah(struct ibv_pd *pd, struct ibv_ah_attr *attr)
 	}
 
 	if (is_eth) {
-		uint16_t vid;
+		if (ctx->cmds_supp_uhw & MLX5_USER_CMDS_SUPP_UHW_CREATE_AH) {
+			struct mlx5_create_ah_resp resp = {};
 
-		if (ibv_resolve_eth_l2_from_gid(pd->context, attr, ah->av.rmac,
-						&vid))
-			goto err;
+			if (ibv_cmd_create_ah(pd, &ah->ibv_ah, attr, &resp.ibv_resp, sizeof(resp)))
+				goto err;
+
+			ah->kern_ah = true;
+			memcpy(ah->av.rmac, resp.dmac, ETHERNET_LL_SIZE);
+		} else {
+			uint16_t vid;
+
+			if (ibv_resolve_eth_l2_from_gid(pd->context, attr,
+							ah->av.rmac, &vid))
+				goto err;
+		}
 	}
 
 	return &ah->ibv_ah;
@@ -1651,8 +1661,16 @@ err:
 
 int mlx5_destroy_ah(struct ibv_ah *ah)
 {
-	free(to_mah(ah));
+	struct mlx5_ah *mah = to_mah(ah);
+	int err;
+
+	if (mah->kern_ah) {
+		err = ibv_cmd_destroy_ah(ah);
+		if (err)
+			return err;
+	}
 
+	free(mah);
 	return 0;
 }
 
-- 
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