diff mbox series

[v2,2/2] RDMA/SRP: calculate max_it_iu_size if remote max_it_iu length available

Message ID 20190917032421.13000-2-honli@redhat.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/2] IB/srp: Add parse function for maximum initiator to target IU size | expand

Commit Message

Honggang LI Sept. 17, 2019, 3:24 a.m. UTC 
From: Honggang Li <honli@redhat.com>

The default maximum immediate size is too big for old srp clients,
which without immediate data support.

According to the SRP and SRP-2 specifications, the IOControllerProfile
attributes for SRP target ports contains the maximum initiator to target
iu length.

The maximum initiator to target iu length can be get from the subnet
manager, such as opensm and opafm. We should calculate the
max_it_iu_size instead of the default value, when remote maximum
initiator to target iu length available.

Signed-off-by: Honggang Li <honli@redhat.com>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Comments

Bart Van Assche Sept. 17, 2019, 5:40 p.m. UTC | #1 
On 9/16/19 8:24 PM, Honggang LI wrote:
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 2eadb038b257..d8dee5900c08 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -76,6 +76,7 @@ static bool register_always = true;
>   static bool never_register;
>   static int topspin_workarounds = 1;
>   static uint32_t srp_opt_max_it_iu_size;
> +static unsigned int srp_max_imm_data;

Each SCSI host can represent a connection to another SRP target, and the 
max_it_iu_size parameter can differ per target. So I think this variable 
should be moved into struct srp_target_port instead of being global.

>   module_param(srp_sg_tablesize, uint, 0444);
>   MODULE_PARM_DESC(srp_sg_tablesize, "Deprecated name for cmd_sg_entries");
> @@ -138,9 +139,9 @@ module_param_named(use_imm_data, srp_use_imm_data, bool, 0644);
>   MODULE_PARM_DESC(use_imm_data,
>   		 "Whether or not to request permission to use immediate data during SRP login.");
>   
> -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.");
> +static unsigned int srp_default_max_imm_data = 8 * 1024;
> +module_param_named(max_imm_data, srp_default_max_imm_data, uint, 0644);
> +MODULE_PARM_DESC(max_imm_data, "Default maximum immediate data size.");

This description might confuse users. How about keeping the name of the 
variable and the module parameter and changing its description into the 
following?

"Maximum immediate data size if max_it_iu_size has not been specified."

