diff mbox series

srp_daemon: Use maximum initiator to target IU size

Message ID 20191018044104.21353-1-honli@redhat.com (mailing list archive)
State Superseded
Headers show
Series srp_daemon: Use maximum initiator to target IU size | expand

Commit Message

Honggang LI Oct. 18, 2019, 4:41 a.m. UTC 
From: Honggang Li <honli@redhat.com>

"ib_srp.ko" module with immediate data support use (8 * 1024)
as default immediate data size. When immediate data support enabled
for "ib_srp.ko" client, the default maximum initiator to target IU
size will greater than (8 * 1024). That means it also greater than
the default maximum initiator to target IU size set by old
"ib_srpt.ko" module, which does not support immediate data.

Signed-off-by: Honggang Li <honli@redhat.com>
---
 srp_daemon/srp_daemon.c                | 26 ++++++++++++++++++++++++--
 srp_daemon/srp_daemon.h                |  1 +
 srp_daemon/srp_daemon.sh.in            |  2 +-
 srp_daemon/srp_daemon_port@.service.in |  2 +-
 4 files changed, 27 insertions(+), 4 deletions(-)

Comments

Honggang LI Oct. 18, 2019, 3:22 p.m. UTC | #1 
On Thu, Oct 17, 2019 at 09:57:08PM -0700, Bart Van Assche wrote:
> On 2019-10-17 21:41, Honggang LI wrote:
> > +	if (config->print_max_it_iu_size) {
> > +		len += snprintf(target_config_str+len,
> > +				sizeof(target_config_str) - len,
> > +				",max_it_iu_size=%d",
> > +				be32toh(target->ioc_prof.send_size));
> > +
> > +		if (len >= sizeof(target_config_str)) {
> > +			pr_err("Target config string is too long, ignoring target\n");
> > +			closedir(dir);
> > +			return -1;
> > +		}
> > +	}
> 
> Hi Honggang,
> 
> I think this patch will make srp_daemon incompatible with versions of
                                          ^^^^^^^^^^^^

Yes, it compatible with old ib_srp kernel driver that do not support
the max_it_iu_size parameter. It will trigger a kernel message like
this:

ib_srp: unknown parameter or missing value 'max_it_iu_size=8276' in target creation request

But SRP login will continue and success.

> the ib_srp kernel driver that do not support the max_it_iu_size
> parameter and also that that's unacceptable. How about the following
> approach:
> * Do not add a new command-line option.

I suggest to use a new command-line, we can avoid the warning message by
remove the parameter from the srp_daemon commad.

> * Add max_it_iu_size at the end. I think that approach will trigger a

If we hard-code max_it_iu_size at the end, users with old srpt module
will not able to avoid the warn message. They have to use srp_daemon
without this patch.

thanks

> warning with older versions of the SRP kernel driver but also that it
> won't break SRP login.
> 
> Thanks,
> 
> Bart.
Bart Van Assche Oct. 18, 2019, 5:10 p.m. UTC | #2 
On 10/18/19 8:22 AM, Honggang LI wrote:
> [ ... ]

Hi Honggang,

The approach of your patch seems suboptimal to me. I think it would be 
cumbersome for users to have to modify the srp_daemon_port@.service file 
to suppress a kernel warning or to pass the max_it_iu_size parameter 
during login.

Had you noticed that the SRP initiator stops parsing parameters if it 
encounters an unrecognized parameter?

 From ib_srp.c, function srp_parse_options():

	default:
		pr_warn("unknown parameter or missing value '%s' in"
			" target creation request\n", p);
			goto out;
		}

The "goto out" breaks out of the while loop that parses options. We 
cannot change this behavior in existing versions of the SRP initiator 
kernel driver. In other words, if the max_it_iu_size parameter is not 
specified at the very end of the login string it will cause some login 
parameters to be ignored.

I think we should use one of the following two approaches:
* Not add a new command-line option to srp_daemon and add the
   max_it_iu_size at the very end of the login string.
* Modify the ib_srp driver such that it exports the login parameters
   through sysfs. Modify srp_daemon such that it only specifies the
   max_it_iu_size parameter if it finds that parameter in the list of
   supported parameters.

Thanks,

