[PATCH for-next 0/2] Add creation flags for user space QPs

[PATCH for-next 0/2] Add creation flags for user space QPs

[Or Gerlitz]

This small series addresses a long time (2008..) pain point, namely 
the inability to specify creation flags for user-space QPs. We use 
the existing uverbs command and a reserved field there to specify the flags.

Or.

Or Gerlitz (2):
  IB/uverbs: Add QP creation flags, allow blocking UD multicast loopback
  IB/mlx4: Enable blocking multicast loopback for user-space consumers

 drivers/infiniband/core/uverbs_cmd.c |   35 ++++++++++++++++++++++++++++++++-
 drivers/infiniband/hw/mlx4/qp.c      |   13 +++++++----
 include/uapi/rdma/ib_user_verbs.h    |    2 +-
 3 files changed, 42 insertions(+), 8 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 for-next 1/2] IB/uverbs: Add QP creation flags, allow blocking UD multicast loopback

[Or Gerlitz]

Currently, there's no way for user-space applications to specify
the IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK QP creation flags defined
by commit 47ee1b9 "IB/core: Add support for multicast loopback blocking".

As a result, applications who send and recieve over the same QP to
the same multicast group get all their packets bouncded back to them,
which is terribly bad performance wise.

To fix this long standing issue, add the ability to provide QP
creation flags through uverbs.

Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c |   35 ++++++++++++++++++++++++++++++++-
 include/uapi/rdma/ib_user_verbs.h    |    2 +-
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 0600c50..1ad489c 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -1579,6 +1579,31 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file,
 	return in_len;
 }
 
+enum ib_uverbs_qp_create_flags {
+	IB_UVERBS_QP_CREATE_LSO	    = 0,
+	IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK = 1,
+	IB_UVERBS_SUPPORTED_FLAGS
+};
+
+static int ib_uverbs_create_qp_trans(u8 u_flags)
+{
+	int i;
+	int res;
+	static const enum ib_qp_create_flags ib_uverbs_qp_create_flags[IB_UVERBS_SUPPORTED_FLAGS] = {
+		[IB_UVERBS_QP_CREATE_LSO] = IB_QP_CREATE_IPOIB_UD_LSO,
+		[IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK] = IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK,
+	};
+
+	if (u_flags & ~((1 << IB_UVERBS_SUPPORTED_FLAGS) - 1))
+		return -1;
+
+	for (i = 0; i < IB_UVERBS_SUPPORTED_FLAGS; i++)
+		if (u_flags & (1 << i))
+			res |= ib_uverbs_qp_create_flags[i];
+
+	return res;
+}
+
 ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 			    const char __user *buf, int in_len,
 			    int out_len)
@@ -1595,7 +1620,7 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 	struct ib_srq                  *srq = NULL;
 	struct ib_qp                   *qp;
 	struct ib_qp_init_attr          attr;
-	int ret;
+	int flags, ret;
 
 	if (out_len < sizeof resp)
 		return -ENOSPC;
@@ -1664,7 +1689,13 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 	attr.xrcd	   = xrcd;
 	attr.sq_sig_type   = cmd.sq_sig_all ? IB_SIGNAL_ALL_WR : IB_SIGNAL_REQ_WR;
 	attr.qp_type       = cmd.qp_type;
-	attr.create_flags  = 0;
+
+	flags  = ib_uverbs_create_qp_trans(cmd.create_flags);
+	if (flags < 0) {
+		ret = -EINVAL;
+		goto err_put;
+	}
+	attr.create_flags  = flags;
 
 	attr.cap.max_send_wr     = cmd.max_send_wr;
 	attr.cap.max_recv_wr     = cmd.max_recv_wr;
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 26daf55..fac6975 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -470,7 +470,7 @@ struct ib_uverbs_create_qp {
 	__u8  sq_sig_all;
 	__u8  qp_type;
 	__u8  is_srq;
-	__u8  reserved;
+	__u8  create_flags;
 	__u64 driver_data[0];
 };
 
