| Message ID | 1447610393-2899-1-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? 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, what do we do in isert and what is the proposed not WA solution? > Having said that, > Looks fine, > Reviewed-by: Sagi Grimberg <sagig@mellanox.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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). -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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; > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(+)