Bart.
Honggang LI Oct. 22, 2019, 7 a.m. UTC | #3 
On Fri, Oct 18, 2019 at 10:10:05AM -0700, Bart Van Assche wrote:
> On 10/18/19 8:22 AM, Honggang LI wrote:
> > [ ... ]
> 
> Hi Honggang,
> 
> The approach of your patch seems suboptimal to me. I think it would be
> cumbersome for users to have to modify the srp_daemon_port@.service file to
> suppress a kernel warning or to pass the max_it_iu_size parameter during
> login.
> 
> Had you noticed that the SRP initiator stops parsing parameters if it
> encounters an unrecognized parameter?
> 
> From ib_srp.c, function srp_parse_options():
> 
> 	default:
> 		pr_warn("unknown parameter or missing value '%s' in"
> 			" target creation request\n", p);
> 			goto out;
> 		}
> 
> The "goto out" breaks out of the while loop that parses options. We cannot
> change this behavior in existing versions of the SRP initiator kernel
> driver. In other words, if the max_it_iu_size parameter is not specified at
> the very end of the login string it will cause some login parameters to be
> ignored.

I see.

> 
> I think we should use one of the following two approaches:
> * Not add a new command-line option to srp_daemon and add the
>   max_it_iu_size at the very end of the login string.

Yes, I will remove the new command-line and append max_it_iu_size at
the very end of login string.

> * Modify the ib_srp driver such that it exports the login parameters

This hints me to check the value of
/sys/module/ib_srp/parameters/use_imm_data .

The regression issue was introduced because of using immediate data.
The new patch will append max_it_iu_size only when use_imm_data enabled.

For old ib_srp client does not support immediate date, the new patch will
not use max_it_iu_size. It will not trigger the kernel warning message
of unsupported parameter.

Please see this patch.

diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
index f0bcf923..24451b87 100644
--- a/srp_daemon/srp_daemon.c
+++ b/srp_daemon/srp_daemon.c
@@ -402,6 +402,28 @@ static int is_enabled_by_rules_file(struct target_details *target)
 }
 
 
+static bool use_imm_data(void)
+{
+#ifdef __linux__
+	bool ret = false;
+	char flag = 0;
+	int cnt;
+	int fd = open("/sys/module/ib_srp/parameters/use_imm_data", O_RDONLY);
+
+	if (fd < 0)
+		return false;
+	cnt = read(fd, &flag, 1);
+	if (cnt != 1)
+		return false;
+
+	if (!strncmp(&flag, "Y", 1))
+		ret = true;
+	close(fd);
+	return ret;
+#else
+	return false;
+#endif
+}
 
 static int add_non_exist_target(struct target_details *target)
 {
@@ -581,6 +603,19 @@ static int add_non_exist_target(struct target_details *target)
 		}
 	}
 
+	if (use_imm_data()) {
+		len += snprintf(target_config_str+len,
+			sizeof(target_config_str) - len,
+			",max_it_iu_size=%d",
+			be32toh(target->ioc_prof.send_size));
+
+		if (len >= sizeof(target_config_str)) {
+			pr_err("Target config string is too long, ignoring target\n");
+			closedir(dir);
+			return -1;
+		}
+	}
+
 	target_config_str[len] = '\0';
 
 	pr_cmd(target_config_str, not_connected);
@@ -1360,6 +1395,10 @@ static void print_config(struct config_t *conf)
 		printf(" Print initiator_ext\n");
 	else
 		printf(" Do not print initiator_ext\n");
+	if (use_imm_data())
+		printf(" Print maximum initiator to target IU size\n");
+	else
+		printf(" Do not print maximum initiator to target IU size\n");
 	if (conf->recalc_time)
 		printf(" Performs full target rescan every %d seconds\n", conf->recalc_time);
 	else
Bart Van Assche Oct. 22, 2019, 10:10 p.m. UTC | #4 
On 2019-10-22 00:00, Honggang LI wrote:
> +static bool use_imm_data(void)
> +{
> +#ifdef __linux__
> +	bool ret = false;
> +	char flag = 0;
> +	int cnt;
> +	int fd = open("/sys/module/ib_srp/parameters/use_imm_data", O_RDONLY);
> +
> +	if (fd < 0)
> +		return false;
> +	cnt = read(fd, &flag, 1);
> +	if (cnt != 1)
> +		return false;
> +
> +	if (!strncmp(&flag, "Y", 1))
> +		ret = true;
> +	close(fd);
> +	return ret;
> +#else
> +	return false;
> +#endif
> +}

There is already plenty of Linux-specific code in srp_daemon. The #ifdef
__linux__ / #endif guard does not seem useful to me.

There is a file descriptor leak in the above function, namely if read()
returns another value than 1.

The use_imm_data kernel module parameter was introduced in kernel v5.0
(commit 882981f4a411; "RDMA/srp: Add support for immediate data"). The
max_it_iu_size will be introduced in kernel v5.5 (commit 547ed331bbe8;
"RDMA/srp: Add parse function for maximum initiator to target IU size").