-- 
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 for-next 2/2] IB/mlx4: Enable blocking multicast loopback for user-space consumers

[Or Gerlitz]

Allow user-space UD QPs to require blocking multicast loopback.

Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/hw/mlx4/qp.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index 577b477..5e4785f 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -706,6 +706,9 @@ static int create_qp_common(struct mlx4_ib_dev *dev, struct ib_pd *pd,
 	if (err)
 		goto err;
 
+	if (init_attr->create_flags & IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK)
+			qp->flags |= MLX4_IB_QP_BLOCK_MULTICAST_LOOPBACK;
+
 	if (pd->uobject) {
 		struct mlx4_ib_create_qp ucmd;
 
@@ -745,9 +748,6 @@ static int create_qp_common(struct mlx4_ib_dev *dev, struct ib_pd *pd,
 	} else {
 		qp->sq_no_prefetch = 0;
 
-		if (init_attr->create_flags & IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK)
-			qp->flags |= MLX4_IB_QP_BLOCK_MULTICAST_LOOPBACK;
-
 		if (init_attr->create_flags & IB_QP_CREATE_IPOIB_UD_LSO)
 			qp->flags |= MLX4_IB_QP_LSO;
 
@@ -1077,9 +1077,12 @@ struct ib_qp *mlx4_ib_create_qp(struct ib_pd *pd,
 			return ERR_PTR(-EINVAL);
 	}
 
+	if (udata &&
+	    init_attr->create_flags & ~(MLX4_IB_QP_BLOCK_MULTICAST_LOOPBACK))
+		return ERR_PTR(-EINVAL);
+
 	if (init_attr->create_flags &&
-	    (udata ||
-	     ((init_attr->create_flags & ~(MLX4_IB_SRIOV_SQP | MLX4_IB_QP_CREATE_USE_GFP_NOIO)) &&
+	    (((init_attr->create_flags & ~(MLX4_IB_SRIOV_SQP | MLX4_IB_QP_CREATE_USE_GFP_NOIO)) &&
 	      init_attr->qp_type != IB_QPT_UD) ||
 	     ((init_attr->create_flags & MLX4_IB_SRIOV_SQP) &&
 	      init_attr->qp_type > IB_QPT_GSI)))
-- 
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 for-next 1/2] IB/uverbs: Add QP creation flags, allow blocking UD multicast loopback

[Yann Droneaud]

Hi,

Le dimanche 14 septembre 2014 à 16:20 +0300, Or Gerlitz a écrit :
> Currently, there's no way for user-space applications to specify
> the IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK QP creation flags defined
> by commit 47ee1b9 "IB/core: Add support for multicast loopback blocking".
> 

Commit identifier is too short, 12 digits is the norm.

> As a result, applications who send and recieve over the same QP to
> the same multicast group get all their packets bouncded back to them,
> which is terribly bad performance wise.
> 
> To fix this long standing issue, add the ability to provide QP
> creation flags through uverbs.
> 
> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/core/uverbs_cmd.c |   35 ++++++++++++++++++++++++++++++++-
>  include/uapi/rdma/ib_user_verbs.h    |    2 +-
>  2 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 0600c50..1ad489c 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -1579,6 +1579,31 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file,
>  	return in_len;
>  }
>  
> +enum ib_uverbs_qp_create_flags {
> +	IB_UVERBS_QP_CREATE_LSO	    = 0,
> +	IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK = 1,
> +	IB_UVERBS_SUPPORTED_FLAGS
> +};

IB_UVERBS_QP_CREATE_LSO and IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK
should moved in include/uapi/rdma/ib_user_verbs.h.

If this is going to be flags, these values might be OR-ed together ?
Then the userspace values should be defined like the kernel ones
(see commit 47ee1b9f2e7b):

+enum ib_uverbs_qp_create_flags {
+	IB_UVERBS_QP_CREATE_LSO	    =                  1 << 0,
+	IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK = 1 << 1,
+}

IB_UVERBS_SUPPORTED_FLAGS must be kept in this module, but defined
as:

#define IB_UVERBS_QP_CREATE_FLAGS_ALL (IB_UVERBS_QP_CREATE_LSO | \
				       IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK)

> +
> +static int ib_uverbs_create_qp_trans(u8 u_flags)
> +{
> +	int i;
> +	int res;
> +	static const enum ib_qp_create_flags ib_uverbs_qp_create_flags[IB_UVERBS_SUPPORTED_FLAGS] = {
> +		[IB_UVERBS_QP_CREATE_LSO] = IB_QP_CREATE_IPOIB_UD_LSO,
> +		[IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK] = IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK,
> +	};
> +
> +	if (u_flags & ~((1 << IB_UVERBS_SUPPORTED_FLAGS) - 1))
> +		return -1;
> +

	if (u_flags & ~(u8)(IB_UVERBS_QP_CREATE_FLAGS_ALL))
	   return -1;

> +	for (i = 0; i < IB_UVERBS_SUPPORTED_FLAGS; i++)
> +		if (u_flags & (1 << i))
> +			res |= ib_uverbs_qp_create_flags[i];
> +
> +	return res;
> +}
> +

This function can be replaced by compile time assert:

BUILD_BUG_ON(IB_UVERBS_QP_CREATE_LSO != IB_QP_CREATE_IPOIB_UD_LSO);
BUILD_BUG_ON(IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK !=
IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK);

This way the values can be used without conversion.

>  ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
>  			    const char __user *buf, int in_len,
>  			    int out_len)
> @@ -1595,7 +1620,7 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
>  	struct ib_srq                  *srq = NULL;
>  	struct ib_qp                   *qp;
>  	struct ib_qp_init_attr          attr;
> -	int ret;
> +	int flags, ret;
>  
>  	if (out_len < sizeof resp)
>  		return -ENOSPC;
> @@ -1664,7 +1689,13 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
>  	attr.xrcd	   = xrcd;
>  	attr.sq_sig_type   = cmd.sq_sig_all ? IB_SIGNAL_ALL_WR : IB_SIGNAL_REQ_WR;
>  	attr.qp_type       = cmd.qp_type;
> -	attr.create_flags  = 0;
> +
> +	flags  = ib_uverbs_create_qp_trans(cmd.create_flags);
> +	if (flags < 0) {
> +		ret = -EINVAL;
> +		goto err_put;
> +	}
> +	attr.create_flags  = flags;
>  
>  	attr.cap.max_send_wr     = cmd.max_send_wr;
>  	attr.cap.max_recv_wr     = cmd.max_recv_wr;
> diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
> index 26daf55..fac6975 100644
> --- a/include/uapi/rdma/ib_user_verbs.h
> +++ b/include/uapi/rdma/ib_user_verbs.h
> @@ -470,7 +470,7 @@ struct ib_uverbs_create_qp {
>  	__u8  sq_sig_all;
>  	__u8  qp_type;
>  	__u8  is_srq;
> -	__u8  reserved;
> +	__u8  create_flags;
>  	__u64 driver_data[0];
>  };
>  

I'm not really happy with the way "reserved" field is used by this
patch: as the field wasn't check for being set to 0, any value could be
given by userspace (imagine the structure lay on stack). Using it now
could be dangerous. It must be double checked.

http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

"Check all unused fields and flags and all the padding for whether it's
0, and reject the ioctl if that's not the case. Otherwise your nice plan
for future extensions is going right down the gutters since someone will
submit an ioctl struct with random stack garbage in the yet unused
parts. Which then bakes in the ABI that those fields can never be used
for anything else but garbage."

Regards.

-- 
Yann Droneaud
OPTEYA



--
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 for-next 1/2] IB/uverbs: Add QP creation flags, allow blocking UD multicast loopback

