* [patch v2 1/2] IB/srp: Add parse function for maximum initiator to target IU size
@ 2019-09-17 3:24 Honggang LI
2019-09-17 3:24 ` [patch v2 2/2] RDMA/SRP: calculate max_it_iu_size if remote max_it_iu length available Honggang LI
2019-09-17 17:44 ` [patch v2 1/2] IB/srp: Add parse function for maximum initiator to target IU size Bart Van Assche
0 siblings, 2 replies; 6+ messages in thread
From: Honggang LI @ 2019-09-17 3:24 UTC (permalink / raw)
To: bvanassche, dledford, jgg; +Cc: linux-rdma, Honggang Li
From: Honggang Li <honli@redhat.com>
According to SRP specifications 'srp-r16a' and 'srp2r06',
IOControllerProfile attributes for SRP target port include
the maximum initiator to target IU size.
SRP connection daemons, such as srp_daemon, can get the value
from subnet manager. The SRP connection daemon can pass this
value to kernel.
This patch add parse function for it.
Upstream commit [1] enables the kernel parameter, 'use_imm_data',
by default. [1] also use (8 * 1024) as the default value for
kernel parameter 'max_imm_data'. With those default values, the
maximum initiator to target IU size will be 8260.
In case the SRPT modules, which include the in-tree 'ib_srpt.ko'
module, do not support SRP-2 'immediate data' feature, the default
maximum initiator to target IU size is significantly samller than
8260. For 'ib_srpt.ko' module, which built from source before
[2], the default maximum initiator to target IU is 2116.
[1] introduces a regression issue for old srp target with default
kernel parameters, as the connection will be reject because of
too large maximum initiator to target IU size.
[1] commit 882981f4a411 ("RDMA/srp: Add support for immediate data")
[2] commit 5dabcd0456d7 ("RDMA/srpt: Add support for immediate data")
Signed-off-by: Honggang Li <honli@redhat.com>
---
drivers/infiniband/ulp/srp/ib_srp.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index b5960351bec0..2eadb038b257 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -75,6 +75,7 @@ static bool prefer_fr = true;
static bool register_always = true;
static bool never_register;
static int topspin_workarounds = 1;
+static uint32_t srp_opt_max_it_iu_size;
module_param(srp_sg_tablesize, uint, 0444);
MODULE_PARM_DESC(srp_sg_tablesize, "Deprecated name for cmd_sg_entries");
@@ -3411,6 +3412,7 @@ enum {
SRP_OPT_IP_SRC = 1 << 15,
SRP_OPT_IP_DEST = 1 << 16,
SRP_OPT_TARGET_CAN_QUEUE= 1 << 17,
+ SRP_OPT_MAX_IT_IU_SIZE = 1 << 18,
};
static unsigned int srp_opt_mandatory[] = {
@@ -3443,6 +3445,7 @@ static const match_table_t srp_opt_tokens = {
{ SRP_OPT_QUEUE_SIZE, "queue_size=%d" },
{ SRP_OPT_IP_SRC, "src=%s" },
{ SRP_OPT_IP_DEST, "dest=%s" },
+ { SRP_OPT_MAX_IT_IU_SIZE, "max_it_iu_size=%d" },
{ SRP_OPT_ERR, NULL }
};
@@ -3736,6 +3739,14 @@ static int srp_parse_options(struct net *net, const char *buf,
target->tl_retry_count = token;
break;
+ case SRP_OPT_MAX_IT_IU_SIZE:
+ if (match_int(args, &token) || token < 0) {
+ pr_warn("bad maximum initiator to target IU size '%s'\n", p);
+ goto out;
+ }
+ srp_opt_max_it_iu_size = token;
+ break;
+
default:
pr_warn("unknown parameter or missing value '%s' in target creation request\n",
p);
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [patch v2 2/2] RDMA/SRP: calculate max_it_iu_size if remote max_it_iu length available
2019-09-17 3:24 [patch v2 1/2] IB/srp: Add parse function for maximum initiator to target IU size Honggang LI
@ 2019-09-17 3:24 ` Honggang LI
2019-09-17 17:40 ` Bart Van Assche
2019-09-17 17:44 ` [patch v2 1/2] IB/srp: Add parse function for maximum initiator to target IU size Bart Van Assche
1 sibling, 1 reply; 6+ messages in thread
From: Honggang LI @ 2019-09-17 3:24 UTC (permalink / raw)
To: bvanassche, dledford, jgg; +Cc: linux-rdma, Honggang Li
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(-)
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;
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch v2 2/2] RDMA/SRP: calculate max_it_iu_size if remote max_it_iu length available
2019-09-17 3:24 ` [patch v2 2/2] RDMA/SRP: calculate max_it_iu_size if remote max_it_iu length available Honggang LI
@ 2019-09-17 17:40 ` Bart Van Assche
2019-09-19 3:36 ` Honggang LI
0 siblings, 1 reply; 6+ messages in thread
From: Bart Van Assche @ 2019-09-17 17:40 UTC (permalink / raw)
To: Honggang LI, dledford, jgg; +Cc: linux-rdma
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch v2 1/2] IB/srp: Add parse function for maximum initiator to target IU size
2019-09-17 3:24 [patch v2 1/2] IB/srp: Add parse function for maximum initiator to target IU size Honggang LI
2019-09-17 3:24 ` [patch v2 2/2] RDMA/SRP: calculate max_it_iu_size if remote max_it_iu length available Honggang LI
@ 2019-09-17 17:44 ` Bart Van Assche
2019-09-19 3:29 ` Honggang LI
1 sibling, 1 reply; 6+ messages in thread
From: Bart Van Assche @ 2019-09-17 17:44 UTC (permalink / raw)
To: Honggang LI, dledford, jgg; +Cc: linux-rdma
On 9/16/19 8:24 PM, Honggang LI wrote:
> In case the SRPT modules, which include the in-tree 'ib_srpt.ko'
> module, do not support SRP-2 'immediate data' feature, the default
> maximum initiator to target IU size is significantly samller than
^^^^^^^
smaller?
> 8260. For 'ib_srpt.ko' module, which built from source before
> [2], the default maximum initiator to target IU is 2116.
[ ... ]
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index b5960351bec0..2eadb038b257 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -75,6 +75,7 @@ static bool prefer_fr = true;
> static bool register_always = true;
> static bool never_register;
> static int topspin_workarounds = 1;
> +static uint32_t srp_opt_max_it_iu_size;
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. See
also srp_max_it_iu_len().
Thanks,
Bart.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch v2 1/2] IB/srp: Add parse function for maximum initiator to target IU size
2019-09-17 17:44 ` [patch v2 1/2] IB/srp: Add parse function for maximum initiator to target IU size Bart Van Assche
@ 2019-09-19 3:29 ` Honggang LI
0 siblings, 0 replies; 6+ messages in thread
From: Honggang LI @ 2019-09-19 3:29 UTC (permalink / raw)
To: Bart Van Assche; +Cc: dledford, jgg, linux-rdma
On Tue, Sep 17, 2019 at 10:44:56AM -0700, Bart Van Assche wrote:
> On 9/16/19 8:24 PM, Honggang LI wrote:
> > In case the SRPT modules, which include the in-tree 'ib_srpt.ko'
> > module, do not support SRP-2 'immediate data' feature, the default
> > maximum initiator to target IU size is significantly samller than
> ^^^^^^^
> smaller?
Yes, will fix this typo.
> > 8260. For 'ib_srpt.ko' module, which built from source before
> > [2], the default maximum initiator to target IU is 2116.
> [ ... ]
> > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> > index b5960351bec0..2eadb038b257 100644
> > --- a/drivers/infiniband/ulp/srp/ib_srp.c
> > +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> > @@ -75,6 +75,7 @@ static bool prefer_fr = true;
> > static bool register_always = true;
> > static bool never_register;
> > static int topspin_workarounds = 1;
> > +static uint32_t srp_opt_max_it_iu_size;
>
> 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. See
> also srp_max_it_iu_len().
Yes, will do as you said.
thanks
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch v2 2/2] RDMA/SRP: calculate max_it_iu_size if remote max_it_iu length available
2019-09-17 17:40 ` Bart Van Assche
@ 2019-09-19 3:36 ` Honggang LI
0 siblings, 0 replies; 6+ messages in thread
From: Honggang LI @ 2019-09-19 3:36 UTC (permalink / raw)
To: Bart Van Assche; +Cc: dledford, jgg, linux-rdma
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-19 3:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-17 3:24 [patch v2 1/2] IB/srp: Add parse function for maximum initiator to target IU size Honggang LI
2019-09-17 3:24 ` [patch v2 2/2] RDMA/SRP: calculate max_it_iu_size if remote max_it_iu length available Honggang LI
2019-09-17 17:40 ` Bart Van Assche
2019-09-19 3:36 ` Honggang LI
2019-09-17 17:44 ` [patch v2 1/2] IB/srp: Add parse function for maximum initiator to target IU size Bart Van Assche
2019-09-19 3:29 ` Honggang LI
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).