Re: [PATCH v1] xprtrdma: Simplify rpcrdma_convert_kvec() and frwr_map()

[Tom Talpey <tom@talpey.com>]

On 2/2/2021 2:20 PM, Chuck Lever wrote:
> 
> 
>> On Feb 2, 2021, at 2:18 PM, Tom Talpey <tom@talpey.com> wrote:
>>
>> What's not to like about a log that uses the words "with aplomb"? :)
>>
>> Minor related comment/question below.
>>
>> On 2/2/2021 9:42 AM, Chuck Lever wrote:
>>> Clean up.
>>> 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.
>>> A related simplification removes an extra conditional branch from
>>> the SGL set-up loop in frwr_map(): Instead of using either
>>> sg_set_page() or sg_set_buf(), initialize the mr_page field properly
>>> when rpcrdma_convert_kvec() converts the kvec to an SGL entry.
>>> frwr_map() can then invoke sg_set_page() unconditionally.
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>   net/sunrpc/xprtrdma/frwr_ops.c  |   10 ++--------
>>>   net/sunrpc/xprtrdma/rpc_rdma.c  |   21 +++++----------------
>>>   net/sunrpc/xprtrdma/xprt_rdma.h |    2 +-
>>>   3 files changed, 8 insertions(+), 25 deletions(-)
>>> diff --git a/net/sunrpc/xprtrdma/frwr_ops.c b/net/sunrpc/xprtrdma/frwr_ops.c
>>> index baca49fe83af..5eb044a5f0be 100644
>>> --- a/net/sunrpc/xprtrdma/frwr_ops.c
>>> +++ b/net/sunrpc/xprtrdma/frwr_ops.c
>>> @@ -306,14 +306,8 @@ struct rpcrdma_mr_seg *frwr_map(struct rpcrdma_xprt *r_xprt,
>>>   	if (nsegs > ep->re_max_fr_depth)
>>>   		nsegs = ep->re_max_fr_depth;
>>>   	for (i = 0; i < nsegs;) {
>>> -		if (seg->mr_page)
>>> -			sg_set_page(&mr->mr_sg[i],
>>> -				    seg->mr_page,
>>> -				    seg->mr_len,
>>> -				    offset_in_page(seg->mr_offset));
>>> -		else
>>> -			sg_set_buf(&mr->mr_sg[i], seg->mr_offset,
>>> -				   seg->mr_len);
>>> +		sg_set_page(&mr->mr_sg[i], seg->mr_page,
>>> +			    seg->mr_len, offset_in_page(seg->mr_offset));
>>>     		++seg;
>>>   		++i;
>>> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c
>>> index 8f5d0cb68360..529adb6ad4db 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) {
>>> -		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;
>>> +	if (vec->iov_len) {
>>> +		seg->mr_page = virt_to_page(vec->iov_base);
>>> +		seg->mr_offset = vec->iov_base;
>>> +		seg->mr_len = vec->iov_len;
>>>   		++seg;
>>>   		++(*n);
>>> -		page_offset = 0;
>>>   	}
>>>   	return seg;
>>>   }
>>> diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_rdma.h
>>> index 94b28657aeeb..4a9fe6592795 100644
>>> --- a/net/sunrpc/xprtrdma/xprt_rdma.h
>>> +++ b/net/sunrpc/xprtrdma/xprt_rdma.h
>>> @@ -285,7 +285,7 @@ enum {
>>>     struct rpcrdma_mr_seg {		/* chunk descriptors */
>>>   	u32		mr_len;		/* length of chunk or segment */
>>> -	struct page	*mr_page;	/* owning page, if any */
>>> +	struct page	*mr_page;	/* underlying struct page */
>>>   	char		*mr_offset;	/* kva if no page, else offset */
>>
>> Is this comment ("kva if no page") actually correct? The hunk just
>> above is an example of a case where mr_page is set, yet mr_offset
>> is an iov_base. Is iov_base not a kva?
> 
> Ah, well the "if no page" part is now obsolete.
> 
> I suppose it should be set to "offset_in_page(vec->iov_base)" ?

Seems like it, yes. Assuming that only the first element in the sgl
has a possibly non-zero offset ("FBO"). All others must be zero for
the FRMR.

Is it guaranteed that each kvec is at most one physical page? If not,
then the length may span into a random physical page, that was not
necessarily contiguous in the original KVA-addressed buffer.

Tom.