[Or Gerlitz]

On Mon, Sep 15, 2014 at 7:52 PM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
>> --- a/include/uapi/rdma/ib_user_verbs.h
>> +++ b/include/uapi/rdma/ib_user_verbs.h
>> @@ -470,7 +470,7 @@ struct ib_uverbs_create_qp {
>>       __u8  sq_sig_all;
>>       __u8  qp_type;
>>       __u8  is_srq;
>> -     __u8  reserved;
>> +     __u8  create_flags;
>>       __u64 driver_data[0];
>>  };
>>
>
> I'm not really happy with the way "reserved" field is used by this
> patch: as the field wasn't check for being set to 0, any value could be
> given by userspace (imagine the structure lay on stack). Using it now
> could be dangerous. It must be double checked.

We  are only allowing user space applications to program certain
aspects in the behavior
of their own QPs, no risk to the system/kernel state.

We've done it very successfully in the past with adding the link_layer field
to struct ib_uverbs_query_port_resp as part of the RoCE story instead of a
reserved field.

Or
--
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 for-next 0/2] Add creation flags for user space QPs

[Christoph Lameter]

On Sun, 14 Sep 2014, Or Gerlitz wrote:

> This small series addresses a long time (2008..) pain point, namely
> the inability to specify creation flags for user-space QPs. We use
> the existing uverbs command and a reserved field there to specify the flags.


