svcrdma: Limit ORD based on client's advertised IRD
Subsystems (3)
PATCH 1/1 svcrdma: Limit ORD based on client's advertised IRD
When adapters have differing IRD limits, the RDMA transport will fail to connect properly. The RDMA transport should use the client's advertised inbound read limit when computing it's outbound read limit. For iWARP transports, there is no exchange of the IRD/ORD during connect request and so the only way to ensure compatability is to use the /proc/sys/sunrpc/svc_rdma/max_outbound_read_requests file to control server ORD.
---
net/sunrpc/xprtrdma/svc_rdma_transport.c | 16 ++++++++++++---- 1 files changed, 12 insertions(+), 4 deletions(-)
net/sunrpc/xprtrdma/svc_rdma_transport.c +12 -4
@@ -580,7 +580,7 @@ int svc_rdma_post_recv(struct svcxprt_rdma *xprt)* will call the recvfrom method on the listen xprt which will accept the new * connection. */ -static void handle_connect_req (struct rdma_cm_id *new_cma_id) +static void handle_connect_req (struct rdma_cm_id *new_cma_id,u8 client_ird) {struct svcxprt_rdma *listen_xprt = new_cma_id->context;struct svcxprt_rdma *newxprt;
@@ -597,6 +597,9 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id)dprintk ("svcrdma: Creating newxprt=%p, cm_id=%p, listenxprt=%p\n" , newxprt, newxprt->sc_cm_id, listen_xprt); +/* Save client advertised inbound read limit for use later in accept. */ + newxprt->sc_ord = client_ird; +/* Set the local and remote addresses in the transport */ sa = (struct sockaddr *)&newxprt->sc_cm_id->route.addr.dst_addr;svc_xprt_set_remote (&newxprt->sc_xprt, sa,svc_addr_len (sa));
@@ -633,7 +636,8 @@ static int rdma_listen_handler(struct rdma_cm_id *cma_id,case RDMA_CM_EVENT_CONNECT_REQUEST :dprintk ("svcrdma: Connect request on cma_id=%p, xprt = %p, " "event=%d\n" , cma_id, cma_id->context, event->event); -handle_connect_req (cma_id); +handle_connect_req (cma_id, + event->param.conn.responder_resources);break ;case RDMA_CM_EVENT_ESTABLISHED :
@@ -807,8 +811,12 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt) (size_t)svcrdma_max_requests); newxprt->sc_sq_depth =RPCRDMA_SQ_DEPTH_MULT * newxprt->sc_max_requests; - newxprt->sc_ord =min ((size_t )devattr.max_qp_rd_atom, - (size_t )svcrdma_ord); +/* +* Limit ORD based on client limit, local device limit, and +* configured svcrdma limit. +*/ + newxprt->sc_ord =min ((size_t )devattr.max_qp_rd_atom, newxprt->sc_ord); + newxprt->sc_ord =min ((size_t )svcrdma_ord, newxprt->sc_ord); newxprt->sc_pd =ib_alloc_pd (newxprt->sc_cm_id->device);if (IS_ERR (newxprt->sc_pd)) {
A message that is not in the archive (sent off-list or not yet received): 1211214842.31725.38.camel-SMNkleLxa3ZimH42XvhXlA@public.gmane.org
Quoted 7 lines · Tom Tucker
When adapters have differing IRD limits, the RDMA transport will fail to connect properly. The RDMA transport should use the client's advertised inbound read limit when computing it's outbound read limit. For iWARP transports, there is no exchange of the IRD/ORD during connect request and so the only way to ensure compatability is to use the
/proc/sys/sunrpc/svc_rdma/max_outbound_read_requestsfile to control server ORD.
Well, technically the iWARP connection manager *currently* does not exchange these values. I think the comment would be more accurate if it said "in the absence of"...
net/sunrpc/xprtrdma/svc_rdma_transport.c int svc_rdma_post_recv(struct svcxprt_rdma *xprt) · line 583
Quoted 2 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +1
-1-static void handle_connect_req (struct rdma_cm_id *new_cma_id) +static void handle_connect_req (struct rdma_cm_id *new_cma_id,u8 client_ird)
While "u8" might be the limit of the implementations today, wouldn't "int" be easier to understand, and more portable?
net/sunrpc/xprtrdma/svc_rdma_transport.c static void handle_connect_req(struct rdma_cm_id *new_cma_id) · line 602
Quoted 3 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +3
-0+/* Save client advertised inbound read limit for use later in accept. */ + newxprt->sc_ord = client_ird; +
and...
net/sunrpc/xprtrdma/svc_rdma_transport.c static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt) · line 819
Quoted 6 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +6
-0+/* +* Limit ORD based on client limit, local device limit, and +* configured svcrdma limit. +*/ + newxprt->sc_ord =min ((size_t )devattr.max_qp_rd_atom, newxprt->sc_ord); + newxprt->sc_ord =min ((size_t )svcrdma_ord, newxprt->sc_ord);
Looks good, but it's a little odd that the value goes into sc_ord in one routine, only to be changed before being used. I guess the device attrs aren't handy at the time the connection is notified though, so ok.
Tom.
A message that is not in the archive (sent off-list or not yet received): RTPCLUEXC1-PRD1q4gt00000114-rtwIt2gI0FxT+ZUat5FNkAK/[email protected]
I got dropped of the cc: somehow--please try not to prune cc's. I have enough trouble keeping up without the additional confusion to my mail filters....
--b.
Quoted 40 lines · Thomas
Quoted 7 lines · Tom Tucker
When adapters have differing IRD limits, the RDMA transport will fail to connect properly. The RDMA transport should use the client's advertised inbound read limit when computing it's outbound read limit. For iWARP transports, there is no exchange of the IRD/ORD during connect request and so the only way to ensure compatability is to use the
/proc/sys/sunrpc/svc_rdma/max_outbound_read_requestsfile to control server ORD.Well, technically the iWARP connection manager *currently* does not exchange these values. I think the comment would be more accurate if it said "in the absence of"...
net/sunrpc/xprtrdma/svc_rdma_transport.c int svc_rdma_post_recv(struct svcxprt_rdma *xprt) · line 583
Quoted 2 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +1
-1-static void handle_connect_req (struct rdma_cm_id *new_cma_id) +static void handle_connect_req (struct rdma_cm_id *new_cma_id,u8 client_ird)While "u8" might be the limit of the implementations today, wouldn't "int" be easier to understand, and more portable?
net/sunrpc/xprtrdma/svc_rdma_transport.c static void handle_connect_req(struct rdma_cm_id *new_cma_id) · line 602
Quoted 3 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +3
-0+/* Save client advertised inbound read limit for use later in accept. */ + newxprt->sc_ord = client_ird; +and...
net/sunrpc/xprtrdma/svc_rdma_transport.c static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt) · line 819
Quoted 6 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +6
-0+/* +* Limit ORD based on client limit, local device limit, and +* configured svcrdma limit. +*/ + newxprt->sc_ord =min ((size_t )devattr.max_qp_rd_atom, newxprt->sc_ord); + newxprt->sc_ord =min ((size_t )svcrdma_ord, newxprt->sc_ord);Looks good, but it's a little odd that the value goes into
sc_ordin one routine, only to be changed before being used. I guess the device attrs aren't handy at the time the connection is notified though, so ok.Tom.
--
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
Quoted 9 lines · Thomas
Quoted 7 lines · Tom Tucker
When adapters have differing IRD limits, the RDMA transport will fail to connect properly. The RDMA transport should use the client's advertised inbound read limit when computing it's outbound read limit. For iWARP transports, there is no exchange of the IRD/ORD during connect request and so the only way to ensure compatability is to use the
/proc/sys/sunrpc/svc_rdma/max_outbound_read_requestsfile to control server ORD.
Quoted 3 lines
Well, technically the iWARP connection manager *currently* does not exchange these values. I think the comment would be more accurate if it said "in the absence of"...
I'm not sure it really has anything to do with where it is implemented. The iWARP protocol(s) do not specify how or if IRD/ORD will be exchanged so this commit doesn't help them. The note is meant to be a hint to users of the transport how to get around potential connection issues between iWARP adapters with differing capabilities.
The two spelling errors should be fixed though ;-)
Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c int svc_rdma_post_recv(struct svcxprt_rdma *xprt) · line 583
Quoted 3 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +1
-1-static void handle_connect_req (struct rdma_cm_id *new_cma_id) +static void handle_connect_req (struct rdma_cm_id *new_cma_id,u8 client_ird)While "u8" might be the limit of the implementations today, wouldn't "int" be easier to understand, and more portable?
I think that 'int' might provoke the "wrath of Chuck". Unsigned something else --
Quoted 4 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c static void handle_connect_req(struct rdma_cm_id *new_cma_id) · line 602
Quoted 3 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +3
-0+/* Save client advertised inbound read limit for use later in accept. */ + newxprt->sc_ord = client_ird; +and...
Quoted 11 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt) · line 819
Quoted 6 lines
net/sunrpc/xprtrdma/svc_rdma_transport.c +6
-0+/* +* Limit ORD based on client limit, local device limit, and +* configured svcrdma limit. +*/ + newxprt->sc_ord =min ((size_t )devattr.max_qp_rd_atom, newxprt->sc_ord); + newxprt->sc_ord =min ((size_t )svcrdma_ord, newxprt->sc_ord);Looks good, but it's a little odd that the value goes into
sc_ordin one routine, only to be changed before being used. I guess the device attrs aren't handy at the time the connection is notified though, so ok.
Yes. It's the initial candidate for ORD that may be updated later based on configuration and/or device capabilities.
Quoted 1 line
Tom.
A message that is not in the archive (sent off-list or not yet received): 1211221933.31725.56.camel-SMNkleLxa3ZimH42XvhXlA@public.gmane.org
Quoted 5 lines · Tom Tucker
Quoted 3 lines
Well, technically the iWARP connection manager *currently* does not exchange these values. I think the comment would be more accurate if it said "in the absence of"...
I'm not sure it really has anything to do with where it is implemented.
That wasn't my point - I meant, in the absence of information what the peer's maximum might be, the server has to make something up. But, future iwarp cm's could pass it.
What value bubbles up in this field from current iWARP connections? Is it total garbage, or at least, recognizably useless (-1)?
Quoted 10 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +1
-1-static void handle_connect_req(struct rdma_cm_id *new_cma_id) +static void handle_connect_req(struct rdma_cm_id *new_cma_id, u8
client_ird)Quoted 4 lines
While "u8" might be the limit of the implementations today, wouldn't "int" be easier to understand, and more portable?
I think that 'int' might provoke the "wrath of Chuck". Unsigned something else --
Okay, well if not "int", then at least some type which is not a specific storage size. u{8,16,32} etc should be reserved for describing hardware and on-wire stuff, and handled as native when used locally.
Tom.
A message that is not in the archive (sent off-list or not yet received): RTPCLUEXC1-PRD4mG0E00000117-rtwIt2gI0FxT+ZUat5FNkAK/[email protected]
Quoted 5 lines · Thomas
Quoted 4 lines · Tom Tucker
Quoted 3 lines
Well, technically the iWARP connection manager *currently* does not exchange these values. I think the comment would be more accurate if it said "in the absence of"...
Quoted 8 lines
Quoted 1 line
I'm not sure it really has anything to do with where it is implemented.
That wasn't my point - I meant, in the absence of information what the peer's maximum might be, the server has to make something up. But, future iwarp cm's could pass it.
What value bubbles up in this field from current iWARP connections? Is it total garbage, or at least, recognizably useless (-1)?
For iWARP it is currently the local device's max. When iWARP (hopefully) gets a standard for this -- it will be the remote peer's IRD. This code won't need to change.
Quoted 17 lines
Quoted 11 lines
Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +1
-1-static void handle_connect_req(struct rdma_cm_id *new_cma_id) +static void handle_connect_req(struct rdma_cm_id *new_cma_id, u8
client_ird)Quoted 4 lines
While "u8" might be the limit of the implementations today, wouldn't "int" be easier to understand, and more portable?
I think that 'int' might provoke the "wrath of Chuck". Unsigned something else --
Okay, well if not "int", then at least some type which is not a specific storage size. u{8,16,32} etc should be reserved for describing hardware and on-wire stuff, and handled as native when used locally.
Tom.