IB/cma: Make timeout dependent on the subnet timeout

PATCH Bart Van Assche 5 participants 12 messages Apr 22, 2014 – Apr 25, 2014 10 messages not in the archive InfiniBand

Share & tools

Bart Van Assche

author

2014-04-22 13:16 UTC #0 lore raw

The default RDMA/CM timeout and retry values are too large for small IB networks and make the SRP initiator reconnect mechanism unnecessary slow. Hence make the CM timeout dependent on the subnet timeout in IB networks.

Signed-off-by Bart Van Assche <[email protected]>

Cc Sean Hefty <[email protected]>

---
 drivers/infiniband/core/cma.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

drivers/infiniband/core/cma.c +14 -4

index 199958d..95528e1 100644
@@ -60,7 +60,6 @@ MODULE_AUTHOR("Sean Hefty");
 MODULE_DESCRIPTION("Generic RDMA CM Agent");
 MODULE_LICENSE("Dual BSD/GPL");
 
-#define CMA_CM_RESPONSE_TIMEOUT 20
 #define CMA_MAX_CM_RETRIES 15
 #define CMA_CM_MRA_SETTING (IB_CM_MRA_FLAG_DELAY | 24)
 #define CMA_IBOE_PACKET_LIFETIME 18

@@ -2728,6 +2727,15 @@ out:
 	return ret;
 }
 