Ok that is the kernel portion. How do I test this? I would need updates to
libibverbs right?

--
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 for-next 0/2] Add creation flags for user space QPs

[Or Gerlitz]

On Tue, Sep 16, 2014 at 5:04 PM, Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org> wrote:

> Ok that is the kernel portion. How do I test this? I would need updates to
> libibverbs right?

yep, it's very common that such development goes bottom up, with the
kernel part (which is the top pain  point here) posted 1st. Once this
is accepted we'll be working on the user space part.

To test it now, you need to patch your libibverbs to set the flag
(say) when a UD QP is created, run your app which subscribes to a
multicast group they also send over packets, and see that there are no
loopbacks, wan't me to provide such libibverbs patch for you to test
with?

Or.
--
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 for-next 0/2] Add creation flags for user space QPs

[Christoph Lameter]

On Tue, 16 Sep 2014, Or Gerlitz wrote:

> To test it now, you need to patch your libibverbs to set the flag
> (say) when a UD QP is created, run your app which subscribes to a
> multicast group they also send over packets, and see that there are no
> loopbacks, wan't me to provide such libibverbs patch for you to test
> with?

Yes, please.

--
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 for-next 1/2] IB/uverbs: Add QP creation flags, allow blocking UD multicast loopback

[Yann Droneaud]

Hi,

Le lundi 15 septembre 2014 à 20:36 +0300, Or Gerlitz a écrit :
> On Mon, Sep 15, 2014 at 7:52 PM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> >> --- a/include/uapi/rdma/ib_user_verbs.h
> >> +++ b/include/uapi/rdma/ib_user_verbs.h
> >> @@ -470,7 +470,7 @@ struct ib_uverbs_create_qp {
> >>       __u8  sq_sig_all;
> >>       __u8  qp_type;
> >>       __u8  is_srq;
> >> -     __u8  reserved;
> >> +     __u8  create_flags;
> >>       __u64 driver_data[0];
> >>  };
> >>
> >
> > I'm not really happy with the way "reserved" field is used by this
> > patch: as the field wasn't check for being set to 0, any value could be
> > given by userspace (imagine the structure lay on stack). Using it now
> > could be dangerous. It must be double checked.
> 
> We  are only allowing user space applications to program certain
> aspects in the behavior
> of their own QPs, no risk to the system/kernel state.
> 

I've checked the implementation on the most common userspace code
accessing the uverbs API, libibverbs, and found the "reserved" field is
explicitely cleared in:

ibv_cmd_create_qp_ex():

c7e3e61052dd7 (Sean Hefty       2013-08-01 18:04:16 +0300  651)         cmd->reserved        = 0;

