[PATCH v2 0/6] Series short description

[PATCH v2 0/6] Series short description

[Chuck Lever]

The following series implements...

---

Chuck Lever (6):
      xprtrdma: Remove FMR support in rpcrdma_convert_iovs()
      xprtrdma: Simplify rpcrdma_convert_kvec() and frwr_map()
      xprtrdma: Refactor invocations of offset_in_page()
      rpcrdma: Fix comments about reverse-direction operation
      xprtrdma: Pad optimization, revisited
      rpcrdma: Capture bytes received in Receive completion tracepoints


 include/trace/events/rpcrdma.h             | 50 +++++++++++++++++++++-
 net/sunrpc/xprtrdma/backchannel.c          |  4 +-
 net/sunrpc/xprtrdma/frwr_ops.c             | 12 ++----
 net/sunrpc/xprtrdma/rpc_rdma.c             | 17 +++-----
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c |  4 +-
 net/sunrpc/xprtrdma/xprt_rdma.h            | 15 ++++---
 6 files changed, 68 insertions(+), 34 deletions(-)

--
Chuck Lever

[PATCH v2 1/6] xprtrdma: Remove FMR support in rpcrdma_convert_iovs()

[Chuck Lever]

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) {
 		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;
 }

Re: [PATCH v2 0/6] Series short description

[Chuck Lever]

Misfire, please ignore.

> On Feb 3, 2021, at 11:20 AM, Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> The following series implements...
> 
> ---
> 
> Chuck Lever (6):
>      xprtrdma: Remove FMR support in rpcrdma_convert_iovs()
>      xprtrdma: Simplify rpcrdma_convert_kvec() and frwr_map()
>      xprtrdma: Refactor invocations of offset_in_page()
>      rpcrdma: Fix comments about reverse-direction operation
>      xprtrdma: Pad optimization, revisited
>      rpcrdma: Capture bytes received in Receive completion tracepoints
> 
> 
> include/trace/events/rpcrdma.h             | 50 +++++++++++++++++++++-
> net/sunrpc/xprtrdma/backchannel.c          |  4 +-
> net/sunrpc/xprtrdma/frwr_ops.c             | 12 ++----
> net/sunrpc/xprtrdma/rpc_rdma.c             | 17 +++-----
> net/sunrpc/xprtrdma/svc_rdma_backchannel.c |  4 +-
> net/sunrpc/xprtrdma/xprt_rdma.h            | 15 ++++---
> 6 files changed, 68 insertions(+), 34 deletions(-)
> 
> --
> Chuck Lever
> 

--
Chuck Lever

Re: [PATCH v2 1/6] xprtrdma: Remove FMR support in rpcrdma_convert_iovs()

[Chuck Lever]

> 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

Re: [PATCH v2 1/6] xprtrdma: Remove FMR support in rpcrdma_convert_iovs()

[Tom Talpey]

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;
>   }
> 
> 
> 

[PATCH v2 1/6] xprtrdma: Remove FMR support in rpcrdma_convert_iovs()

[Chuck Lever]

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) {
 		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;
 }