Delete successive assignments to the same location.
A simplified version of Coccinelle semantic match that finds this problem is as
follows:
// <smpl>
@@
expression i;
@@
*i = ...;
i = ...;
// </smpl>
Signed-off-by: Himangi Saraogi <[email protected]>
---
Should the assignment be the maximum of the 2 values?
net/sunrpc/xprtrdma/svc_rdma_transport.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index 06a5d92..8976529 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -956,7 +956,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
dprintk("svcrdma: failed to create QP, ret=%d\n", ret);
goto errout;
}
- newxprt->sc_max_sge = qp_attr.cap.max_send_sge;
newxprt->sc_max_sge = qp_attr.cap.max_recv_sge;
newxprt->sc_sq_depth = qp_attr.cap.max_send_wr;
newxprt->sc_max_requests = qp_attr.cap.max_recv_wr;
--
1.9.1
On 7/29/2014 5:50 PM, David Miller wrote:
> From: Himangi Saraogi <[email protected]>
> Date: Mon, 28 Jul 2014 20:59:38 +0530
>
>> Delete successive assignments to the same location.
>>
>> A simplified version of Coccinelle semantic match that finds this problem is as
>> follows:
>>
>> // <smpl>
>> @@
>> expression i;
>> @@
>>
>> *i = ...;
>> i = ...;
>> // </smpl>
>>
>> Signed-off-by: Himangi Saraogi <[email protected]>
> I am not so sure about this change either.
>
>> @@ -956,7 +956,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
>> dprintk("svcrdma: failed to create QP, ret=%d\n", ret);
>> goto errout;
>> }
>> - newxprt->sc_max_sge = qp_attr.cap.max_send_sge;
>> newxprt->sc_max_sge = qp_attr.cap.max_recv_sge;
>> newxprt->sc_sq_depth = qp_attr.cap.max_send_wr;
>> newxprt->sc_max_requests = qp_attr.cap.max_recv_wr;
> ->sc_max_sge is used to limit the number of segments used during sends,
> currently in this code. Grep for where it is used.
>
> Therefore, if anything, the correct thing to do would be to retain the
> first line rather than the second line.
>
> Someone who actually works on this code and understands it should
> really take a close look at this before anyone even thinks about
> applying this patch.
We should remove that whole block that tries to recover from a
rdma_create_qp() failure. I don't believe we would ever hit it. The
value initially stored in sc_max_sge is from the device attribute
max_sge returned from ib_query_device() just above the code we're
talking about. If rdma_create_qp() fails, it would not be because the
max_send_sge and max_recv_sge values passed in exceed the device's max...
Like this:
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c
b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index e7323fb..282a43b 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -942,23 +942,8 @@ static struct svc_xprt *svc_rdma_accept(struct
svc_xprt *xprt)
ret = rdma_create_qp(newxprt->sc_cm_id, newxprt->sc_pd, &qp_attr);
if (ret) {
- /*
- * XXX: This is a hack. We need a xx_request_qp interface
- * that will adjust the qp_attr's with a best-effort
- * number
- */
- qp_attr.cap.max_send_sge -= 2;
- qp_attr.cap.max_recv_sge -= 2;
- ret = rdma_create_qp(newxprt->sc_cm_id, newxprt->sc_pd,
- &qp_attr);
- if (ret) {
- dprintk("svcrdma: failed to create QP,
ret=%d\n", ret);
- goto errout;
- }
- newxprt->sc_max_sge = qp_attr.cap.max_send_sge;
- newxprt->sc_max_sge = qp_attr.cap.max_recv_sge;
- newxprt->sc_sq_depth = qp_attr.cap.max_send_wr;
- newxprt->sc_max_requests = qp_attr.cap.max_recv_wr;
+ dprintk("svcrdma: failed to create QP, ret=%d\n", ret);
+ goto errout;
}
newxprt->sc_qp = newxprt->sc_cm_id->qp;
Steve.
From: Himangi Saraogi <[email protected]>
Date: Mon, 28 Jul 2014 20:59:38 +0530
> Delete successive assignments to the same location.
>
> A simplified version of Coccinelle semantic match that finds this problem is as
> follows:
>
> // <smpl>
> @@
> expression i;
> @@
>
> *i = ...;
> i = ...;
> // </smpl>
>
> Signed-off-by: Himangi Saraogi <[email protected]>
I am not so sure about this change either.
> @@ -956,7 +956,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
> dprintk("svcrdma: failed to create QP, ret=%d\n", ret);
> goto errout;
> }
> - newxprt->sc_max_sge = qp_attr.cap.max_send_sge;
> newxprt->sc_max_sge = qp_attr.cap.max_recv_sge;
> newxprt->sc_sq_depth = qp_attr.cap.max_send_wr;
> newxprt->sc_max_requests = qp_attr.cap.max_recv_wr;
->sc_max_sge is used to limit the number of segments used during sends,
currently in this code. Grep for where it is used.
Therefore, if anything, the correct thing to do would be to retain the
first line rather than the second line.
Someone who actually works on this code and understands it should
really take a close look at this before anyone even thinks about
applying this patch.