diff mbox series

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

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

Commit Message

Honggang LI Sept. 19, 2019, 3:50 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 | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

Comments

Bart Van Assche Sept. 20, 2019, 4:18 p.m. UTC | #1 
On 9/18/19 8:50 PM, Honggang LI wrote:
> From: Honggang Li <honli@redhat.com>
> 
> The default maximum immediate size is too big for old srp clients,
> which without immediate data support.
         ^^^^^^^
         do not?
> 
> 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?
> 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 <bvanassche@acm.org>
Honggang LI Sept. 23, 2019, 3:33 a.m. UTC | #2 
On Fri, Sep 20, 2019 at 09:18:49AM -0700, Bart Van Assche wrote:
> On 9/18/19 8:50 PM, Honggang LI wrote:
> > From: Honggang Li <honli@redhat.com>
> > 
> > The default maximum immediate size is too big for old srp clients,
> > which without immediate data support.
>         ^^^^^^^
>         do not?

OK, will fix it.

> > 
> > 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'.

> > 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.

> 
> Anyway, since the code changes look fine to me, feel free to add:
> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Bart Van Assche Sept. 23, 2019, 3:10 p.m. UTC | #3 
On 9/22/19 8:33 PM, Honggang LI wrote:
> On Fri, Sep 20, 2019 at 09:18:49AM -0700, Bart Van Assche wrote:
>> On 9/18/19 8:50 PM, Honggang LI wrote:
>>> The maximum initiator to target iu length can be get from the subnet
>>                                                     ^^^
>>                                           retrieved? obtained?
> 
> OK, will replace it with 'retrieved'.
> 
>>> 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.
  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 Sept. 27, 2019, 5:18 p.m. UTC | #4 
On Mon, Sep 23, 2019 at 08:10:48AM -0700, Bart Van Assche wrote:
> On 9/22/19 8:33 PM, Honggang LI wrote:
> > On Fri, Sep 20, 2019 at 09:18:49AM -0700, Bart Van Assche wrote:
> > > On 9/18/19 8:50 PM, Honggang LI wrote:
> > > > The maximum initiator to target iu length can be get from the subnet
> > >                                                     ^^^
> > >                                           retrieved? obtained?
> > 
> > OK, will replace it with 'retrieved'.
> > 
> > > > 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.
>  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 Sept. 27, 2019, 5:24 p.m. UTC | #5 
On 9/27/19 10:18 AM, Honggang LI wrote:
> On Mon, Sep 23, 2019 at 08:10:48AM -0700, Bart Van Assche wrote:
>> On 9/22/19 8:33 PM, Honggang LI wrote:
>>> On Fri, Sep 20, 2019 at 09:18:49AM -0700, Bart Van Assche wrote:
>>>> On 9/18/19 8:50 PM, Honggang LI wrote:
>>>>> The maximum initiator to target iu length can be get from the subnet
>>>>                                                      ^^^
>>>>                                            retrieved? obtained?
>>>
>>> OK, will replace it with 'retrieved'.
>>>
>>>>> 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.
>>   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?

[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.
diff mbox series

Patch

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;
 }
@@ -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);