[PATCH] ib_srp: initialize dma_length in srp_map_idb

[PATCH] ib_srp: initialize dma_length in srp_map_idb

[Christoph Hellwig]

Without this sg_dma_len will return 0 on architectures tha have
the dma_length field.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 32f7962..445c0a6 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1520,6 +1520,9 @@ static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req,
 		state.sg_nents = 1;
 		sg_set_buf(idb_sg, req->indirect_desc, idb_len);
 		idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
+#ifdef CONFIG_NEED_SG_DMA_LENGTH
+		idb_sg->dma_length = idb_sg->length;	      /* hack^2 */
+#endif
 		ret = srp_map_finish_fr(&state, ch);
 		if (ret < 0)
 			return ret;
-- 
1.9.1

Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb

[Sagi Grimberg]

On 15/11/2015 19:59, Christoph Hellwig wrote:
> Without this sg_dma_len will return 0 on architectures tha have
> the dma_length field.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 32f7962..445c0a6 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1520,6 +1520,9 @@ static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req,
>   		state.sg_nents = 1;
>   		sg_set_buf(idb_sg, req->indirect_desc, idb_len);
>   		idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
> +#ifdef CONFIG_NEED_SG_DMA_LENGTH
> +		idb_sg->dma_length = idb_sg->length;	      /* hack^2 */
> +#endif

:)

We should really get this properly map/unmap per IO at some point.
Probably do it in both code paths...

Having said that,

Looks fine,

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb

[Or Gerlitz]

On Sun, Nov 15, 2015, Sagi Grimberg <sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> On 15/11/2015 19:59, Christoph Hellwig wrote:

>> Without this sg_dma_len will return 0 on architectures tha have
>> the dma_length field.

and what wrong with that?

Christoph, probably typo here? "tha" needs to be "that"

>> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
>> ---
>>   drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
>> b/drivers/infiniband/ulp/srp/ib_srp.c
>> index 32f7962..445c0a6 100644
>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>> @@ -1520,6 +1520,9 @@ static int srp_map_idb(struct srp_rdma_ch *ch,
>> struct srp_request *req,
>>                 state.sg_nents = 1;
>>                 sg_set_buf(idb_sg, req->indirect_desc, idb_len);
>>                 idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
>> +#ifdef CONFIG_NEED_SG_DMA_LENGTH
>> +               idb_sg->dma_length = idb_sg->length;          /* hack^2 */
>> +#endif
>
>
> :)
>
> We should really get this properly map/unmap per IO at some point.
> Probably do it in both code paths...

Sagi, can you please elaborate a little further on the problem, srpt
WA, what do we do in isert and what is the proposed not WA solution?

> Having said that,
> Looks fine,
> Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb

[Sagi Grimberg]

On 15/11/2015 23:10, Or Gerlitz wrote:
> On Sun, Nov 15, 2015, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:
>> On 15/11/2015 19:59, Christoph Hellwig wrote:
>
>>> Without this sg_dma_len will return 0 on architectures tha have
>>> the dma_length field.
>
> and what wrong with that?

Because it's not length 0. It's because I did a (documented) hack
with the new memory registration API. I hope we can fix it soon.

>
> Christoph, probably typo here? "tha" needs to be "that"
>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>>    drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
>>> b/drivers/infiniband/ulp/srp/ib_srp.c
>>> index 32f7962..445c0a6 100644
>>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>>> @@ -1520,6 +1520,9 @@ static int srp_map_idb(struct srp_rdma_ch *ch,
>>> struct srp_request *req,
>>>                  state.sg_nents = 1;
>>>                  sg_set_buf(idb_sg, req->indirect_desc, idb_len);
>>>                  idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
>>> +#ifdef CONFIG_NEED_SG_DMA_LENGTH
>>> +               idb_sg->dma_length = idb_sg->length;          /* hack^2 */
>>> +#endif
>>
>>
>> :)
>>
>> We should really get this properly map/unmap per IO at some point.
>> Probably do it in both code paths...
>
> Sagi, can you please elaborate a little further on the problem, srpt
> WA,

It's srp initiator. This falls in the case where srp sends an indirect
buffer descriptor to the target (which is registered). For this
descriptor it uses a pre-dma-mapped buffer, so in order to fit it into
the new memory registration scheme (which works on SG lists), I hacked
a single entry SG and used the pre-dma-mapped address. What was missing
is the dma_length setting (because we didn't dma mapped the SG).

The proper fix is doing correct map/unmap per IO.

> what do we do in isert and what is the proposed not WA solution?

iser does not have indirect buffer descriptors, only a single rkey
(per-direction).

Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb

[Bart Van Assche]

On 11/15/2015 09:59 AM, Christoph Hellwig wrote:
> Without this sg_dma_len will return 0 on architectures tha have
> the dma_length field.
>
> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 32f7962..445c0a6 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1520,6 +1520,9 @@ static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req,
>   		state.sg_nents = 1;
>   		sg_set_buf(idb_sg, req->indirect_desc, idb_len);
>   		idb_sg->dma_address = req->indirect_dma_addr; /* hack! */
> +#ifdef CONFIG_NEED_SG_DMA_LENGTH
> +		idb_sg->dma_length = idb_sg->length;	      /* hack^2 */
> +#endif
>   		ret = srp_map_finish_fr(&state, ch);
>   		if (ret < 0)
>   			return ret;
>

Hello Christoph,

How about adding "Cc: stable" to this patch such that it not only will 
be integrated in kernel v4.4 but also in kernel v4.3.1 or later ?

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb

[Christoph Hellwig]

Hi Bart,

the code in this area changed enough since 4.3 that it won't easily
apply.  But a backport would still be very useful!

Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb

[Bart Van Assche]

On 11/16/2015 09:22 AM, Christoph Hellwig wrote:
> the code in this area changed enough since 4.3 that it won't easily
> apply.  But a backport would still be very useful!

In that case:

Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>