>   
>   static unsigned ch_count;
>   module_param(ch_count, uint, 0444);
> @@ -1369,9 +1370,19 @@ static uint32_t srp_max_it_iu_len(int cmd_sg_cnt, bool use_imm_data)
>   		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 (srp_opt_max_it_iu_size == 0) {
> +			srp_max_imm_data = srp_default_max_imm_data;
> +			max_iu_len = max(max_iu_len,
> +			   SRP_IMM_DATA_OFFSET + srp_max_imm_data);
> +		} else {
> +			srp_max_imm_data =
> +			   srp_opt_max_it_iu_size - SRP_IMM_DATA_OFFSET;

Please use as much of a line as possible. I think the recommended style 
in the Linux kernel is as follows:

			srp_max_imm_data = srp_opt_max_it_iu_size -
				SRP_IMM_DATA_OFFSET;

> @@ -3829,6 +3840,8 @@ static ssize_t srp_create_target(struct device *dev,
>   	if (ret < 0)
>   		goto put;
>   
> +	srp_opt_max_it_iu_size = 0;
> +

Static variables that are not initialized explicitly are initialized to 
zero. Since this initialization is redundant, please remove it.

Thanks,

Bart.
Honggang LI Sept. 19, 2019, 3:36 a.m. UTC | #2 
On Tue, Sep 17, 2019 at 10:40:00AM -0700, Bart Van Assche wrote:
> On 9/16/19 8:24 PM, Honggang LI wrote:
> > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> > index 2eadb038b257..d8dee5900c08 100644
> > --- a/drivers/infiniband/ulp/srp/ib_srp.c
> > +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> > @@ -76,6 +76,7 @@ static bool register_always = true;
> >   static bool never_register;
> >   static int topspin_workarounds = 1;
> >   static uint32_t srp_opt_max_it_iu_size;
> > +static unsigned int srp_max_imm_data;
> 
> Each SCSI host can represent a connection to another SRP target, and the
> max_it_iu_size parameter can differ per target. So I think this variable
> should be moved into struct srp_target_port instead of being global.

Yes, will do as you said.

> 
> >   module_param(srp_sg_tablesize, uint, 0444);
> >   MODULE_PARM_DESC(srp_sg_tablesize, "Deprecated name for cmd_sg_entries");
> > @@ -138,9 +139,9 @@ module_param_named(use_imm_data, srp_use_imm_data, bool, 0644);
> >   MODULE_PARM_DESC(use_imm_data,
> >   		 "Whether or not to request permission to use immediate data during SRP login.");
> > -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.");
> > +static unsigned int srp_default_max_imm_data = 8 * 1024;
> > +module_param_named(max_imm_data, srp_default_max_imm_data, uint, 0644);
> > +MODULE_PARM_DESC(max_imm_data, "Default maximum immediate data size.");
> 
> This description might confuse users. How about keeping the name of the
> variable and the module parameter and changing its description into the

Yes, will keep the name of the variable and the module parameter.

> following?
> 
> "Maximum immediate data size if max_it_iu_size has not been specified."

Yes, will use this description.

> 
> >   static unsigned ch_count;
> >   module_param(ch_count, uint, 0444);
> > @@ -1369,9 +1370,19 @@ static uint32_t srp_max_it_iu_len(int cmd_sg_cnt, bool use_imm_data)
> >   		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 (srp_opt_max_it_iu_size == 0) {
> > +			srp_max_imm_data = srp_default_max_imm_data;
> > +			max_iu_len = max(max_iu_len,
> > +			   SRP_IMM_DATA_OFFSET + srp_max_imm_data);
> > +		} else {
> > +			srp_max_imm_data =
> > +			   srp_opt_max_it_iu_size - SRP_IMM_DATA_OFFSET;
> 
> Please use as much of a line as possible. I think the recommended style in
> the Linux kernel is as follows:
> 
> 			srp_max_imm_data = srp_opt_max_it_iu_size -
> 				SRP_IMM_DATA_OFFSET;

The new patch no longer needs this. So, it will not be a problem.

> 
> > @@ -3829,6 +3840,8 @@ static ssize_t srp_create_target(struct device *dev,
> >   	if (ret < 0)
> >   		goto put;
> > +	srp_opt_max_it_iu_size = 0;
> > +
> 
> Static variables that are not initialized explicitly are initialized to
> zero. Since this initialization is redundant, please remove it.

The initialization is not redundant. For example, a srp client connect
to target-1 with 'max_it_iu=1234', and then try to connect target-2
without 'max_it_iu'. At this time srp_opt_max_it_iu_size has garbage
value 1234. That is why we have to initialize it for echo login.

Because srp_opt_max_it_iu_size will be removed in new patch, it
will not be a problem.

thanks
diff mbox series

Patch

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 2eadb038b257..d8dee5900c08 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -76,6 +76,7 @@  static bool register_always = true;
 static bool never_register;
 static int topspin_workarounds = 1;
 static uint32_t srp_opt_max_it_iu_size;
+static unsigned int srp_max_imm_data;
 
 module_param(srp_sg_tablesize, uint, 0444);
 MODULE_PARM_DESC(srp_sg_tablesize, "Deprecated name for cmd_sg_entries");
@@ -138,9 +139,9 @@  module_param_named(use_imm_data, srp_use_imm_data, bool, 0644);
 MODULE_PARM_DESC(use_imm_data,
 		 "Whether or not to request permission to use immediate data during SRP login.");
 
-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.");
+static unsigned int srp_default_max_imm_data = 8 * 1024;
+module_param_named(max_imm_data, srp_default_max_imm_data, uint, 0644);
+MODULE_PARM_DESC(max_imm_data, "Default maximum immediate data size.");
 
 static unsigned ch_count;
 module_param(ch_count, uint, 0444);
@@ -1369,9 +1370,19 @@  static uint32_t srp_max_it_iu_len(int cmd_sg_cnt, bool use_imm_data)
 		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 (srp_opt_max_it_iu_size == 0) {
+			srp_max_imm_data = srp_default_max_imm_data;
+			max_iu_len = max(max_iu_len,
+			   SRP_IMM_DATA_OFFSET + srp_max_imm_data);
+		} else {
+			srp_max_imm_data =
+			   srp_opt_max_it_iu_size - SRP_IMM_DATA_OFFSET;
+			max_iu_len = srp_opt_max_it_iu_size;
+		}
+		pr_debug("srp_max_imm_data = %d, max_iu_len = %d\n",
+			srp_max_imm_data, max_iu_len);
+	}
 
 	return max_iu_len;
 }
@@ -3829,6 +3840,8 @@  static ssize_t srp_create_target(struct device *dev,
 	if (ret < 0)
 		goto put;
 
+	srp_opt_max_it_iu_size = 0;
+
 	ret = srp_parse_options(target->net, buf, target);
 	if (ret)
 		goto out;