From: Tom Tucker <[email protected]>
This logic sets the connection parameter that configures the local device
and informs the remote peer how many concurrent incoming RDMA_READ
requests are supported. The original logic didn't really do what was
intended for two reasons:
- The max number supported by the device is typically smaller than
any one factor in the calculation used, and
- The field in the connection parameter structure where the value is
stored is a u8 and always overflows for the default settings.
So what really happens is the value requested for responder resources
is the left over 8 bits from the "desired value". If the desired value
happened to be a multiple of 256, the result was zero and it wouldn't
connect at all.
Given the above and the fact that max_requests is almost always larger
than the max responder resources supported by the adapter, this patch
simplifies this logic and simply requests the max supported by the device,
subject to a reasonable limit.
This bug was found by Jim Schutt at Sandia.
Signed-off-by: Tom Tucker <[email protected]>
Signed-off-by: Tom Talpey <[email protected]>
---
net/sunrpc/xprtrdma/verbs.c | 51 ++++++++++++-------------------------------
1 files changed, 14 insertions(+), 37 deletions(-)
diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
index 39a1652..e3fe905 100644
--- a/net/sunrpc/xprtrdma/verbs.c
+++ b/net/sunrpc/xprtrdma/verbs.c
@@ -705,30 +705,13 @@ rpcrdma_ep_create(struct rpcrdma_ep *ep, struct rpcrdma_ia *ia,
ep->rep_remote_cma.private_data_len = 0;
/* Client offers RDMA Read but does not initiate */
- switch (ia->ri_memreg_strategy) {
- case RPCRDMA_BOUNCEBUFFERS:
+ ep->rep_remote_cma.initiator_depth = 0;
+ if (ia->ri_memreg_strategy == RPCRDMA_BOUNCEBUFFERS)
ep->rep_remote_cma.responder_resources = 0;
- break;
- case RPCRDMA_MTHCAFMR:
- case RPCRDMA_REGISTER:
- case RPCRDMA_FRMR:
- ep->rep_remote_cma.responder_resources = cdata->max_requests *
- (RPCRDMA_MAX_DATA_SEGS / 8);
- break;
- case RPCRDMA_MEMWINDOWS:
- case RPCRDMA_MEMWINDOWS_ASYNC:
-#if RPCRDMA_PERSISTENT_REGISTRATION
- case RPCRDMA_ALLPHYSICAL:
-#endif
- ep->rep_remote_cma.responder_resources = cdata->max_requests *
- (RPCRDMA_MAX_DATA_SEGS / 2);
- break;
- default:
- break;
- }
- if (ep->rep_remote_cma.responder_resources > devattr.max_qp_rd_atom)
+ else if (devattr.max_qp_rd_atom > 32) /* arbitrary but <= 255 */
+ ep->rep_remote_cma.responder_resources = 32;
+ else
ep->rep_remote_cma.responder_resources = devattr.max_qp_rd_atom;
- ep->rep_remote_cma.initiator_depth = 0;
ep->rep_remote_cma.retry_count = 7;
ep->rep_remote_cma.flow_control = 0;
@@ -858,14 +841,6 @@ if (strnicmp(ia->ri_id->device->dma_device->bus->name, "pci", 3) == 0) {
}
}
- /* Theoretically a client initiator_depth > 0 is not needed,
- * but many peers fail to complete the connection unless they
- * == responder_resources! */
- if (ep->rep_remote_cma.initiator_depth !=
- ep->rep_remote_cma.responder_resources)
- ep->rep_remote_cma.initiator_depth =
- ep->rep_remote_cma.responder_resources;
-
ep->rep_connected = 0;
rc = rdma_connect(ia->ri_id, &ep->rep_remote_cma);
@@ -894,14 +869,16 @@ if (strnicmp(ia->ri_id->device->dma_device->bus->name, "pci", 3) == 0) {
if (ep->rep_connected <= 0) {
/* Sometimes, the only way to reliably connect to remote
* CMs is to use same nonzero values for ORD and IRD. */
- ep->rep_remote_cma.initiator_depth =
- ep->rep_remote_cma.responder_resources;
- if (ep->rep_remote_cma.initiator_depth == 0)
- ++ep->rep_remote_cma.initiator_depth;
- if (ep->rep_remote_cma.responder_resources == 0)
- ++ep->rep_remote_cma.responder_resources;
- if (retry_count++ == 0)
+ if (retry_count++ <= RDMA_CONNECT_RETRY_MAX + 1 &&
+ (ep->rep_remote_cma.responder_resources == 0 ||
+ ep->rep_remote_cma.initiator_depth !=
+ ep->rep_remote_cma.responder_resources)) {
+ if (ep->rep_remote_cma.responder_resources == 0)
+ ep->rep_remote_cma.responder_resources = 1;
+ ep->rep_remote_cma.initiator_depth =
+ ep->rep_remote_cma.responder_resources;
goto retry;
+ }
rc = ep->rep_connected;
} else {
dprintk("RPC: %s: connected\n", __func__);
On Wed, 2008-10-08 at 11:47 -0400, Tom Talpey wrote:
> From: Tom Tucker <[email protected]>
Now I'm really confused!
> This logic sets the connection parameter that configures the local device
> and informs the remote peer how many concurrent incoming RDMA_READ
> requests are supported. The original logic didn't really do what was
> intended for two reasons:
>
> - The max number supported by the device is typically smaller than
> any one factor in the calculation used, and
>
> - The field in the connection parameter structure where the value is
> stored is a u8 and always overflows for the default settings.
>
> So what really happens is the value requested for responder resources
> is the left over 8 bits from the "desired value". If the desired value
> happened to be a multiple of 256, the result was zero and it wouldn't
> connect at all.
>
> Given the above and the fact that max_requests is almost always larger
> than the max responder resources supported by the adapter, this patch
> simplifies this logic and simply requests the max supported by the device,
> subject to a reasonable limit.
>
> This bug was found by Jim Schutt at Sandia.
>
> Signed-off-by: Tom Tucker <[email protected]>
> Signed-off-by: Tom Talpey <[email protected]>
> ---
>
> net/sunrpc/xprtrdma/verbs.c | 51 ++++++++++++-------------------------------
> 1 files changed, 14 insertions(+), 37 deletions(-)
>
> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
> index 39a1652..e3fe905 100644
> --- a/net/sunrpc/xprtrdma/verbs.c
> +++ b/net/sunrpc/xprtrdma/verbs.c
> @@ -705,30 +705,13 @@ rpcrdma_ep_create(struct rpcrdma_ep *ep, struct rpcrdma_ia *ia,
> ep->rep_remote_cma.private_data_len = 0;
>
> /* Client offers RDMA Read but does not initiate */
> - switch (ia->ri_memreg_strategy) {
> - case RPCRDMA_BOUNCEBUFFERS:
> + ep->rep_remote_cma.initiator_depth = 0;
> + if (ia->ri_memreg_strategy == RPCRDMA_BOUNCEBUFFERS)
> ep->rep_remote_cma.responder_resources = 0;
> - break;
> - case RPCRDMA_MTHCAFMR:
> - case RPCRDMA_REGISTER:
> - case RPCRDMA_FRMR:
> - ep->rep_remote_cma.responder_resources = cdata->max_requests *
> - (RPCRDMA_MAX_DATA_SEGS / 8);
> - break;
> - case RPCRDMA_MEMWINDOWS:
> - case RPCRDMA_MEMWINDOWS_ASYNC:
> -#if RPCRDMA_PERSISTENT_REGISTRATION
> - case RPCRDMA_ALLPHYSICAL:
> -#endif
> - ep->rep_remote_cma.responder_resources = cdata->max_requests *
> - (RPCRDMA_MAX_DATA_SEGS / 2);
> - break;
> - default:
> - break;
> - }
> - if (ep->rep_remote_cma.responder_resources > devattr.max_qp_rd_atom)
> + else if (devattr.max_qp_rd_atom > 32) /* arbitrary but <= 255 */
> + ep->rep_remote_cma.responder_resources = 32;
> + else
> ep->rep_remote_cma.responder_resources = devattr.max_qp_rd_atom;
> - ep->rep_remote_cma.initiator_depth = 0;
>
> ep->rep_remote_cma.retry_count = 7;
> ep->rep_remote_cma.flow_control = 0;
> @@ -858,14 +841,6 @@ if (strnicmp(ia->ri_id->device->dma_device->bus->name, "pci", 3) == 0) {
> }
> }
>
> - /* Theoretically a client initiator_depth > 0 is not needed,
> - * but many peers fail to complete the connection unless they
> - * == responder_resources! */
> - if (ep->rep_remote_cma.initiator_depth !=
> - ep->rep_remote_cma.responder_resources)
> - ep->rep_remote_cma.initiator_depth =
> - ep->rep_remote_cma.responder_resources;
> -
> ep->rep_connected = 0;
>
> rc = rdma_connect(ia->ri_id, &ep->rep_remote_cma);
> @@ -894,14 +869,16 @@ if (strnicmp(ia->ri_id->device->dma_device->bus->name, "pci", 3) == 0) {
> if (ep->rep_connected <= 0) {
> /* Sometimes, the only way to reliably connect to remote
> * CMs is to use same nonzero values for ORD and IRD. */
> - ep->rep_remote_cma.initiator_depth =
> - ep->rep_remote_cma.responder_resources;
> - if (ep->rep_remote_cma.initiator_depth == 0)
> - ++ep->rep_remote_cma.initiator_depth;
> - if (ep->rep_remote_cma.responder_resources == 0)
> - ++ep->rep_remote_cma.responder_resources;
> - if (retry_count++ == 0)
> + if (retry_count++ <= RDMA_CONNECT_RETRY_MAX + 1 &&
> + (ep->rep_remote_cma.responder_resources == 0 ||
> + ep->rep_remote_cma.initiator_depth !=
> + ep->rep_remote_cma.responder_resources)) {
> + if (ep->rep_remote_cma.responder_resources == 0)
> + ep->rep_remote_cma.responder_resources = 1;
> + ep->rep_remote_cma.initiator_depth =
> + ep->rep_remote_cma.responder_resources;
> goto retry;
> + }
> rc = ep->rep_connected;
> } else {
> dprintk("RPC: %s: connected\n", __func__);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
At 01:26 PM 10/8/2008, Trond Myklebust wrote:
>On Wed, 2008-10-08 at 11:47 -0400, Tom Talpey wrote:
>> From: Tom Tucker <[email protected]>
>
>Now I'm really confused!
Ok, that's a bug. :-)
Will fix, after figuring out the format you prefer for joint authors.
Tom.
>
>> This logic sets the connection parameter that configures the local device
>> and informs the remote peer how many concurrent incoming RDMA_READ
>> requests are supported. The original logic didn't really do what was
>> intended for two reasons:
>>
>> - The max number supported by the device is typically smaller than
>> any one factor in the calculation used, and
>>
>> - The field in the connection parameter structure where the value is
>> stored is a u8 and always overflows for the default settings.
>>
>> So what really happens is the value requested for responder resources
>> is the left over 8 bits from the "desired value". If the desired value
>> happened to be a multiple of 256, the result was zero and it wouldn't
>> connect at all.
>>
>> Given the above and the fact that max_requests is almost always larger
>> than the max responder resources supported by the adapter, this patch
>> simplifies this logic and simply requests the max supported by the device,
>> subject to a reasonable limit.
>>
>> This bug was found by Jim Schutt at Sandia.
>>
>> Signed-off-by: Tom Tucker <[email protected]>
>> Signed-off-by: Tom Talpey <[email protected]>
>> ---
>>
>> net/sunrpc/xprtrdma/verbs.c | 51
>++++++++++++-------------------------------
>> 1 files changed, 14 insertions(+), 37 deletions(-)
>>
>> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
>> index 39a1652..e3fe905 100644
>> --- a/net/sunrpc/xprtrdma/verbs.c
>> +++ b/net/sunrpc/xprtrdma/verbs.c
>> @@ -705,30 +705,13 @@ rpcrdma_ep_create(struct rpcrdma_ep *ep,
>struct rpcrdma_ia *ia,
>> ep->rep_remote_cma.private_data_len = 0;
>>
>> /* Client offers RDMA Read but does not initiate */
>> - switch (ia->ri_memreg_strategy) {
>> - case RPCRDMA_BOUNCEBUFFERS:
>> + ep->rep_remote_cma.initiator_depth = 0;
>> + if (ia->ri_memreg_strategy == RPCRDMA_BOUNCEBUFFERS)
>> ep->rep_remote_cma.responder_resources = 0;
>> - break;
>> - case RPCRDMA_MTHCAFMR:
>> - case RPCRDMA_REGISTER:
>> - case RPCRDMA_FRMR:
>> - ep->rep_remote_cma.responder_resources = cdata->max_requests *
>> - (RPCRDMA_MAX_DATA_SEGS / 8);
>> - break;
>> - case RPCRDMA_MEMWINDOWS:
>> - case RPCRDMA_MEMWINDOWS_ASYNC:
>> -#if RPCRDMA_PERSISTENT_REGISTRATION
>> - case RPCRDMA_ALLPHYSICAL:
>> -#endif
>> - ep->rep_remote_cma.responder_resources = cdata->max_requests *
>> - (RPCRDMA_MAX_DATA_SEGS / 2);
>> - break;
>> - default:
>> - break;
>> - }
>> - if (ep->rep_remote_cma.responder_resources > devattr.max_qp_rd_atom)
>> + else if (devattr.max_qp_rd_atom > 32) /* arbitrary but <= 255 */
>> + ep->rep_remote_cma.responder_resources = 32;
>> + else
>> ep->rep_remote_cma.responder_resources = devattr.max_qp_rd_atom;
>> - ep->rep_remote_cma.initiator_depth = 0;
>>
>> ep->rep_remote_cma.retry_count = 7;
>> ep->rep_remote_cma.flow_control = 0;
>> @@ -858,14 +841,6 @@ if
>(strnicmp(ia->ri_id->device->dma_device->bus->name, "pci", 3) == 0) {
>> }
>> }
>>
>> - /* Theoretically a client initiator_depth > 0 is not needed,
>> - * but many peers fail to complete the connection unless they
>> - * == responder_resources! */
>> - if (ep->rep_remote_cma.initiator_depth !=
>> - ep->rep_remote_cma.responder_resources)
>> - ep->rep_remote_cma.initiator_depth =
>> - ep->rep_remote_cma.responder_resources;
>> -
>> ep->rep_connected = 0;
>>
>> rc = rdma_connect(ia->ri_id, &ep->rep_remote_cma);
>> @@ -894,14 +869,16 @@ if
>(strnicmp(ia->ri_id->device->dma_device->bus->name, "pci", 3) == 0) {
>> if (ep->rep_connected <= 0) {
>> /* Sometimes, the only way to reliably connect to remote
>> * CMs is to use same nonzero values for ORD and IRD. */
>> - ep->rep_remote_cma.initiator_depth =
>> - ep->rep_remote_cma.responder_resources;
>> - if (ep->rep_remote_cma.initiator_depth == 0)
>> - ++ep->rep_remote_cma.initiator_depth;
>> - if (ep->rep_remote_cma.responder_resources == 0)
>> - ++ep->rep_remote_cma.responder_resources;
>> - if (retry_count++ == 0)
>> + if (retry_count++ <= RDMA_CONNECT_RETRY_MAX + 1 &&
>> + (ep->rep_remote_cma.responder_resources == 0 ||
>> + ep->rep_remote_cma.initiator_depth !=
>> + ep->rep_remote_cma.responder_resources)) {
>> + if (ep->rep_remote_cma.responder_resources == 0)
>> + ep->rep_remote_cma.responder_resources = 1;
>> + ep->rep_remote_cma.initiator_depth =
>> + ep->rep_remote_cma.responder_resources;
>> goto retry;
>> + }
>> rc = ep->rep_connected;
>> } else {
>> dprintk("RPC: %s: connected\n", __func__);
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html