ibv_cmd_create_qp():

0e0604213ed79 (Roland Dreier    2006-10-04 23:57:10 +0000  726)         cmd->reserved        = 0;

Latter commit is part of libibverbs-1.1-rc1, so before libibverbs-1.1,
reserved field was not cleared.

For example, mlx4_create_qp() allocate the ibv_create_qp structure on
stack and doesn't clear reserved field:

^d049a1279b82 (Roland Dreier    2007-04-09 00:49:42 -0700 358)  struct mlx4_create_qp     cmd;

So existing userspace program using libibverbs < 1.1 is likely to break
with newer kernel if reserved field is going to be used.

> We've done it very successfully in the past with adding the link_layer field
> to struct ib_uverbs_query_port_resp as part of the RoCE story instead of a
> reserved field.
> 

ib_uverbs_query_port_resp is the opposite side: the kernel is adding stuff
where older userspace code don't expect to found anything, so this extra
information is skipped by such pre-existing program.

Using an unused field in the request has more chance to break than using
an extra field in the response.

Again:

"Check all unused fields and flags and all the padding for whether it's 0,
 and reject the ioctl if that's not the case. Otherwise your nice plan for
 future extensions is going right down the gutters since someone will
 submit an ioctl struct with random stack garbage in the yet unused parts.
 Which then bakes in the ABI that those fields can never be used for
 anything else but garbage."

As the "reserved" field was never check for being 0 in kernel side,
ensuring it could be used in the future for other purpose, we're in
a situation where "reserved" field is garbage when using older
libibverbs or other userspace software that can address uverbs
by-passing libibverbs.

That's likely to break existing userspace application.

Regards.

-- 
Yann Droneaud
OPTEYA


--
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 for-next 1/2] IB/uverbs: Add QP creation flags, allow blocking UD multicast loopback

[Or Gerlitz]

On 9/15/2014 7:52 PM, Yann Droneaud wrote:
> Hi,
>
> Le dimanche 14 septembre 2014 à 16:20 +0300, Or Gerlitz a écrit :
> Commit identifier is too short, 12 digits is the norm. 

OK, will fix

>> As a result, applications who send and recieve over the same QP to
>> the same multicast group get all their packets bouncded back to them,
>> which is terribly bad performance wise.
>>
>> To fix this long standing issue, add the ability to provide QP
>> creation flags through uverbs.
>>
>> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> ---
>>   drivers/infiniband/core/uverbs_cmd.c |   35 ++++++++++++++++++++++++++++++++-
>>   include/uapi/rdma/ib_user_verbs.h    |    2 +-
>>   2 files changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
>> index 0600c50..1ad489c 100644
>> --- a/drivers/infiniband/core/uverbs_cmd.c
>> +++ b/drivers/infiniband/core/uverbs_cmd.c
>> @@ -1579,6 +1579,31 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file,
>>   	return in_len;
>>   }
>>   
>> +enum ib_uverbs_qp_create_flags {
>> +	IB_UVERBS_QP_CREATE_LSO	    = 0,
>> +	IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK = 1,
>> +	IB_UVERBS_SUPPORTED_FLAGS
>> +};
> IB_UVERBS_QP_CREATE_LSO and IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK
> should moved in include/uapi/rdma/ib_user_verbs.h.
>
> If this is going to be flags, these values might be OR-ed together ?
> Then the userspace values should be defined like the kernel ones
> (see commit 47ee1b9f2e7b):
>
> +enum ib_uverbs_qp_create_flags {
> +	IB_UVERBS_QP_CREATE_LSO	    =                  1 << 0,
> +	IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK = 1 << 1,
> +}
>

Some misunderstanding here.. enum ib_uverbs_qp_create_flags is internal 
kernel construct used
to check the flags provided by user space (the _flags suffix is 
confusing, will remove it). The actual flags
values to be used by user space are shifted values (e.g 1 << 
IB_UVERBS_QP_CREATE_LSO).