So the above check will help for kernel versions before v5.0 but not for
kernel versions [v5.0..v5.5). If that is really what you want, please
explain this in a comment above the use_imm_data() function.

Thanks,

Bart.
Honggang LI Oct. 23, 2019, 3:06 a.m. UTC | #5 
On Tue, Oct 22, 2019 at 03:10:26PM -0700, Bart Van Assche wrote:
> On 2019-10-22 00:00, Honggang LI wrote:
> > +static bool use_imm_data(void)
> > +{
> > +#ifdef __linux__
> > +	bool ret = false;
> > +	char flag = 0;
> > +	int cnt;
> > +	int fd = open("/sys/module/ib_srp/parameters/use_imm_data", O_RDONLY);
> > +
> > +	if (fd < 0)
> > +		return false;
> > +	cnt = read(fd, &flag, 1);
> > +	if (cnt != 1)
> > +		return false;
> > +
> > +	if (!strncmp(&flag, "Y", 1))
> > +		ret = true;
> > +	close(fd);
> > +	return ret;
> > +#else
> > +	return false;
> > +#endif
> > +}
> 
> There is already plenty of Linux-specific code in srp_daemon. The #ifdef
> __linux__ / #endif guard does not seem useful to me.

Will delete the guard.

> 
> There is a file descriptor leak in the above function, namely if read()
> returns another value than 1.

Yes, will fix it.

> 
> The use_imm_data kernel module parameter was introduced in kernel v5.0
> (commit 882981f4a411; "RDMA/srp: Add support for immediate data"). The
> max_it_iu_size will be introduced in kernel v5.5 (commit 547ed331bbe8;
> "RDMA/srp: Add parse function for maximum initiator to target IU size").
> 
> So the above check will help for kernel versions before v5.0 but not for
> kernel versions [v5.0..v5.5). 

Yes, you are right. The patch does not fix the issue for kernel
versions [v5.0..v5.5). But it also does not do anything bad for
kernel versions before v5.5 (commit 547ed331bbe8). It will fix
the issue for kernel after 547ed331bbe8.

> If that is really what you want, please
> explain this in a comment above the use_imm_data() function.

Yes, will add a comment for it.

thanks
Honggang LI Oct. 23, 2019, 3:33 p.m. UTC | #6 
On Wed, Oct 23, 2019 at 11:06:44AM +0800, Honggang LI wrote:
> On Tue, Oct 22, 2019 at 03:10:26PM -0700, Bart Van Assche wrote:
> > On 2019-10-22 00:00, Honggang LI wrote:
> > > +static bool use_imm_data(void)
> > > +{
> > > +#ifdef __linux__
> > > +	bool ret = false;
> > > +	char flag = 0;
> > > +	int cnt;
> > > +	int fd = open("/sys/module/ib_srp/parameters/use_imm_data", O_RDONLY);
> > > +
> > > +	if (fd < 0)
> > > +		return false;
> > > +	cnt = read(fd, &flag, 1);
> > > +	if (cnt != 1)
> > > +		return false;
> > > +
> > > +	if (!strncmp(&flag, "Y", 1))
> > > +		ret = true;
> > > +	close(fd);
> > > +	return ret;
> > > +#else
> > > +	return false;
> > > +#endif
> > > +}
> > 
> > There is already plenty of Linux-specific code in srp_daemon. The #ifdef
> > __linux__ / #endif guard does not seem useful to me.
> 
> Will delete the guard.
> 
> > 
> > There is a file descriptor leak in the above function, namely if read()
> > returns another value than 1.
> 
> Yes, will fix it.
> 
> > 
> > The use_imm_data kernel module parameter was introduced in kernel v5.0
> > (commit 882981f4a411; "RDMA/srp: Add support for immediate data"). The
> > max_it_iu_size will be introduced in kernel v5.5 (commit 547ed331bbe8;
> > "RDMA/srp: Add parse function for maximum initiator to target IU size").
> > 
> > So the above check will help for kernel versions before v5.0 but not for
> > kernel versions [v5.0..v5.5). 
> 
> Yes, you are right. The patch does not fix the issue for kernel
> versions [v5.0..v5.5). But it also does not do anything bad for
> kernel versions before v5.5 (commit 547ed331bbe8). It will fix
> the issue for kernel after 547ed331bbe8.

Well, for kernel versions [v5.0..v5.5), this patch will cause kernel to
emit warning message of unknown parameter. It seems we need add a flag
like this for ib_srp module:


diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index b7f7a5f7bd98..96434f743a91 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -74,6 +74,7 @@ static bool allow_ext_sg;
 static bool prefer_fr = true;
 static bool register_always = true;
 static bool never_register;
