[PATCH v2] IB/core: remove dead code

[PATCH v2] IB/core: remove dead code

[Sudip Mukherjee]

prot_sg_cnt has been assigned with the value of ret which we have
already checked to be non-zero so prot_sg_cnt can never be zero at this
point of the code and hence the else part can never execute.
And since we know prot_sg_cnt is non zero there is no use for the
if condition also.

Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
---
 drivers/infiniband/core/rw.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
index 1eb9b12..a829a14 100644
--- a/drivers/infiniband/core/rw.c
+++ b/drivers/infiniband/core/rw.c
@@ -386,21 +386,17 @@ int rdma_rw_ctx_signature_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
 	count += ret;
 	prev_wr = &ctx->sig->data.reg_wr.wr;
 
-	if (prot_sg_cnt) {
-		ret = rdma_rw_init_one_mr(qp, port_num, &ctx->sig->prot,
-				prot_sg, prot_sg_cnt, 0);
-		if (ret < 0)
-			goto out_destroy_data_mr;
-		count += ret;
+	ret = rdma_rw_init_one_mr(qp, port_num, &ctx->sig->prot,
+				  prot_sg, prot_sg_cnt, 0);
+	if (ret < 0)
+		goto out_destroy_data_mr;
+	count += ret;
 
-		if (ctx->sig->prot.inv_wr.next)
-			prev_wr->next = &ctx->sig->prot.inv_wr;
-		else
-			prev_wr->next = &ctx->sig->prot.reg_wr.wr;
-		prev_wr = &ctx->sig->prot.reg_wr.wr;
-	} else {
-		ctx->sig->prot.mr = NULL;
-	}
+	if (ctx->sig->prot.inv_wr.next)
+		prev_wr->next = &ctx->sig->prot.inv_wr;
+	else
+		prev_wr->next = &ctx->sig->prot.reg_wr.wr;
+	prev_wr = &ctx->sig->prot.reg_wr.wr;
 
 	ctx->sig->sig_mr = ib_mr_pool_get(qp, &qp->sig_mrs);
 	if (!ctx->sig->sig_mr) {
-- 
1.9.1

Re: [PATCH v2] IB/core: remove dead code

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2078 bytes --]

On Mon, Jun 06, 2016 at 10:15:06PM +0100, Sudip Mukherjee wrote:
> prot_sg_cnt has been assigned with the value of ret which we have
> already checked to be non-zero so prot_sg_cnt can never be zero at this
> point of the code and hence the else part can never execute.
> And since we know prot_sg_cnt is non zero there is no use for the
> if condition also.
> 
> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>

And what about other places?

428         if (prot_sg_cnt)
429                 ctx->sig->sig_wr.prot = &ctx->sig->prot.sge;

....


455         if (prot_sg_cnt)
456                 ib_mr_pool_put(qp, &qp->rdma_mrs, ctx->sig->prot.mr);

....


> ---
>  drivers/infiniband/core/rw.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> index 1eb9b12..a829a14 100644
> --- a/drivers/infiniband/core/rw.c
> +++ b/drivers/infiniband/core/rw.c
> @@ -386,21 +386,17 @@ int rdma_rw_ctx_signature_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
>  	count += ret;
>  	prev_wr = &ctx->sig->data.reg_wr.wr;
>  
> -	if (prot_sg_cnt) {
> -		ret = rdma_rw_init_one_mr(qp, port_num, &ctx->sig->prot,
> -				prot_sg, prot_sg_cnt, 0);
> -		if (ret < 0)
> -			goto out_destroy_data_mr;
> -		count += ret;
> +	ret = rdma_rw_init_one_mr(qp, port_num, &ctx->sig->prot,
> +				  prot_sg, prot_sg_cnt, 0);
> +	if (ret < 0)
> +		goto out_destroy_data_mr;
> +	count += ret;
>  
> -		if (ctx->sig->prot.inv_wr.next)
> -			prev_wr->next = &ctx->sig->prot.inv_wr;
> -		else
> -			prev_wr->next = &ctx->sig->prot.reg_wr.wr;
> -		prev_wr = &ctx->sig->prot.reg_wr.wr;
> -	} else {
> -		ctx->sig->prot.mr = NULL;
> -	}
> +	if (ctx->sig->prot.inv_wr.next)
> +		prev_wr->next = &ctx->sig->prot.inv_wr;
> +	else
> +		prev_wr->next = &ctx->sig->prot.reg_wr.wr;
> +	prev_wr = &ctx->sig->prot.reg_wr.wr;
>  
>  	ctx->sig->sig_mr = ib_mr_pool_get(qp, &qp->sig_mrs);
>  	if (!ctx->sig->sig_mr) {
> -- 
> 1.9.1
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] IB/core: remove dead code

