| Message ID | 161236943446.1030487.4542967452464402073.stgit@manet.1015granger.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RPC/RDMA client fixes | expand |
On 2/3/2021 11:23 AM, Chuck Lever wrote: > Support for FMR was removed by commit ba69cd122ece ("xprtrdma: > Remove support for FMR memory registration") [Dec 2018]. That means > the buffer-splitting behavior of rpcrdma_convert_kvec(), added by > commit 821c791a0bde ("xprtrdma: Segment head and tail XDR buffers > on page boundaries") [Mar 2016], is no longer necessary. FRWR > memory registration handles this case with aplomb. > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > net/sunrpc/xprtrdma/rpc_rdma.c | 19 ++++--------------- > 1 file changed, 4 insertions(+), 15 deletions(-) > > diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c > index 8f5d0cb68360..832765f3ebba 100644 > --- a/net/sunrpc/xprtrdma/rpc_rdma.c > +++ b/net/sunrpc/xprtrdma/rpc_rdma.c > @@ -204,9 +204,7 @@ rpcrdma_alloc_sparse_pages(struct xdr_buf *buf) > return 0; > } > > -/* Split @vec on page boundaries into SGEs. FMR registers pages, not > - * a byte range. Other modes coalesce these SGEs into a single MR > - * when they can. > +/* Convert @vec to a single SGL element. > * > * Returns pointer to next available SGE, and bumps the total number > * of SGEs consumed. > @@ -215,21 +213,12 @@ static struct rpcrdma_mr_seg * > rpcrdma_convert_kvec(struct kvec *vec, struct rpcrdma_mr_seg *seg, > unsigned int *n) > { > - u32 remaining, page_offset; > - char *base; > - > - base = vec->iov_base; > - page_offset = offset_in_page(base); > - remaining = vec->iov_len; > - while (remaining) { > + if (vec->iov_len) { This is weird. Is it ever possible for a zero-length segment to be passed? If so, it's obviously wrong to return an uninitialized "seg" to the caller. I'd suggest simply asserting that iov_len is != 0. I guess this was an issue in the existing code, but there, it would only trigger if *all* the segs were zero. > seg->mr_page = NULL; > - seg->mr_offset = base; > - seg->mr_len = min_t(u32, PAGE_SIZE - page_offset, remaining); > - remaining -= seg->mr_len; > - base += seg->mr_len; > + seg->mr_offset = vec->iov_base; I thought the previous discussion was that this should be offset_in_page not the (virtual) iov_base. Tom. > + seg->mr_len = vec->iov_len; > ++seg; > ++(*n); > - page_offset = 0; > } > return seg; > } > > >
> On Feb 3, 2021, at 1:06 PM, Tom Talpey <tom@talpey.com> wrote: > > On 2/3/2021 11:23 AM, Chuck Lever wrote: >> Support for FMR was removed by commit ba69cd122ece ("xprtrdma: >> Remove support for FMR memory registration") [Dec 2018]. That means >> the buffer-splitting behavior of rpcrdma_convert_kvec(), added by >> commit 821c791a0bde ("xprtrdma: Segment head and tail XDR buffers >> on page boundaries") [Mar 2016], is no longer necessary. FRWR >> memory registration handles this case with aplomb. >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> net/sunrpc/xprtrdma/rpc_rdma.c | 19 ++++--------------- >> 1 file changed, 4 insertions(+), 15 deletions(-) >> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c >> index 8f5d0cb68360..832765f3ebba 100644 >> --- a/net/sunrpc/xprtrdma/rpc_rdma.c >> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c >> @@ -204,9 +204,7 @@ rpcrdma_alloc_sparse_pages(struct xdr_buf *buf) >> return 0; >> } >> -/* Split @vec on page boundaries into SGEs. FMR registers pages, not >> - * a byte range. Other modes coalesce these SGEs into a single MR >> - * when they can. >> +/* Convert @vec to a single SGL element. >> * >> * Returns pointer to next available SGE, and bumps the total number >> * of SGEs consumed. >> @@ -215,21 +213,12 @@ static struct rpcrdma_mr_seg * >> rpcrdma_convert_kvec(struct kvec *vec, struct rpcrdma_mr_seg *seg, >> unsigned int *n) >> { >> - u32 remaining, page_offset; >> - char *base; >> - >> - base = vec->iov_base; >> - page_offset = offset_in_page(base); >> - remaining = vec->iov_len; >> - while (remaining) { >> + if (vec->iov_len) { > > This is weird. Is it ever possible for a zero-length segment to be > passed? If so, it's obviously wrong to return an uninitialized "seg" > to the caller. I'd suggest simply asserting that iov_len is != 0. > > I guess this was an issue in the existing code, but there, it would > only trigger if *all* the segs were zero. It would be a bug if head.iov_len was zero. tail.iov_len is checked before the call. I think that means this check is superfluous and can be removed. > >> seg->mr_page = NULL; >> - seg->mr_offset = base; >> - seg->mr_len = min_t(u32, PAGE_SIZE - page_offset, remaining); >> - remaining -= seg->mr_len; >> - base += seg->mr_len; >> + seg->mr_offset = vec->iov_base; > > I thought the previous discussion was that this should be offset_in_page > not the (virtual) iov_base. Addressed in 3/6? > > Tom. > >> + seg->mr_len = vec->iov_len; >> ++seg; >> ++(*n); >> - page_offset = 0; >> } >> return seg; >> } -- Chuck Lever
diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c index 8f5d0cb68360..832765f3ebba 100644 --- a/net/sunrpc/xprtrdma/rpc_rdma.c +++ b/net/sunrpc/xprtrdma/rpc_rdma.c @@ -204,9 +204,7 @@ rpcrdma_alloc_sparse_pages(struct xdr_buf *buf) return 0; } -/* Split @vec on page boundaries into SGEs. FMR registers pages, not - * a byte range. Other modes coalesce these SGEs into a single MR - * when they can. +/* Convert @vec to a single SGL element. * * Returns pointer to next available SGE, and bumps the total number * of SGEs consumed. @@ -215,21 +213,12 @@ static struct rpcrdma_mr_seg * rpcrdma_convert_kvec(struct kvec *vec, struct rpcrdma_mr_seg *seg, unsigned int *n) { - u32 remaining, page_offset; - char *base; - - base = vec->iov_base; - page_offset = offset_in_page(base); - remaining = vec->iov_len; - while (remaining) { + if (vec->iov_len) { seg->mr_page = NULL; - seg->mr_offset = base; - seg->mr_len = min_t(u32, PAGE_SIZE - page_offset, remaining); - remaining -= seg->mr_len; - base += seg->mr_len; + seg->mr_offset = vec->iov_base; + seg->mr_len = vec->iov_len; ++seg; ++(*n); - page_offset = 0; } return seg; }
Support for FMR was removed by commit ba69cd122ece ("xprtrdma: Remove support for FMR memory registration") [Dec 2018]. That means the buffer-splitting behavior of rpcrdma_convert_kvec(), added by commit 821c791a0bde ("xprtrdma: Segment head and tail XDR buffers on page boundaries") [Mar 2016], is no longer necessary. FRWR memory registration handles this case with aplomb. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- net/sunrpc/xprtrdma/rpc_rdma.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-)