[PATCH] svcrdma: delete double assignment

[PATCH] svcrdma: delete double assignment

[Himangi Saraogi]

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 <himangi774@gmail.com>
---
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

Re: [PATCH] svcrdma: delete double assignment

[David Miller]

From: Himangi Saraogi <himangi774@gmail.com>
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 <himangi774@gmail.com>

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.

Re: [PATCH] svcrdma: delete double assignment

[Steve Wise]

On 7/29/2014 5:50 PM, David Miller wrote:
> From: Himangi Saraogi <himangi774@gmail.com>
> 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 <himangi774@gmail.com>
> 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.