| Message ID | 1447152255-28231-3-git-send-email-sagig@mellanox.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Nov 10, 2015 at 12:44:14PM +0200, Sagi Grimberg wrote: > --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c > +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c > @@ -238,7 +238,7 @@ int rdma_read_chunk_frmr(struct svcxprt_rdma *xprt, > read = min_t(int, nents << PAGE_SHIFT, rs_length); > > frmr->direction = DMA_FROM_DEVICE; > - frmr->access_flags = (IB_ACCESS_LOCAL_WRITE|IB_ACCESS_REMOTE_WRITE); > + frmr->access_flags = xprt->sc_cm_id->device->rdma_read_access_flags; > frmr->sg_nents = nents; I think you can simply drop the access_flags member and use rdma_read_access_flags directly later in the function. -- 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
Oh, and while we're at it. Can someone explain why we're even using rdma_read_chunk_frmr for IB? It seems to work around the fact tat iWarp only allow a single RDMA READ SGE, but it's used whenever the device has IB_DEVICE_MEM_MGT_EXTENSIONS, which seems wrong. -- 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 10/11/2015 13:38, Christoph Hellwig wrote: > On Tue, Nov 10, 2015 at 12:44:14PM +0200, Sagi Grimberg wrote: >> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c >> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c >> @@ -238,7 +238,7 @@ int rdma_read_chunk_frmr(struct svcxprt_rdma *xprt, >> read = min_t(int, nents << PAGE_SHIFT, rs_length); >> >> frmr->direction = DMA_FROM_DEVICE; >> - frmr->access_flags = (IB_ACCESS_LOCAL_WRITE|IB_ACCESS_REMOTE_WRITE); >> + frmr->access_flags = xprt->sc_cm_id->device->rdma_read_access_flags; >> frmr->sg_nents = nents; > > I think you can simply drop the access_flags member and use > rdma_read_access_flags directly later in the function. > I will. -- 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 10/11/2015 13:41, Christoph Hellwig wrote: > Oh, and while we're at it. Can someone explain why we're even > using rdma_read_chunk_frmr for IB? It seems to work around the > fact tat iWarp only allow a single RDMA READ SGE, but it's used > whenever the device has IB_DEVICE_MEM_MGT_EXTENSIONS, which seems > wrong. I think Steve can answer it better than I can. I think that it is just to have a single code path for both IB and iWARP. I agree that the condition seems wrong and for small transfers rdma_read_chunk_frmr is really a performance loss. -- 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/10/2015 5:46 AM, Sagi Grimberg wrote: > > > On 10/11/2015 13:41, Christoph Hellwig wrote: >> Oh, and while we're at it. Can someone explain why we're even >> using rdma_read_chunk_frmr for IB? It seems to work around the >> fact tat iWarp only allow a single RDMA READ SGE, but it's used >> whenever the device has IB_DEVICE_MEM_MGT_EXTENSIONS, which seems >> wrong. > > I think Steve can answer it better than I can. I think that it is > just to have a single code path for both IB and iWARP. I agree that > the condition seems wrong and for small transfers rdma_read_chunk_frmr > is really a performance loss. This was probably just an oversight/mistake. The focus was on enabling iWARP at the time. -- 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 Nov 10, 2015, at 6:38 AM, Christoph Hellwig <hch@infradead.org> wrote: > > On Tue, Nov 10, 2015 at 12:44:14PM +0200, Sagi Grimberg wrote: >> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c >> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c >> @@ -238,7 +238,7 @@ int rdma_read_chunk_frmr(struct svcxprt_rdma *xprt, >> read = min_t(int, nents << PAGE_SHIFT, rs_length); >> >> frmr->direction = DMA_FROM_DEVICE; >> - frmr->access_flags = (IB_ACCESS_LOCAL_WRITE|IB_ACCESS_REMOTE_WRITE); >> + frmr->access_flags = xprt->sc_cm_id->device->rdma_read_access_flags; >> frmr->sg_nents = nents; > > I think you can simply drop the access_flags member and use > rdma_read_access_flags directly later in the function. > Oh, and while we're at it. Can someone explain why we're even > using rdma_read_chunk_frmr for IB? It seems to work around the > fact tat iWarp only allow a single RDMA READ SGE, but it's used > whenever the device has IB_DEVICE_MEM_MGT_EXTENSIONS, which seems > wrong. Christoph: I agree on both points. Sagi: the overall approach looks fine to me. Will wait for you to repost with Christoph’s first suggestion addressed. -- Chuck Lever -- 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/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c index 303f194970f9..692c0142c7b6 100644 --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c @@ -238,7 +238,7 @@ int rdma_read_chunk_frmr(struct svcxprt_rdma *xprt, read = min_t(int, nents << PAGE_SHIFT, rs_length); frmr->direction = DMA_FROM_DEVICE; - frmr->access_flags = (IB_ACCESS_LOCAL_WRITE|IB_ACCESS_REMOTE_WRITE); + frmr->access_flags = xprt->sc_cm_id->device->rdma_read_access_flags; frmr->sg_nents = nents; for (pno = 0; pno < nents; pno++) {
Instead of hard-coding remote access (which is not secured issue in IB). Signed-off-by: Sagi Grimberg <sagig@mellanox.com> --- net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)