RDMA/srp: Add parse function for maximum initiator to target IU size

PATCH Superseded Honggang LI 2 participants 10 messages Sep 19, 2019 – Sep 27, 2019

Subsystems (2)

InfiniBand SCSI RDMA protocol (SRP) initiator

lore

Share & tools

lore

Honggang LI

author

2019-09-19 03:50 UTC #0 lore raw

PATCH v3 1/2 RDMA/srp: Add parse function for maximum initiator to target IU size

From: Honggang Li <[email protected]>

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 smaller 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 <[email protected]>

---
 drivers/infiniband/ulp/srp/ib_srp.c | 10 ++++++++++
 drivers/infiniband/ulp/srp/ib_srp.h |  1 +
 2 files changed, 11 insertions(+)

drivers/infiniband/ulp/srp/ib_srp.c +10 -0

index b5960351bec0..b829dab0df77 100644
@@ -3411,6 +3411,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 +3444,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 +3738,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;
+			}
+			target->max_it_iu_size = token;
+			break;
+
 		default:
 			pr_warn("unknown parameter or missing value '%s' in target creation request\n",
 				p);

drivers/infiniband/ulp/srp/ib_srp.h +1 -0

index b2861cd2087a..105b2bc6aa2f 100644
@@ -209,6 +209,7 @@ struct srp_target_port {
 	u32			ch_count;
 	u32			lkey;
 	enum srp_target_state	state;
+	uint32_t		max_it_iu_size;
 	unsigned int		cmd_sg_cnt;
 	unsigned int		indirect_size;
 	bool			allow_ext_sg;

Signature · 1 line

2.21.0

Honggang LI

author

2019-09-19 03:51 UTC #1 lore raw

PATCH v3 2/2 RDMA/srp: calculate max_it_iu_size if remote max_it_iu length available

From: Honggang Li <[email protected]>

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 <[email protected]>

---
 drivers/infiniband/ulp/srp/ib_srp.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +20 -8

index b829dab0df77..b3bf5d552de9 100644
@@ -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;
 }

@@ -1389,7 +1397,8 @@ static int srp_rport_reconnect(struct srp_rport *rport)
 	struct srp_target_port *target = rport->lld_data;
 	struct srp_rdma_ch *ch;
 	uint32_t max_iu_len = srp_max_it_iu_len(target->cmd_sg_cnt,
-						srp_use_imm_data);
+						srp_use_imm_data,
+						target->max_it_iu_size);
 	int i, j, ret = 0;
 	bool multich = false;

@@ -2538,7 +2547,8 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
 		ch->req_lim       = be32_to_cpu(lrsp->req_lim_delta);
 		ch->use_imm_data  = lrsp->rsp_flags & SRP_LOGIN_RSP_IMMED_SUPP;
 		ch->max_it_iu_len = srp_max_it_iu_len(target->cmd_sg_cnt,
-						      ch->use_imm_data);
+						      ch->use_imm_data,
+						      target->max_it_iu_size);
 		WARN_ON_ONCE(ch->max_it_iu_len >
 			     be32_to_cpu(lrsp->max_it_iu_len));