+static u8 cma_get_ib_subnet_timeout(struct rdma_cm_id *id)
+{
+	struct ib_port_attr attr;
+	int ret;
+
+	ret = ib_query_port(id->device, id->port_num, &attr);
+	return ret == 0 ? attr.subnet_timeout : 18;
+}
+
 static int cma_resolve_ib_udp(struct rdma_id_private *id_priv,
 			      struct rdma_conn_param *conn_param)
 {

@@ -2735,6 +2743,7 @@ static int cma_resolve_ib_udp(struct rdma_id_private *id_priv,
 	struct ib_cm_id	*id;
 	void *private_data;
 	int offset, ret;
+	u8 cm_response_timeout = cma_get_ib_subnet_timeout(&id_priv->id) + 2;
 
 	memset(&req, 0, sizeof req);
 	offset = cma_user_data_offset(id_priv);

@@ -2771,7 +2780,7 @@ static int cma_resolve_ib_udp(struct rdma_id_private *id_priv,
 
 	req.path = id_priv->id.route.path_rec;
 	req.service_id = rdma_get_service_id(&id_priv->id, cma_dst_addr(id_priv));
-	req.timeout_ms = 1 << (CMA_CM_RESPONSE_TIMEOUT - 8);
+	req.timeout_ms = 1 << max(cm_response_timeout - 8, 0);
 	req.max_cm_retries = CMA_MAX_CM_RETRIES;
 
 	ret = ib_send_cm_sidr_req(id_priv->cm_id.ib, &req);

@@ -2792,6 +2801,7 @@ static int cma_connect_ib(struct rdma_id_private *id_priv,
 	void *private_data;
 	struct ib_cm_id	*id;
 	int offset, ret;
+	u8 cm_response_timeout = cma_get_ib_subnet_timeout(&id_priv->id) + 2;
 
 	memset(&req, 0, sizeof req);
 	offset = cma_user_data_offset(id_priv);

@@ -2839,8 +2849,8 @@ static int cma_connect_ib(struct rdma_id_private *id_priv,
 	req.flow_control = conn_param->flow_control;
 	req.retry_count = min_t(u8, 7, conn_param->retry_count);
 	req.rnr_retry_count = min_t(u8, 7, conn_param->rnr_retry_count);
-	req.remote_cm_response_timeout = CMA_CM_RESPONSE_TIMEOUT;
-	req.local_cm_response_timeout = CMA_CM_RESPONSE_TIMEOUT;
+	req.remote_cm_response_timeout = cm_response_timeout;
+	req.local_cm_response_timeout = cm_response_timeout;
 	req.max_cm_retries = CMA_MAX_CM_RETRIES;
 	req.srq = id_priv->srq ? 1 : 0;

Signature · 6 lines

1.8.4.5

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Hefty, Sean

2014-04-22 18:41 UTC #2 lore raw

drivers/infiniband/core/cma.c out: · line 2737

Quoted 8 lines

drivers/infiniband/core/cma.c +8 -0

+static u8 cma_get_ib_subnet_timeout(struct rdma_cm_id *id)
+{
+	struct ib_port_attr attr;
+	int ret;
+
+	ret = ib_query_port(id->device, id->port_num, &attr);
+	return ret == 0 ? attr.subnet_timeout : 18;
+}

Can we query the port once (or only on a change) and cache the result, rather than querying it for every request?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1828884A29C6694DAF28B7E6B8A82373992F2F15-P5GAC/[email protected]

Hal Rosenstock

2014-04-23 12:30 UTC #4 lore raw

Quoted 10 lines · Sean

drivers/infiniband/core/cma.c out: · line 2737
Quoted 8 lines
drivers/infiniband/core/cma.c +8 -0
+static u8 cma_get_ib_subnet_timeout(struct rdma_cm_id *id)
+{
+	struct ib_port_attr attr;
+	int ret;
+
+	ret = ib_query_port(id->device, id->port_num, &attr);
+	return ret == 0 ? attr.subnet_timeout : 18;
+}
Can we query the port once (or only on a change) and cache the result, rather than querying it for every request?

To be IBA spec compliant, SubnetTimeout could change so some new local event would need to be added and handled to avoid the requerying.

In practice, however, that's not very likely AFAIK.

-- Hal
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 5357B25D.2000108-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org

Bart Van Assche

author

2014-04-23 12:46 UTC #6 lore raw

Quoted 12 lines · Hal Rosenstock

Quoted 10 lines · Sean
drivers/infiniband/core/cma.c out: · line 2737
Quoted 8 lines
drivers/infiniband/core/cma.c +8 -0
+static u8 cma_get_ib_subnet_timeout(struct rdma_cm_id *id)
+{
+	struct ib_port_attr attr;
+	int ret;
+
+	ret = ib_query_port(id->device, id->port_num, &attr);
+	return ret == 0 ? attr.subnet_timeout : 18;
+}
Can we query the port once (or only on a change) and cache the result, rather than querying it for every request?

Quoted 4 lines

To be IBA spec compliant, SubnetTimeout could change so some new local event would need to be added and handled to avoid the requerying.
In practice, however, that's not very likely AFAIK.

Thanks Sean and Hal for the feedback.

Regarding SubnetTimeout changes: the code in drivers/infiniband/core/cache.c already queues a work request after each port state change. Inside that work request e.g. the P_Key cache is updated. Would it be acceptable to modify ib_cache_update() such that it also queries the port attributes and caches these ? Cached port attributes could e.g. be stored in struct ib_port. However, doing so would probably require to protect the port_list member in struct ib_device against concurrent modifications of that list by the sysfs code.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Hal Rosenstock

2014-04-23 12:55 UTC #8 lore raw

Quoted 20 lines · Bart Van Assche

Quoted 12 lines · Hal Rosenstock
Quoted 10 lines · Sean
Review
Quoted 8 lines
(quoted diff) +8 -0
+static u8 cma_get_ib_subnet_timeout(struct rdma_cm_id *id)
+{
+	struct ib_port_attr attr;
+	int ret;
+
+	ret = ib_query_port(id->device, id->port_num, &attr);
+	return ret == 0 ? attr.subnet_timeout : 18;
+}
Can we query the port once (or only on a change) and cache the result, rather than querying it for every request?
Quoted 4 lines
To be IBA spec compliant, SubnetTimeout could change so some new local event would need to be added and handled to avoid the requerying.
In practice, however, that's not very likely AFAIK.
Thanks Sean and Hal for the feedback.

Quoted 8 lines

Regarding SubnetTimeout changes: the code in drivers/infiniband/core/cache.c already queues a work request after each port state change. Inside that work request e.g. the P_Key cache is updated. Would it be acceptable to modify ib_cache_update() such that it also queries the port attributes and caches these ? Cached port attributes could e.g. be stored in struct ib_port. However, doing so would probably require to protect the port_list member in struct ib_device against concurrent modifications of that list by the sysfs code.

Unfortunately, it's possible that the subnet timeout could be modified without any of the local events that currently trigger cache update occurring.

-- Hal

Quoted 3 lines

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 5357B83A.5070000-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org

Weiny, Ira

2014-04-23 14:34 UTC #10 lore raw

Quoted 32 lines · Bart Van Assche

Quoted 10 lines · Hal Rosenstock
Quoted 9 lines · Sean
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6 -0
+static u8 cma_get_ib_subnet_timeout(struct rdma_cm_id *id) {
+	struct ib_port_attr attr;
+	int ret;
+
+	ret = ib_query_port(id->device, id->port_num, &attr);
+	return ret == 0 ? attr.subnet_timeout : 18; }
Can we query the port once (or only on a change) and cache the result,
rather than querying it for every request?
Quoted 10 lines
Quoted 7 lines
To be IBA spec compliant, SubnetTimeout could change so some new local event would need to be added and handled to avoid the requerying.
In practice, however, that's not very likely AFAIK.
Thanks Sean and Hal for the feedback.
Quoted 8 lines
Regarding SubnetTimeout changes: the code in drivers/infiniband/core/cache.c already queues a work request after each port state change. Inside that work request e.g. the P_Key cache is updated. Would it be acceptable to modify ib_cache_update() such that it also queries the port attributes and caches these ? Cached port attributes could e.g. be stored in struct ib_port. However, doing so would probably require to protect the port_list member in struct ib_device against concurrent modifications of that list by the sysfs code.

Quoted 2 lines

Unfortunately, it's possible that the subnet timeout could be modified without any of the local events that currently trigger cache update occurring.

The Spec lists SubnetTimeout change as an unaffiliated Asynchronous event in section 11.6.3.3.

I think adding this event would be the correct course of action when supported. For HCA's which do not support "port change" events (which I don't think there are any) I guess a cache could be used.

Does this cache in core/cache.c respond to current events like P_Key table change?

Ira

Quoted 11 lines

-- Hal
Quoted 3 lines
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hefty, Sean

2014-04-23 13:44 UTC #11 lore raw

In reply to a message not in the archive

Quoted 6 lines

Regarding SubnetTimeout changes: the code in drivers/infiniband/core/cache.c already queues a work request after each port state change. Inside that work request e.g. the P_Key cache is updated. Would it be acceptable to modify ib_cache_update() such that it also queries the port attributes and caches these ? Cached port attributes could e.g. be stored in struct ib_port.

Without looking at details, this at least sounds reasonable.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1828884A29C6694DAF28B7E6B8A82373992F32A2-P5GAC/[email protected]

Or Gerlitz

2014-04-24 07:39 UTC #13 lore raw

Quoted 8 lines · Sean

Quoted 6 lines
Regarding SubnetTimeout changes: the code in drivers/infiniband/core/cache.c already queues a work request after each port state change. Inside that work request e.g. the P_Key cache is updated. Would it be acceptable to modify ib_cache_update() such that it also queries the port attributes and caches these ? Cached port attributes could e.g. be stored in struct ib_port.
Without looking at details, this at least sounds reasonable.

Sean, can't we have CMA to follow the same practice used in the CM where we derive the RC QP timeout based on the packet life time retrieved in path queries? e.g base the cm response time out on this value too?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 5358BF9D.4030409-VPRAkNaXOzVWk0Htik3J/[email protected]

Hefty, Sean

2014-04-25 02:59 UTC #15 lore raw

Quoted 3 lines

Sean, can't we have CMA to follow the same practice used in the CM where we derive the RC QP timeout based on the packet life time retrieved in path queries? e.g base the cm response time out on this value too?

We could. The timeout that's being modified by the patch is the time needed by the remote peer to process the incoming message and send a response. This time is in addition to the packet life time value that gets used.

For a remote kernel agent, the time needed to respond to a CM message may be fairly small. For a user space client, the time may be significant, on the order to seconds to minutes. We can probably make due with a fairly short timeout, provided that MRAs are used by the remote side.

There's no great solution that I can think of. Maybe the RDMA CM can adjust the timeout based on the remote address, assuming that it can determine if the remote address is a user space or kernel agent.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1828884A29C6694DAF28B7E6B8A82373992F4F6E-P5GAC/[email protected]

Bart Van Assche

author

2014-04-25 16:21 UTC #17 lore raw

Quoted 6 lines · Sean

Quoted 3 lines
Sean, can't we have CMA to follow the same practice used in the CM where we derive the RC QP timeout based on the packet life time retrieved in path queries? e.g base the cm response time out on this value too?
We could. The timeout that's being modified by the patch is the time needed by the remote peer to process the incoming message and send a

response. This

Quoted 4 lines

time is in addition to the packet life time value that gets used.
For a remote kernel agent, the time needed to respond to a CM message may be fairly small. For a user space client, the time may be significant,

on the

Quoted 6 lines

order to seconds to minutes. We can probably make due with a fairly short timeout, provided that MRAs are used by the remote side.
There's no great solution that I can think of. Maybe the RDMA CM can adjust the timeout based on the remote address, assuming that it can determine if the remote address is a user space or kernel agent.

Another possible approach is to make the CM timeout configurable for kernel clients only. How about creating two versions of struct rdma_conn_param - the existing version for communication between user space and kernel and a second version for in-kernel clients only ? In that second version a field could be added that allows to specify the CM timeout.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Hefty, Sean

2014-04-25 16:56 UTC #19 lore raw

Quoted 6 lines

Another possible approach is to make the CM timeout configurable for kernel clients only. How about creating two versions of struct rdma_conn_param - the existing version for communication between user space and kernel and a second version for in-kernel clients only ? In that second version a field could be added that allows to specify the CM timeout.

A single structure with a new field is sufficient. A value of 0 could just mean the default. But the timeout in this instance is per request, with the number of retries set to the max for IB (15). These are IB specific. It sounds like you're wanting to limit the total timeout.

Btw, up til now most of the requests regarding CM timeouts has been to increase it. We need to make sure that we don't end up breaking that. Even if we set this timeout to 0, the total timeout for a connection request is 2 x packet lifetime x # retries. With a 1 second packet lifetime, that's still 30 seconds. What timeout range are you looking for? The app could always set a separate timer and cancel the request as well. --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1828884A29C6694DAF28B7E6B8A82373992F50A6-P5GAC/[email protected]

Bart Van Assche

author

2014-04-25 17:47 UTC #21 lore raw

Quoted 7 lines · Sean

Btw, up til now most of the requests regarding CM timeouts has been to increase it. We need to make sure that we don't end up breaking that. Even if we set this timeout to 0, the total timeout for a connection request is 2 x packet lifetime x # retries. With a 1 second packet lifetime, that's still 30 seconds. What timeout range are you looking for? The app could always set a separate timer and cancel the request as well.

What I had noticed is that with the current timeout settings after a cable pull it can take really long before a cm_id object reaches TimeWait exit state. Timeout settings that are between 1/4th and 1/8th of the current settings result in more reasonable behavior.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html