[v7,02/23] iov_iter: DDP copy to iter/pages

Commit Message

Aurelien Aptel Oct. 25, 2022, 1:59 p.m. UTC

From: Ben Ben-Ishay <[e-mail]>

When using direct data placement (DDP) the NIC writes some of the payload
directly to the destination buffer, and constructs SKBs such that they
point to this data. To skip copies when SKB data already resides in the
destination we use the newly introduced routines in this commit, which
check if (src == dst), and skip the copy when that's true.

As the current user for these routines is in the block layer (nvme-tcp),
then we only apply the change for bio_vec. Other routines use the normal
methods for copying.

Signed-off-by: Ben Ben-Ishay <[e-mail]>
Signed-off-by: Boris Pismenny <[e-mail]>
Signed-off-by: Or Gerlitz <[e-mail]>
Signed-off-by: Yoray Zack <[e-mail]>
Signed-off-by: Shai Malin <[e-mail]>
Signed-off-by: Aurelien Aptel <[e-mail]>
Reviewed-by: Chaitanya Kulkarni <[e-mail]>
---
 lib/iov_iter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christoph Hellwig Oct. 25, 2022, 4:01 p.m. UTC | #1

I don't think this is a good subject line.  What the patch does is
to skip the memcpy, so something about that in the subject.  You
can then explain the commit log why that is done.  And given that
the behavior isn't all that obvious I think a big fat comment in the
code would be very helpful in this case as well.

Jakub Kicinski Oct. 25, 2022, 10:40 p.m. UTC | #2

On Tue, 25 Oct 2022 16:59:37 +0300 Aurelien Aptel wrote:
> Signed-off-by: Ben Ben-Ishay <[e-mail]>
> Signed-off-by: Boris Pismenny <[e-mail]>
> Signed-off-by: Or Gerlitz <[e-mail]>
> Signed-off-by: Yoray Zack <[e-mail]>
> Signed-off-by: Shai Malin <[e-mail]>
> Signed-off-by: Aurelien Aptel <[e-mail]>

Great stuff :) Please get someone who matters to ack this.

> Reviewed-by: Chaitanya Kulkarni <[e-mail]>
> ---
>  lib/iov_iter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index c3ca28ca68a6..75470a4b8ab3 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -526,7 +526,7 @@ size_t _copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i)
>  		might_fault();
>  	iterate_and_advance(i, bytes, base, len, off,
>  		copyout(base, addr + off, len),
> -		memcpy(base, addr + off, len)
> +		(base != addr + off) && memcpy(base, addr + off, len)

Aurelien Aptel Oct. 26, 2022, 4:05 p.m. UTC | #3

Hi Christoph,

Christoph Hellwig <[e-mail]> writes:
> I don't think this is a good subject line.  What the patch does is
> to skip the memcpy, so something about that in the subject.  You

Sure, we will use the following subject:

  iov_iter: skip copy if src == dst for direct data placement

> can then explain the commit log why that is done.  And given that
> the behavior isn't all that obvious I think a big fat comment in the
> code would be very helpful in this case as well.

And we will add the big comment.

Patch

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index c3ca28ca68a6..75470a4b8ab3 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -526,7 +526,7 @@  size_t _copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i)
 		might_fault();
 	iterate_and_advance(i, bytes, base, len, off,
 		copyout(base, addr + off, len),
-		memcpy(base, addr + off, len)
+		(base != addr + off) && memcpy(base, addr + off, len)
 	)
 
 	return bytes;