@@ -3897,7 +3907,9 @@ static ssize_t srp_create_target(struct device *dev,
 	target->mr_per_cmd = mr_per_cmd;
 	target->indirect_size = target->sg_tablesize *
 				sizeof (struct srp_direct_buf);
-	max_iu_len = srp_max_it_iu_len(target->cmd_sg_cnt, srp_use_imm_data);
+	max_iu_len = srp_max_it_iu_len(target->cmd_sg_cnt,
+				       srp_use_imm_data,
+				       target->max_it_iu_size);
 
 	INIT_WORK(&target->tl_err_work, srp_tl_err_work);
 	INIT_WORK(&target->remove_work, srp_remove_work);

Signature · 1 line

2.21.0

Bart Van Assche

maintainer Reviewed-by on v3 2/2

2019-09-20 16:18 UTC #2 lore raw

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

Quoted 4 lines · Honggang LI

From: Honggang Li <[email protected]>
The default maximum immediate size is too big for old srp clients, which without immediate data support.

        ^^^^^^^
        do not?

Quoted 6 lines

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

                                                   ^^^
                                         retrieved? obtained?

Quoted 1 line

manager, such as opensm and opafm. We should calculate the

^^^^^^^

Are you sure that information comes from the subnet manager?
Isn't the LID passed to get_ioc_prof() in the srp_daemon the LID of the SRP target?

Anyway, since the code changes look fine to me, feel free to add:

Reviewed-by Bart Van Assche <[email protected]>

Honggang LI

author

2019-09-23 03:33 UTC #3 lore raw

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

Quoted 7 lines · Bart Van Assche

Quoted 4 lines · Honggang LI
From: Honggang Li <[email protected]>
The default maximum immediate size is too big for old srp clients, which without immediate data support.
        ^^^^^^^
        do not?

OK, will fix it.

Quoted 8 lines

Quoted 6 lines
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
                                                   ^^^
                                         retrieved? obtained?

OK, will replace it with 'retrieved'.

Quoted 6 lines

Quoted 1 line
manager, such as opensm and opafm. We should calculate the
^^^^^^^
Are you sure that information comes from the subnet manager? Isn't the LID passed to get_ioc_prof() in the srp_daemon the LID of the SRP target?

Yes, you are right. But srp_daemon/get_ioc_prof() send MAD packet to subnet manager to obtain the maximum initiator to target iu length.

Quoted 4 lines

Anyway, since the code changes look fine to me, feel free to add:
Reviewed-by Bart Van Assche <[email protected]>

Bart Van Assche

maintainer

2019-09-23 15:10 UTC #4 lore raw

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

Quoted 9 lines · Honggang LI

Quoted 4 lines · Bart Van Assche
Quoted 1 line · Honggang LI
The maximum initiator to target iu length can be get from the subnet
                                                    ^^^
                                          retrieved? obtained?
OK, will replace it with 'retrieved'.
Quoted 1 line
Quoted 1 line
manager, such as opensm and opafm. We should calculate the

Quoted 8 lines

Quoted 5 lines
   ^^^^^^^
Are you sure that information comes from the subnet manager? Isn't the LID passed to get_ioc_prof() in the srp_daemon the LID of the SRP target?
Yes, you are right. But srp_daemon/get_ioc_prof() send MAD packet to subnet manager to obtain the maximum initiator to target iu length.

I do not agree that the maximum initiator to target IU length comes
from the subnet manager. This is how I think the srp_daemon works:
1. The srp_daemon process sends a query to the subnet manager and asks the subnet manager to report all IB ports that support device management.
2. The subnet manager sends back information about all ports that support device management (struct srp_sa_port_info_rec).
3. The srp_daemon sends a query to the SRP target(s) to retrieve the IOUnitInfo (struct srp_dm_iou_info) and IOControllerProfile (struct srp_dm_ioc_prof). The maximum initiator to target IU length is present in that data structure (srp_dm_ioc_prof.send_size).

Bart.

Honggang LI

author

2019-09-27 17:18 UTC #5 lore raw

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

Quoted 18 lines · Bart Van Assche

Quoted 9 lines · Honggang LI
Quoted 4 lines · Bart Van Assche
Quoted 1 line · Honggang LI
The maximum initiator to target iu length can be get from the subnet
                                                    ^^^
                                          retrieved? obtained?
OK, will replace it with 'retrieved'.
Quoted 1 line
Quoted 1 line
manager, such as opensm and opafm. We should calculate the
Quoted 8 lines
Quoted 5 lines
   ^^^^^^^
Are you sure that information comes from the subnet manager? Isn't the LID passed to get_ioc_prof() in the srp_daemon the LID of the SRP target?
Yes, you are right. But srp_daemon/get_ioc_prof() send MAD packet to subnet manager to obtain the maximum initiator to target iu length.

Quoted 11 lines

I do not agree that the maximum initiator to target IU length comes from
the subnet manager. This is how I think the srp_daemon works:
1. The srp_daemon process sends a query to the subnet manager and asks the subnet manager to report all IB ports that support device management.
2. The subnet manager sends back information about all ports that support device management (struct srp_sa_port_info_rec).
3. The srp_daemon sends a query to the SRP target(s) to retrieve the IOUnitInfo (struct srp_dm_iou_info) and IOControllerProfile (struct srp_dm_ioc_prof). The maximum initiator to target IU length is present in that data structure (srp_dm_ioc_prof.send_size).

Yes, your description is more accurate.

[1] "The maximum initiator to target iu length can be retrieved from the subnet manager, such as opensm and opafm."

[2] "The maximum initiator to target iu length can be obtained by sending MAD packets to query subnet manager port and SRP target ports."

How about replacing sentence [1] with [2] in commit message?

thanks

Bart Van Assche

maintainer

2019-09-27 17:24 UTC #6 lore raw

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

Quoted 33 lines · Honggang LI

Quoted 18 lines · Bart Van Assche
Quoted 9 lines · Honggang LI
Quoted 4 lines · Bart Van Assche
Quoted 1 line · Honggang LI
The maximum initiator to target iu length can be get from the subnet
                                                     ^^^
                                           retrieved? obtained?
OK, will replace it with 'retrieved'.
Quoted 1 line
Quoted 1 line
manager, such as opensm and opafm. We should calculate the
Quoted 8 lines
Quoted 5 lines
    ^^^^^^^
Are you sure that information comes from the subnet manager? Isn't the LID passed to get_ioc_prof() in the srp_daemon the LID of the SRP target?
Yes, you are right. But srp_daemon/get_ioc_prof() send MAD packet to subnet manager to obtain the maximum initiator to target iu length.
Quoted 11 lines
I do not agree that the maximum initiator to target IU length comes from
the subnet manager. This is how I think the srp_daemon works:
1. The srp_daemon process sends a query to the subnet manager and asks the subnet manager to report all IB ports that support device management.
2. The subnet manager sends back information about all ports that support device management (struct srp_sa_port_info_rec).
3. The srp_daemon sends a query to the SRP target(s) to retrieve the IOUnitInfo (struct srp_dm_iou_info) and IOControllerProfile (struct srp_dm_ioc_prof). The maximum initiator to target IU length is present in that data structure (srp_dm_ioc_prof.send_size).
Yes, your description is more accurate.

Quoted 7 lines

[1] "The maximum initiator to target iu length can be retrieved from the subnet manager, such as opensm and opafm."
[2] "The maximum initiator to target iu length can be obtained by sending MAD packets to query subnet manager port and SRP target ports."
How about replacing sentence [1] with [2] in commit message?

[2] sounds a little bit confusing to me but I think we have already spent way too much time on discussing the commit message, so I'm fine with [2].

Bart.

Bart Van Assche

maintainer Reviewed-by on v3 1/2

2019-09-20 16:11 UTC #7 lore raw

In reply to Honggang LI

Quoted 41 lines · Honggang LI

drivers/infiniband/ulp/srp/ib_srp.c +10 -0

index b5960351bec0..b829dab0df77 100644
@@ -3411,6 +3411,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 +3444,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 +3738,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;
+			}
+			target->max_it_iu_size = token;
+			break;
+
  		default:
  			pr_warn("unknown parameter or missing value '%s' in target creation request\n",
  				p);