As the whole set of flags/enum/attribute values moved between libibverbs 
to the kernel, this will be defined
in libibverbs header (take a look on include/uapi/rdma/ib_user_verbs.h 
it doesn't have the enumeration for event types defined).


> IB_UVERBS_SUPPORTED_FLAGS must be kept in this module, but defined
> as:
>
> #define IB_UVERBS_QP_CREATE_FLAGS_ALL (IB_UVERBS_QP_CREATE_LSO | \
> 				       IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK)
>
>> +
>> +static int ib_uverbs_create_qp_trans(u8 u_flags)
>> +{
>> +	int i;
>> +	int res;
>> +	static const enum ib_qp_create_flags ib_uverbs_qp_create_flags[IB_UVERBS_SUPPORTED_FLAGS] = {
>> +		[IB_UVERBS_QP_CREATE_LSO] = IB_QP_CREATE_IPOIB_UD_LSO,
>> +		[IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK] = IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK,
>> +	};
>> +
>> +	if (u_flags & ~((1 << IB_UVERBS_SUPPORTED_FLAGS) - 1))
>> +		return -1;
>> +
> 	if (u_flags & ~(u8)(IB_UVERBS_QP_CREATE_FLAGS_ALL))
> 	   return -1;
>
>> +	for (i = 0; i < IB_UVERBS_SUPPORTED_FLAGS; i++)
>> +		if (u_flags & (1 << i))
>> +			res |= ib_uverbs_qp_create_flags[i];
>> +
>> +	return res;
>> +}
>> +
> This function can be replaced by compile time assert:
>
> BUILD_BUG_ON(IB_UVERBS_QP_CREATE_LSO != IB_QP_CREATE_IPOIB_UD_LSO);
> BUILD_BUG_ON(IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK !=
> IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK);
>
> This way the values can be used without conversion.

We prefer to have larger flexibility here which we can do with run time 
checks (this is slow path) and remain with the practice suggested by the 
original patch.


Or.
--
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 for-next 1/2] IB/uverbs: Add QP creation flags, allow blocking UD multicast loopback

[Or Gerlitz]

On 9/18/2014 10:25 AM, Yann Droneaud wrote:
> Le lundi 15 septembre 2014 à 20:36 +0300, Or Gerlitz a écrit :
>> On Mon, Sep 15, 2014 at 7:52 PM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
>>>> --- a/include/uapi/rdma/ib_user_verbs.h
>>>> +++ b/include/uapi/rdma/ib_user_verbs.h
>>>> @@ -470,7 +470,7 @@ struct ib_uverbs_create_qp {
>>>>        __u8  sq_sig_all;
>>>>        __u8  qp_type;
>>>>        __u8  is_srq;
>>>> -     __u8  reserved;
>>>> +     __u8  create_flags;
>>>>        __u64 driver_data[0];
>>>>   };
>>>>
>>> I'm not really happy with the way "reserved" field is used by this
>>> patch: as the field wasn't check for being set to 0, any value could be
>>> given by userspace (imagine the structure lay on stack). Using it now
>>> could be dangerous. It must be double checked.
>> We are only allowing user space applications to program certain
>> aspects in the behavior of their own QPs, no risk to the system/kernel state.
>>
> I've checked the implementation on the most common userspace code
> accessing the uverbs API, libibverbs, and found the "reserved" field is
> explicitely cleared in:
>
> ibv_cmd_create_qp_ex():
>
> c7e3e61052dd7 (Sean Hefty       2013-08-01 18:04:16 +0300  651)         cmd->reserved        = 0;
>
> ibv_cmd_create_qp():
>
> 0e0604213ed79 (Roland Dreier    2006-10-04 23:57:10 +0000  726)         cmd->reserved        = 0;
>
> Latter commit is part of libibverbs-1.1-rc1, so before libibverbs-1.1,
> reserved field was not cleared.
>
> For example, mlx4_create_qp() allocate the ibv_create_qp structure on
> stack and doesn't clear reserved field:
>
> ^d049a1279b82 (Roland Dreier    2007-04-09 00:49:42 -0700 358)  struct mlx4_create_qp     cmd;
>

