Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb
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 --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb
On Sun, Nov 15, 2015, Sagi Grimberg 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 >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb
On 15/11/2015 23:10, Or Gerlitz wrote: On Sun, Nov 15, 2015, Sagi Grimberg 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 --- 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). -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb
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 --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb
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! -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ib_srp: initialize dma_length in srp_map_idb
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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
