LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH for-next v2] RDMA/core/sa_query: Retry SA queries
@ 2021-08-12 16:12 Håkon Bugge
  2021-08-23 12:25 ` Haakon Bugge
  2021-08-25 17:49 ` Jason Gunthorpe
  0 siblings, 2 replies; 4+ messages in thread
From: Håkon Bugge @ 2021-08-12 16:12 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe, Leon Romanovsky, linux-rdma, linux-kernel

A MAD packet is sent as an unreliable datagram (UD). SA requests are
sent as MAD packets. As such, SA requests or responses may be silently
dropped.

IB Core's MAD layer has a timeout and retry mechanism, which amongst
other, is used by RDMA CM. But it is not used by SA queries. The lack
of retries of SA queries leads to long specified timeout, and error
being returned in case of packet loss. The ULP or user-land process
has to perform the retry.

Fix this by taking advantage of the MAD layer's retry mechanism.

First, a check against a zero timeout is added in
rdma_resolve_route(). In send_mad(), we set the MAD layer timeout to
one tenth of the specified timeout and the number of retries to
10. The special case when timeout is less than 10 is handled.

With this fix:

 # ucmatose -c 1000 -S 1024 -C 1

runs stable on an Infiniband fabric. Without this fix, we see an
intermittent behavior and it errors out with:

cmatose: event: RDMA_CM_EVENT_ROUTE_ERROR, error: -110

(110 is ETIMEDOUT)

Fixes: f75b7a529494 ("[PATCH] IB: Add automatic retries to MAD layer")
Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
---
 drivers/infiniband/core/cma.c      | 3 +++
 drivers/infiniband/core/sa_query.c | 9 ++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 515a7e9..c3f2fac 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -3117,6 +3117,9 @@ int rdma_resolve_route(struct rdma_cm_id *id, unsigned long timeout_ms)
 	struct rdma_id_private *id_priv;
 	int ret;
 
+	if (!timeout_ms)
+		return -EINVAL;
+
 	id_priv = container_of(id, struct rdma_id_private, id);
 	if (!cma_comp_exch(id_priv, RDMA_CM_ADDR_RESOLVED, RDMA_CM_ROUTE_QUERY))
 		return -EINVAL;
diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index b61576f..5a56082 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -1358,6 +1358,7 @@ static int send_mad(struct ib_sa_query *query, unsigned long timeout_ms,
 {
 	unsigned long flags;
 	int ret, id;
+	const int nmbr_sa_query_retries = 10;
 
 	xa_lock_irqsave(&queries, flags);
 	ret = __xa_alloc(&queries, &id, query, xa_limit_32b, gfp_mask);
@@ -1365,7 +1366,13 @@ static int send_mad(struct ib_sa_query *query, unsigned long timeout_ms,
 	if (ret < 0)
 		return ret;
 
-	query->mad_buf->timeout_ms  = timeout_ms;
+	query->mad_buf->timeout_ms  = timeout_ms / nmbr_sa_query_retries;
+	query->mad_buf->retries = nmbr_sa_query_retries;
+	if (!query->mad_buf->timeout_ms) {
+		/* Special case, very small timeout_ms */
+		query->mad_buf->timeout_ms = 1;
+		query->mad_buf->retries = timeout_ms;
+	}
 	query->mad_buf->context[0] = query;
 	query->id = id;
 
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH for-next v2] RDMA/core/sa_query: Retry SA queries
  2021-08-12 16:12 [PATCH for-next v2] RDMA/core/sa_query: Retry SA queries Håkon Bugge
@ 2021-08-23 12:25 ` Haakon Bugge
  2021-08-25 17:49 ` Jason Gunthorpe
  1 sibling, 0 replies; 4+ messages in thread
From: Haakon Bugge @ 2021-08-23 12:25 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe, Leon Romanovsky, OFED mailing list, LKML

A gentle ping on this one.


Thxs, Håkon


> On 12 Aug 2021, at 18:12, Håkon Bugge <Haakon.Bugge@oracle.com> wrote:
> 
> A MAD packet is sent as an unreliable datagram (UD). SA requests are
> sent as MAD packets. As such, SA requests or responses may be silently
> dropped.
> 
> IB Core's MAD layer has a timeout and retry mechanism, which amongst
> other, is used by RDMA CM. But it is not used by SA queries. The lack
> of retries of SA queries leads to long specified timeout, and error
> being returned in case of packet loss. The ULP or user-land process
> has to perform the retry.
> 
> Fix this by taking advantage of the MAD layer's retry mechanism.
> 
> First, a check against a zero timeout is added in
> rdma_resolve_route(). In send_mad(), we set the MAD layer timeout to
> one tenth of the specified timeout and the number of retries to
> 10. The special case when timeout is less than 10 is handled.
> 
> With this fix:
> 
> # ucmatose -c 1000 -S 1024 -C 1
> 
> runs stable on an Infiniband fabric. Without this fix, we see an
> intermittent behavior and it errors out with:
> 
> cmatose: event: RDMA_CM_EVENT_ROUTE_ERROR, error: -110
> 
> (110 is ETIMEDOUT)
> 
> Fixes: f75b7a529494 ("[PATCH] IB: Add automatic retries to MAD layer")
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> ---
> drivers/infiniband/core/cma.c      | 3 +++
> drivers/infiniband/core/sa_query.c | 9 ++++++++-
> 2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 515a7e9..c3f2fac 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -3117,6 +3117,9 @@ int rdma_resolve_route(struct rdma_cm_id *id, unsigned long timeout_ms)
> 	struct rdma_id_private *id_priv;
> 	int ret;
> 
> +	if (!timeout_ms)
> +		return -EINVAL;
> +
> 	id_priv = container_of(id, struct rdma_id_private, id);
> 	if (!cma_comp_exch(id_priv, RDMA_CM_ADDR_RESOLVED, RDMA_CM_ROUTE_QUERY))
> 		return -EINVAL;
> diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
> index b61576f..5a56082 100644
> --- a/drivers/infiniband/core/sa_query.c
> +++ b/drivers/infiniband/core/sa_query.c
> @@ -1358,6 +1358,7 @@ static int send_mad(struct ib_sa_query *query, unsigned long timeout_ms,
> {
> 	unsigned long flags;
> 	int ret, id;
> +	const int nmbr_sa_query_retries = 10;
> 
> 	xa_lock_irqsave(&queries, flags);
> 	ret = __xa_alloc(&queries, &id, query, xa_limit_32b, gfp_mask);
> @@ -1365,7 +1366,13 @@ static int send_mad(struct ib_sa_query *query, unsigned long timeout_ms,
> 	if (ret < 0)
> 		return ret;
> 
> -	query->mad_buf->timeout_ms  = timeout_ms;
> +	query->mad_buf->timeout_ms  = timeout_ms / nmbr_sa_query_retries;
> +	query->mad_buf->retries = nmbr_sa_query_retries;
> +	if (!query->mad_buf->timeout_ms) {
> +		/* Special case, very small timeout_ms */
> +		query->mad_buf->timeout_ms = 1;
> +		query->mad_buf->retries = timeout_ms;
> +	}
> 	query->mad_buf->context[0] = query;
> 	query->id = id;
> 
> -- 
> 1.8.3.1
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH for-next v2] RDMA/core/sa_query: Retry SA queries
  2021-08-12 16:12 [PATCH for-next v2] RDMA/core/sa_query: Retry SA queries Håkon Bugge
  2021-08-23 12:25 ` Haakon Bugge
@ 2021-08-25 17:49 ` Jason Gunthorpe
  2021-08-26 15:59   ` Haakon Bugge
  1 sibling, 1 reply; 4+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 17:49 UTC (permalink / raw)
  To: Håkon Bugge; +Cc: Doug Ledford, Leon Romanovsky, linux-rdma, linux-kernel

On Thu, Aug 12, 2021 at 06:12:35PM +0200, Håkon Bugge wrote:
> A MAD packet is sent as an unreliable datagram (UD). SA requests are
> sent as MAD packets. As such, SA requests or responses may be silently
> dropped.
> 
> IB Core's MAD layer has a timeout and retry mechanism, which amongst
> other, is used by RDMA CM. But it is not used by SA queries. The lack
> of retries of SA queries leads to long specified timeout, and error
> being returned in case of packet loss. The ULP or user-land process
> has to perform the retry.
> 
> Fix this by taking advantage of the MAD layer's retry mechanism.
> 
> First, a check against a zero timeout is added in
> rdma_resolve_route(). In send_mad(), we set the MAD layer timeout to
> one tenth of the specified timeout and the number of retries to
> 10. The special case when timeout is less than 10 is handled.
> 
> With this fix:
> 
>  # ucmatose -c 1000 -S 1024 -C 1
> 
> runs stable on an Infiniband fabric. Without this fix, we see an
> intermittent behavior and it errors out with:
> 
> cmatose: event: RDMA_CM_EVENT_ROUTE_ERROR, error: -110
> 
> (110 is ETIMEDOUT)
> 
> Fixes: f75b7a529494 ("[PATCH] IB: Add automatic retries to MAD layer")
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> ---
>  drivers/infiniband/core/cma.c      | 3 +++
>  drivers/infiniband/core/sa_query.c | 9 ++++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)

I'm nervous about this, mostly because the mad layer is very
complicated, but it does seem aligned with the spec.

However, it seems quite wrong that the timeout comes in from outside,
the SA timeout should be integral to the SA layer..

Anyhow, applied to for-next

Jason

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH for-next v2] RDMA/core/sa_query: Retry SA queries
  2021-08-25 17:49 ` Jason Gunthorpe
@ 2021-08-26 15:59   ` Haakon Bugge
  0 siblings, 0 replies; 4+ messages in thread
From: Haakon Bugge @ 2021-08-26 15:59 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Doug Ledford, Leon Romanovsky, OFED mailing list, linux-kernel



> On 25 Aug 2021, at 19:49, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> On Thu, Aug 12, 2021 at 06:12:35PM +0200, Håkon Bugge wrote:
>> A MAD packet is sent as an unreliable datagram (UD). SA requests are
>> sent as MAD packets. As such, SA requests or responses may be silently
>> dropped.
>> 
>> IB Core's MAD layer has a timeout and retry mechanism, which amongst
>> other, is used by RDMA CM. But it is not used by SA queries. The lack
>> of retries of SA queries leads to long specified timeout, and error
>> being returned in case of packet loss. The ULP or user-land process
>> has to perform the retry.
>> 
>> Fix this by taking advantage of the MAD layer's retry mechanism.
>> 
>> First, a check against a zero timeout is added in
>> rdma_resolve_route(). In send_mad(), we set the MAD layer timeout to
>> one tenth of the specified timeout and the number of retries to
>> 10. The special case when timeout is less than 10 is handled.
>> 
>> With this fix:
>> 
>> # ucmatose -c 1000 -S 1024 -C 1
>> 
>> runs stable on an Infiniband fabric. Without this fix, we see an
>> intermittent behavior and it errors out with:
>> 
>> cmatose: event: RDMA_CM_EVENT_ROUTE_ERROR, error: -110
>> 
>> (110 is ETIMEDOUT)
>> 
>> Fixes: f75b7a529494 ("[PATCH] IB: Add automatic retries to MAD layer")
>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>> ---
>> drivers/infiniband/core/cma.c      | 3 +++
>> drivers/infiniband/core/sa_query.c | 9 ++++++++-
>> 2 files changed, 11 insertions(+), 1 deletion(-)
> 
> I'm nervous about this, mostly because the mad layer is very
> complicated, but it does seem aligned with the spec.
> 
> However, it seems quite wrong that the timeout comes in from outside,
> the SA timeout should be integral to the SA layer..

They are quite different (timeout in ms):

	iser:      1000
	rtrs:     30000
	srp:       1000
	nvme:      3000
	samba:     5000
	p9:       30000
	rds:       5000
	xprtrdma:  5000

Dividing 30 seconds by ten and get 3, seems OK. But for iser/srp, we get 100ms, which is in the low end for some system I would expect.

> Anyhow, applied to for-next


Thanks!


Håkon


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-08-26 15:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-12 16:12 [PATCH for-next v2] RDMA/core/sa_query: Retry SA queries Håkon Bugge
2021-08-23 12:25 ` Haakon Bugge
2021-08-25 17:49 ` Jason Gunthorpe
2021-08-26 15:59   ` Haakon Bugge

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