As  you noted, in libibverbs this field is cleared since 2006 for all 
use cases of this uverbs command . The libmlx4 code pointer you provided 
is calling into libibverbs code (ibv_cmd_create_qp) which does the zeroing.


> So existing userspace program using libibverbs < 1.1 is likely to break with newer kernel if reserved field is going to be used.

as I wrote earlier, not break.

>> We've done it very successfully in the past with adding the link_layer field
>> to struct ib_uverbs_query_port_resp as part of the RoCE story instead of a
>> reserved field.
>>
> ib_uverbs_query_port_resp is the opposite side: the kernel is adding stuff
> where older userspace code don't expect to found anything, so this extra
> information is skipped by such pre-existing program.

yep, the example wasn't 1:1 to this case, understood.

> Using an unused field in the request has more chance to break than using
> an extra field in the response.
>
> Again:
>
> "Check all unused fields and flags and all the padding for whether it's 0,
>   and reject the ioctl if that's not the case. Otherwise your nice plan for
>   future extensions is going right down the gutters since someone will
>   submit an ioctl struct with random stack garbage in the yet unused parts.
>   Which then bakes in the ABI that those fields can never be used for
>   anything else but garbage."
>
> As the "reserved" field was never check for being 0 in kernel side,
> ensuring it could be used in the future for other purpose, we're in
> a situation where "reserved" field is garbage when using older
> libibverbs or other userspace software that can address uverbs
> by-passing libibverbs.
>
> That's likely to break existing userspace application.

Again, applications that for some reason don't zero out this field, will 
get their QP to potentially support some features which they will not use.

Or.



--
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 for-next 1/2] IB/uverbs: Add QP creation flags, allow blocking UD multicast loopback

[Yann Droneaud]

Hi,

Le jeudi 18 septembre 2014 à 15:51 +0300, Or Gerlitz a écrit :
> On 9/15/2014 7:52 PM, Yann Droneaud wrote:
> > Le dimanche 14 septembre 2014 à 16:20 +0300, Or Gerlitz a écrit :
>

[...]

> >> As a result, applications who send and recieve over the same QP to
> >> the same multicast group get all their packets bouncded back to them,
> >> which is terribly bad performance wise.
> >>
> >> To fix this long standing issue, add the ability to provide QP
> >> creation flags through uverbs.
> >>
> >> Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> ---
> >>   drivers/infiniband/core/uverbs_cmd.c |   35 ++++++++++++++++++++++++++++++++-
> >>   include/uapi/rdma/ib_user_verbs.h    |    2 +-
> >>   2 files changed, 34 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> >> index 0600c50..1ad489c 100644
> >> --- a/drivers/infiniband/core/uverbs_cmd.c
> >> +++ b/drivers/infiniband/core/uverbs_cmd.c
> >> @@ -1579,6 +1579,31 @@ ssize_t ib_uverbs_destroy_cq(struct ib_uverbs_file *file,
> >>   	return in_len;
> >>   }
> >>   
> >> +enum ib_uverbs_qp_create_flags {
> >> +	IB_UVERBS_QP_CREATE_LSO	    = 0,
> >> +	IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK = 1,
> >> +	IB_UVERBS_SUPPORTED_FLAGS
> >> +};
> > IB_UVERBS_QP_CREATE_LSO and IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK
> > should moved in include/uapi/rdma/ib_user_verbs.h.
> >
> > If this is going to be flags, these values might be OR-ed together ?
> > Then the userspace values should be defined like the kernel ones
> > (see commit 47ee1b9f2e7b):
> >
> > +enum ib_uverbs_qp_create_flags {
> > +	IB_UVERBS_QP_CREATE_LSO	    =                  1 << 0,
> > +	IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK = 1 << 1,
> > +}
> >
> 
> Some misunderstanding here.. enum ib_uverbs_qp_create_flags is internal 
> kernel construct used
> to check the flags provided by user space (the _flags suffix is 
> confusing, will remove it). The actual flags
> values to be used by user space are shifted values (e.g 1 << 
> IB_UVERBS_QP_CREATE_LSO).
> 