+static bool has_max_it_iu_size = true;
 static int topspin_workarounds = 1;
 
 module_param(srp_sg_tablesize, uint, 0444);
@@ -103,6 +104,10 @@ module_param(register_always, bool, 0444);
 MODULE_PARM_DESC(register_always,
 		 "Use memory registration even for contiguous memory regions");
 
+module_param(has_max_it_iu_size, bool, 0444);
+MODULE_PARM_DESC(has_max_it_iu_size,
+		  "Indicate the module supports max_it_iu_size login parameter");
+
 module_param(never_register, bool, 0444);
 MODULE_PARM_DESC(never_register, "Never register memory");
 
==============
Then, srp_daemon should check file "/sys/module/ib_srp/parameters/has_max_it_iu_size"
instead of "/sys/module/ib_srp/parameters/use_imm_data".
Bart Van Assche Oct. 24, 2019, 2:13 a.m. UTC | #7 
On 2019-10-23 08:33, Honggang LI wrote:
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index b7f7a5f7bd98..96434f743a91 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -74,6 +74,7 @@ static bool allow_ext_sg;
>  static bool prefer_fr = true;
>  static bool register_always = true;
>  static bool never_register;
> +static bool has_max_it_iu_size = true;
>  static int topspin_workarounds = 1;
>  
>  module_param(srp_sg_tablesize, uint, 0444);
> @@ -103,6 +104,10 @@ module_param(register_always, bool, 0444);
>  MODULE_PARM_DESC(register_always,
>  		 "Use memory registration even for contiguous memory regions");
>  
> +module_param(has_max_it_iu_size, bool, 0444);
> +MODULE_PARM_DESC(has_max_it_iu_size,
> +		  "Indicate the module supports max_it_iu_size login parameter");
> +
>  module_param(never_register, bool, 0444);
>  MODULE_PARM_DESC(never_register, "Never register memory");
>  
> ==============
> Then, srp_daemon should check file "/sys/module/ib_srp/parameters/has_max_it_iu_size"
> instead of "/sys/module/ib_srp/parameters/use_imm_data".

This should work but I'm not sure this is the best approach we can come
up with. Will one new kernel module parameter be added to the ib_srp
kernel module every time a login parameter is added?

Thanks,

Bart.
Honggang LI Oct. 24, 2019, 1:18 p.m. UTC | #8 
On Wed, Oct 23, 2019 at 07:13:27PM -0700, Bart Van Assche wrote:
> 
> This should work but I'm not sure this is the best approach we can come
> up with. Will one new kernel module parameter be added to the ib_srp
> kernel module every time a login parameter is added?

It seems we do not have better choice for backward compatibility.
I will send the kernel patch and srp_daemon patch.

thanks
diff mbox series

Patch

diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
index f0bcf923..43caf9d4 100644
--- a/srp_daemon/srp_daemon.c
+++ b/srp_daemon/srp_daemon.c
@@ -217,7 +217,7 @@  static int srpd_sys_read_uint64(const char *dir_name, const char *file_name,
 
 static void usage(const char *argv0)
 {
-	fprintf(stderr, "Usage: %s [-vVcaeon] [-d <umad device> | -i <infiniband device> [-p <port_num>]] [-t <timeout (ms)>] [-r <retries>] [-R <rescan time>] [-f <rules file>\n", argv0);
+	fprintf(stderr, "Usage: %s [-vVcaeonm] [-d <umad device> | -i <infiniband device> [-p <port_num>]] [-t <timeout (ms)>] [-r <retries>] [-R <rescan time>] [-f <rules file>\n", argv0);
 	fprintf(stderr, "-v 			Verbose\n");
 	fprintf(stderr, "-V 			debug Verbose\n");
 	fprintf(stderr, "-c 			prints connection Commands\n");
@@ -235,6 +235,7 @@  static void usage(const char *argv0)
 	fprintf(stderr, "-t <timeout>		Timeout for mad response in milliseconds\n");
 	fprintf(stderr, "-r <retries>		number of send Retries for each mad\n");
 	fprintf(stderr, "-n 			New connection command format - use also initiator extension\n");
+	fprintf(stderr, "-m 			New connection command format - use also maximum initiator to target IU size\n");
 	fprintf(stderr, "--systemd		Enable systemd integration.\n");
 	fprintf(stderr, "\nExample: srp_daemon -e -n -i mthca0 -p 1 -R 60\n");
 }
@@ -556,6 +557,19 @@  static int add_non_exist_target(struct target_details *target)
 		}
 	}
 
