Re: [patch v4 2/2] RDMA/srp: calculate max_it_iu_size if remote max_it_iu length available

[Bart Van Assche <bvanassche@xxxxxxx>]

On 9/22/19 11:29 PM, Honggang LI wrote:
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index b829dab0df77..b3bf5d552de9 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -139,7 +139,7 @@ MODULE_PARM_DESC(use_imm_data,
>   
>   static unsigned int srp_max_imm_data = 8 * 1024;
>   module_param_named(max_imm_data, srp_max_imm_data, uint, 0644);
> -MODULE_PARM_DESC(max_imm_data, "Maximum immediate data size.");
> +MODULE_PARM_DESC(max_imm_data, "Maximum immediate data size if max_it_iu_size has not been specified.");
>   
>   static unsigned ch_count;
>   module_param(ch_count, uint, 0444);
> @@ -1362,15 +1362,23 @@ static void srp_terminate_io(struct srp_rport *rport)
>   }
>   
>   /* Calculate maximum initiator to target information unit length. */
> -static uint32_t srp_max_it_iu_len(int cmd_sg_cnt, bool use_imm_data)
> +static uint32_t srp_max_it_iu_len(int cmd_sg_cnt, bool use_imm_data,
> +				  uint32_t max_it_iu_size)
>   {
>   	uint32_t max_iu_len = sizeof(struct srp_cmd) + SRP_MAX_ADD_CDB_LEN +
>   		sizeof(struct srp_indirect_buf) +
>   		cmd_sg_cnt * sizeof(struct srp_direct_buf);
>   
> -	if (use_imm_data)
> -		max_iu_len = max(max_iu_len, SRP_IMM_DATA_OFFSET +
> -				 srp_max_imm_data);
> +	if (use_imm_data) {
> +		if (max_it_iu_size == 0) {
> +			max_iu_len = max(max_iu_len,
> +			   SRP_IMM_DATA_OFFSET + srp_max_imm_data);
> +		} else {
> +			max_iu_len = max_it_iu_size;
> +		}
> +	}
> +
> +	pr_debug("max_iu_len = %d\n", max_iu_len);
>   
>   	return max_iu_len;
>   }

Thinking further about this, this removes the ability for users to limit 
immediate data to a certain number of bytes. I think that's a step back. 
How about not modifying the description of max_imm_data and to use the 
following approach in srp_max_it_iu_len() for calculating max_iu_len?

uint32_t max_iu_len = sizeof(struct srp_cmd) + SRP_MAX_ADD_CDB_LEN +
		sizeof(struct srp_indirect_buf) +
		cmd_sg_cnt * sizeof(struct srp_direct_buf);
if (use_imm_data)
	max_iu_len = max(max_iu_len, SRP_IMM_DATA_OFFSET +
			 srp_max_imm_data);
if (max_it_iu_size)
	max_iu_len = min(max_iu_len, max_it_iu_size);

Thanks,

Bart.