Re: [PATCH rdma-core 2/3] ibverbs: Add support for packet pacing

[Yishai Hadas <yishaih-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On 1/9/2017 5:38 PM, Jason Gunthorpe wrote:
> On Sun, Jan 08, 2017 at 06:32:27PM +0200, Yishai Hadas wrote:
>> +int ibv_cmd_modify_qp_ex(struct ibv_qp *qp, struct ibv_qp_attr *attr,
>> +			 int attr_mask, struct ibv_modify_qp_ex *cmd,
>> +			 size_t cmd_core_size, size_t cmd_size,
>> +			 struct ibv_modify_qp_resp_ex *resp,
>> +			 size_t resp_core_size, size_t resp_size)
>> +{
>> +	if (resp_core_size < offsetof(struct ibv_modify_qp_resp_ex,
>> +			     response_length) + sizeof(resp->response_length))
>> +		return EINVAL;
>> +
>> +	IBV_INIT_CMD_RESP_EX_V(cmd, cmd_core_size, cmd_size, MODIFY_QP_EX,
>> +			       resp, resp_core_size, resp_size);
>> +
>> +	copy_modify_qp_fields(qp, attr, attr_mask, &cmd->base);
>> +
>> +	if (attr_mask & IBV_QP_RATE_LIMIT) {
>> +		if (cmd_size >= offsetof(struct ibv_modify_qp_ex, rate_limit) +
>> +		    sizeof(cmd->rate_limit))
>> +			cmd->rate_limit = attr->rate_limit;
>> +		else
>> +			return EINVAL;
>> +	}
>
> cmd->rate_limit is not being 0'd if it isn't set

The 'cmd' was fully set to 0'd by the provider, see usage in the last patch.

In case we want to do it in this layer I would vote on using memset at 
the beginning of that function to prevent repeating this for any new 
field. Your last patch in that area could do it as well.

>> @@ -829,7 +829,8 @@ enum ibv_qp_attr_mask {
>>  	IBV_QP_MAX_DEST_RD_ATOMIC	= 1 << 17,
>>  	IBV_QP_PATH_MIG_STATE		= 1 << 18,
>>  	IBV_QP_CAP			= 1 << 19,
>> -	IBV_QP_DEST_QPN			= 1 << 20
>> +	IBV_QP_DEST_QPN			= 1 << 20,
>> +	IBV_QP_RATE_LIMIT		= 1 << 25,
>>  };
>
> Why 25? 21 I think.

To match kernel, 21-24 are reserved.

>
>>  enum ibv_qp_state {
>> @@ -875,6 +876,7 @@ struct ibv_qp_attr {
>>  	uint8_t			rnr_retry;
>>  	uint8_t			alt_port_num;
>>  	uint8_t			alt_timeout;
>> +	uint32_t		rate_limit;
>>  };
>
> Nope, this is returned by query_qp so you cannot just extend it.
It's safe, please see below.

>
> You are supporting query_qp, right?
At the moment the kernel doesn't support an extended command for 
query_qp so user space doesn't get and touch this field.

Once support will be added in the kernel, the attr_mask field of 
ibv_query_qp should be used to indicate whether application asked for 
that field (i.e. IBV_QP_RATE_LIMIT) and supplied an input structure that 
has memory to hold this output so it's safe to extend the above 
structure. Same idea was used in this series for accessing this field in 
modify_qp as an input.


We can consider adding an extra check in ibv_cmd_query_qp as part of 
this series to check whether IBV_QP_RATE_LIMIT or higher bits were 
asked, if yes return an error as there is no support for that value 
without using an extended command, makes sense ?

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

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