drivers/infiniband/ulp/srp/ib_srp.h +0 -0

index b2861cd2087a..105b2bc6aa2f 100644

drivers/infiniband/ulp/srp/ib_srp.h struct srp_target_port { · line 215

Quoted 7 lines

drivers/infiniband/ulp/srp/ib_srp.h +1 -0

@@ -209,6 +209,7 @@ struct srp_target_port {
  	u32			ch_count;
  	u32			lkey;
  	enum srp_target_state	state;
+	uint32_t		max_it_iu_size;
  	unsigned int		cmd_sg_cnt;
  	unsigned int		indirect_size;
  	bool			allow_ext_sg;

Something I forgot to mention last time: since this patch adds a new login parameter Documentation/ABI/stable/sysfs-driver-ib_srp should be updated. I don't have a strong opinion about whether that should happen through a separate patch or not. Since the code changes look fine to me:

Reviewed-by Bart Van Assche <[email protected]>

Honggang LI

author

2019-09-23 03:30 UTC #8 lore raw

Quoted 3 lines · Bart Van Assche

Something I forgot to mention last time: since this patch adds a new login parameter Documentation/ABI/stable/sysfs-driver-ib_srp should be updated. I

I will submit a new patch to update 'sysfs-driver-ib_srp'.

thanks

Bart Van Assche

maintainer

2019-09-23 15:12 UTC #9 lore raw

Quoted 6 lines · Honggang LI

Quoted 3 lines · Bart Van Assche
Something I forgot to mention last time: since this patch adds a new login parameter Documentation/ABI/stable/sysfs-driver-ib_srp should be updated. I
I will submit a new patch to update 'sysfs-driver-ib_srp'.

Thanks!

Bart.