OK, I haven't pay attention to the shift in the conversion function.

I would still prefer having the flag value be declared, instead of the 
shift offset.

> As the whole set of flags/enum/attribute values moved between libibverbs 
> to the kernel, this will be defined
> in libibverbs header (take a look on include/uapi/rdma/ib_user_verbs.h 
> it doesn't have the enumeration for event types defined).
> 
> 

OK.

(BTW, not having the flags/enum/attributes in 
 include/uapi/rdma/ib_user_verbs.h is a pity, as the header 
 is supposed to expose kernel API/ABI to userspace).

> > IB_UVERBS_SUPPORTED_FLAGS must be kept in this module, but defined
> > as:
> >
> > #define IB_UVERBS_QP_CREATE_FLAGS_ALL (IB_UVERBS_QP_CREATE_LSO | \
> > 				       IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK)
> >
> >> +
> >> +static int ib_uverbs_create_qp_trans(u8 u_flags)
> >> +{
> >> +	int i;
> >> +	int res;
> >> +	static const enum ib_qp_create_flags ib_uverbs_qp_create_flags[IB_UVERBS_SUPPORTED_FLAGS] = {
> >> +		[IB_UVERBS_QP_CREATE_LSO] = IB_QP_CREATE_IPOIB_UD_LSO,
> >> +		[IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK] = IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK,
> >> +	};
> >> +
> >> +	if (u_flags & ~((1 << IB_UVERBS_SUPPORTED_FLAGS) - 1))
> >> +		return -1;
> >> +
> > 	if (u_flags & ~(u8)(IB_UVERBS_QP_CREATE_FLAGS_ALL))
> > 	   return -1;
> >
> >> +	for (i = 0; i < IB_UVERBS_SUPPORTED_FLAGS; i++)
> >> +		if (u_flags & (1 << i))
> >> +			res |= ib_uverbs_qp_create_flags[i];
> >> +
> >> +	return res;
> >> +}
> >> +
> > This function can be replaced by compile time assert:
> >
> > BUILD_BUG_ON(IB_UVERBS_QP_CREATE_LSO != IB_QP_CREATE_IPOIB_UD_LSO);
> > BUILD_BUG_ON(IB_UVERBS_QP_CREATE_BLOCK_MULTICAST_LOOPBACK !=
> > IB_QP_CREATE_BLOCK_MULTICAST_LOOPBACK);
> >
> > This way the values can be used without conversion.
> 
> We prefer to have larger flexibility here which we can do with run time 
> checks (this is slow path) and remain with the practice suggested by the 
> original patch.
> 

I don't buy this argument: it's not clear for me that there's an 
advantage in such complexity. If the internal kernel flags are going to
change, the function might be needed, but until it happen, it's not.

What I've suggested is a common construction, see

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/signalfd.c?id=v3.17-rc5#n262
fs/signalfd.c:signalfd()

        /* Check the SFD_* constants for consistency.  */
        BUILD_BUG_ON(SFD_CLOEXEC != O_CLOEXEC);
        BUILD_BUG_ON(SFD_NONBLOCK != O_NONBLOCK);

        if (flags & ~(SFD_CLOEXEC | SFD_NONBLOCK))
                return -EINVAL;


http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/signalfd.c?id=v3.17-rc5#n381
fs/timerfd.c:timerfd()

        /* Check the TFD_* constants for consistency.  */
        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);

        if ((flags & ~TFD_CREATE_FLAGS) ||


See also perf_event_open() for the check against recognized flags.


Regards.

-- 
Yann Droneaud
OPTEYA


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