Re: [PATCH for-next 1/2] IB/uverbs: Add QP creation flags, allow blocking UD multicast loopback

[Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

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