[Sagi Grimberg]

On 07/06/16 00:15, Sudip Mukherjee wrote:
> prot_sg_cnt has been assigned with the value of ret which we have
> already checked to be non-zero so prot_sg_cnt can never be zero at this
> point of the code and hence the else part can never execute.
> And since we know prot_sg_cnt is non zero there is no use for the
> if condition also.
>
> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
> ---
>   drivers/infiniband/core/rw.c | 24 ++++++++++--------------
>   1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/infiniband/core/rw.c b/drivers/infiniband/core/rw.c
> index 1eb9b12..a829a14 100644
> --- a/drivers/infiniband/core/rw.c
> +++ b/drivers/infiniband/core/rw.c
> @@ -386,21 +386,17 @@ int rdma_rw_ctx_signature_init(struct rdma_rw_ctx *ctx, struct ib_qp *qp,
>   	count += ret;
>   	prev_wr = &ctx->sig->data.reg_wr.wr;
>
> -	if (prot_sg_cnt) {
> -		ret = rdma_rw_init_one_mr(qp, port_num, &ctx->sig->prot,
> -				prot_sg, prot_sg_cnt, 0);
> -		if (ret < 0)
> -			goto out_destroy_data_mr;
> -		count += ret;
> +	ret = rdma_rw_init_one_mr(qp, port_num, &ctx->sig->prot,
> +				  prot_sg, prot_sg_cnt, 0);
> +	if (ret < 0)
> +		goto out_destroy_data_mr;
> +	count += ret;
>
> -		if (ctx->sig->prot.inv_wr.next)
> -			prev_wr->next = &ctx->sig->prot.inv_wr;
> -		else
> -			prev_wr->next = &ctx->sig->prot.reg_wr.wr;
> -		prev_wr = &ctx->sig->prot.reg_wr.wr;
> -	} else {
> -		ctx->sig->prot.mr = NULL;
> -	}
> +	if (ctx->sig->prot.inv_wr.next)
> +		prev_wr->next = &ctx->sig->prot.inv_wr;
> +	else
> +		prev_wr->next = &ctx->sig->prot.reg_wr.wr;
> +	prev_wr = &ctx->sig->prot.reg_wr.wr;
>
>   	ctx->sig->sig_mr = ib_mr_pool_get(qp, &qp->sig_mrs);
>   	if (!ctx->sig->sig_mr) {
>

Actually it looks like the data-integrity insert/strip operation
(where protection sg list does not exist) is broken.

It looks that the protection scatterlist should be done only if
prot_sg_count was provided...

I don't have access to mlx5 devices at the moment (still waiting
to get some...)

Re: [PATCH v2] IB/core: remove dead code

[Christoph Hellwig]

On Tue, Jun 07, 2016 at 03:41:04PM +0300, Sagi Grimberg wrote:
> Actually it looks like the data-integrity insert/strip operation
> (where protection sg list does not exist) is broken.
> 
> It looks that the protection scatterlist should be done only if
> prot_sg_count was provided...
> 
> I don't have access to mlx5 devices at the moment (still waiting
> to get some...)

I've reported the brokenness in the existing iSER code when I started
working on signature MR support in the generic API.  As no one helped
managed to figure out the issue or how it should operate I clone the
existing semantics, even given I knew they are broken.

Re: [PATCH v2] IB/core: remove dead code

[Sagi Grimberg]

>> Actually it looks like the data-integrity insert/strip operation
>> (where protection sg list does not exist) is broken.
>>
>> It looks that the protection scatterlist should be done only if
>> prot_sg_count was provided...
>>
>> I don't have access to mlx5 devices at the moment (still waiting
>> to get some...)

Hey Christoph,

> I've reported the brokenness in the existing iSER code when I started
> working on signature MR support in the generic API.  As no one helped
> managed to figure out the issue or how it should operate I clone the
> existing semantics, even given I knew they are broken.

There are two modes for the insert/strip operation, one is with PI on
memory and without on the wire (which is useful when having a target
working against legacy initiators), and one is with PI on the wire but
not in memory. You reported that the latter was broken (the less
useful case) and my understanding is that it was a result of
the target<->isert interaction. The last time I used the first mode
it worked fine.

I'll try to get my hands on some mlx5 devices soon and have a look into
this. I still want to change the API so it will hide all this noise from
the consumer but I need to find some spare cycles for it.

In the meantime, CC'ing Max from Mellanox and maybe he can check if
PI insert/strip are still alive.

Cheers,
Sagi.