+	if (config->print_max_it_iu_size) {
+		len += snprintf(target_config_str+len,
+				sizeof(target_config_str) - len,
+				",max_it_iu_size=%d",
+				be32toh(target->ioc_prof.send_size));
+
+		if (len >= sizeof(target_config_str)) {
+			pr_err("Target config string is too long, ignoring target\n");
+			closedir(dir);
+			return -1;
+		}
+	}
+
 	if (config->execute && config->tl_retry_count) {
 		len += snprintf(target_config_str + len,
 				sizeof(target_config_str) - len,
@@ -1360,6 +1374,10 @@  static void print_config(struct config_t *conf)
 		printf(" Print initiator_ext\n");
 	else
 		printf(" Do not print initiator_ext\n");
+	if (conf->print_max_it_iu_size)
+		printf(" Print maximum initiator to target IU size\n");
+	else
+		printf(" Do not print maximum initiator to target IU size\n");
 	if (conf->recalc_time)
 		printf(" Performs full target rescan every %d seconds\n", conf->recalc_time);
 	else
@@ -1629,7 +1647,7 @@  static const struct option long_opts[] = {
 	{ "systemd",        0, NULL, 'S' },
 	{}
 };
-static const char short_opts[] = "caveod:i:j:p:t:r:R:T:l:Vhnf:";
+static const char short_opts[] = "caveod:i:j:p:t:r:R:T:l:Vhnmf:";
 
 /* Check if the --systemd options was passed in very early so we can setup
  * logging properly.
@@ -1670,6 +1688,7 @@  static int get_config(struct config_t *conf, int argc, char *argv[])
 	conf->retry_timeout 		= 20;
 	conf->add_target_file  		= NULL;
 	conf->print_initiator_ext	= 0;
+	conf->print_max_it_iu_size	= 0;
 	conf->rules_file		= SRP_DEAMON_CONFIG_FILE;
 	conf->rules			= NULL;
 	conf->tl_retry_count		= 0;
@@ -1734,6 +1753,9 @@  static int get_config(struct config_t *conf, int argc, char *argv[])
 		case 'n':
 			++conf->print_initiator_ext;
 			break;
+		case 'm':
+			++conf->print_max_it_iu_size;
+			break;
 		case 't':
 			conf->timeout = atoi(optarg);
 			if (conf->timeout == 0) {
diff --git a/srp_daemon/srp_daemon.h b/srp_daemon/srp_daemon.h
index b753cecd..d4111afc 100644
--- a/srp_daemon/srp_daemon.h
+++ b/srp_daemon/srp_daemon.h
@@ -206,6 +206,7 @@  struct config_t {
 	int		timeout;
 	int		recalc_time;
 	int		print_initiator_ext;
+	int		print_max_it_iu_size;
 	const char     *rules_file;
 	struct rule    *rules;
 	int 		retry_timeout;
diff --git a/srp_daemon/srp_daemon.sh.in b/srp_daemon/srp_daemon.sh.in
index 75e8a31b..dacbc9f5 100755
--- a/srp_daemon/srp_daemon.sh.in
+++ b/srp_daemon/srp_daemon.sh.in
@@ -76,7 +76,7 @@  for d in ${ibdir}_mad/umad*; do
     port="$(<"$d/port")"
     add_target="${ibdir}_srp/srp-${hca_id}-${port}/add_target"
     if [ -e "${add_target}" ]; then
-        ${prog} -e -c -n -i "${hca_id}" -p "${port}" -R "${rescan_interval}" "${params[@]}" >/dev/null 2>&1 &
+        ${prog} -e -c -n -m -i "${hca_id}" -p "${port}" -R "${rescan_interval}" "${params[@]}" >/dev/null 2>&1 &
         pids+=($!)
     fi
 done
diff --git a/srp_daemon/srp_daemon_port@.service.in b/srp_daemon/srp_daemon_port@.service.in
index 3d5a11e8..b91532ca 100644
--- a/srp_daemon/srp_daemon_port@.service.in
+++ b/srp_daemon/srp_daemon_port@.service.in
@@ -23,7 +23,7 @@  BindsTo=srp_daemon.service
 
 [Service]
 Type=simple
-ExecStart=@CMAKE_INSTALL_FULL_SBINDIR@/srp_daemon --systemd -e -c -n -j %I -R 60
+ExecStart=@CMAKE_INSTALL_FULL_SBINDIR@/srp_daemon --systemd -e -c -n -m -j %I -R 60
 MemoryDenyWriteExecute=yes
 PrivateNetwork=yes
 PrivateTmp=yes