copyin_iovec is a helper which wraps copyin and selects the right copy
method based on the iter_copy_type.
Signed-off-by: Joe Damato <[email protected]>
---
lib/iov_iter.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index d32d7e5..6720cb2 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -168,6 +168,15 @@ static int copyin(void *to, const void __user *from, size_t n)
return n;
}
+static int copyin_iovec(void *to, const void __user *from, size_t n,
+ struct iov_iter *i)
+{
+ if (unlikely(iov_iter_copy_is_nt(i)))
+ return __copy_from_user_nocache(to, from, n);
+ else
+ return copyin(to, from, n);
+}
+
static size_t copy_page_to_iter_iovec(struct page *page, size_t offset, size_t bytes,
struct iov_iter *i)
{
@@ -278,7 +287,7 @@ static size_t copy_page_from_iter_iovec(struct page *page, size_t offset, size_t
to = kaddr + offset;
/* first chunk, usually the only one */
- left = copyin(to, buf, copy);
+ left = copyin_iovec(to, buf, copy, i);
copy -= left;
skip += copy;
to += copy;
@@ -288,7 +297,7 @@ static size_t copy_page_from_iter_iovec(struct page *page, size_t offset, size_t
iov++;
buf = iov->iov_base;
copy = min(bytes, iov->iov_len);
- left = copyin(to, buf, copy);
+ left = copyin_iovec(to, buf, copy, i);
copy -= left;
skip = copy;
to += copy;
@@ -307,7 +316,7 @@ static size_t copy_page_from_iter_iovec(struct page *page, size_t offset, size_t
kaddr = kmap(page);
to = kaddr + offset;
- left = copyin(to, buf, copy);
+ left = copyin_iovec(to, buf, copy, i);
copy -= left;
skip += copy;
to += copy;
@@ -316,7 +325,7 @@ static size_t copy_page_from_iter_iovec(struct page *page, size_t offset, size_t
iov++;
buf = iov->iov_base;
copy = min(bytes, iov->iov_len);
- left = copyin(to, buf, copy);
+ left = copyin_iovec(to, buf, copy, i);
copy -= left;
skip = copy;
to += copy;
@@ -766,7 +775,7 @@ size_t _copy_from_iter(void *addr, size_t bytes, struct iov_iter *i)
if (iter_is_iovec(i))
might_fault();
iterate_and_advance(i, bytes, base, len, off,
- copyin(addr + off, base, len),
+ copyin_iovec(addr + off, base, len, i),
memcpy(addr + off, base, len)
)
--
2.7.4
On Sun, Jun 12, 2022 at 01:57:52AM -0700, Joe Damato wrote:
> copyin_iovec is a helper which wraps copyin and selects the right copy
> method based on the iter_copy_type.
>
> Signed-off-by: Joe Damato <[email protected]>
> ---
> lib/iov_iter.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index d32d7e5..6720cb2 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -168,6 +168,15 @@ static int copyin(void *to, const void __user *from, size_t n)
> return n;
> }
>
> +static int copyin_iovec(void *to, const void __user *from, size_t n,
> + struct iov_iter *i)
> +{
> + if (unlikely(iov_iter_copy_is_nt(i)))
> + return __copy_from_user_nocache(to, from, n);
> + else
> + return copyin(to, from, n);
> +}
Just a sanity check - your testing is *not* with KASAN/KCSAN, right?
And BTW, why is that only on the userland side? If you are doing
that at all, it would make sense to cover the memcpy() side as
well...
On Mon, Jun 13, 2022 at 04:25:39AM +0000, Al Viro wrote:
> On Sun, Jun 12, 2022 at 01:57:52AM -0700, Joe Damato wrote:
> > copyin_iovec is a helper which wraps copyin and selects the right copy
> > method based on the iter_copy_type.
> >
> > Signed-off-by: Joe Damato <[email protected]>
> > ---
> > lib/iov_iter.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> > index d32d7e5..6720cb2 100644
> > --- a/lib/iov_iter.c
> > +++ b/lib/iov_iter.c
> > @@ -168,6 +168,15 @@ static int copyin(void *to, const void __user *from, size_t n)
> > return n;
> > }
> >
> > +static int copyin_iovec(void *to, const void __user *from, size_t n,
> > + struct iov_iter *i)
> > +{
> > + if (unlikely(iov_iter_copy_is_nt(i)))
> > + return __copy_from_user_nocache(to, from, n);
> > + else
> > + return copyin(to, from, n);
> > +}
>
> Just a sanity check - your testing is *not* with KASAN/KCSAN, right?
Yes, that is correct.
> And BTW, why is that only on the userland side? If you are doing
> that at all, it would make sense to cover the memcpy() side as
> well...
I assume here you mean the memcpy() in the splice() path? I do have a
separate change I've been testing which does this, but I thought that can
be sent separately.
This RFC basically takes an existing kernel feature (tx-nocache-copy) and
makes it applicable to more protocols and more fine grained so that it does
not need to be enabled interface-wide. The memcpy() change you mention is,
in my mind, a separate change which adds a new feature and can be sent if
this change is accepted upstream.
Let me know if that makes sense and if there are any issues you think I
should address before I send a v1 for consideration.
Thanks for taking a look!
From: Joe Damato
> Sent: 12 June 2022 09:58
>
> copyin_iovec is a helper which wraps copyin and selects the right copy
> method based on the iter_copy_type.
A pretty bad description.
> Signed-off-by: Joe Damato <[email protected]>
> ---
> lib/iov_iter.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index d32d7e5..6720cb2 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -168,6 +168,15 @@ static int copyin(void *to, const void __user *from, size_t n)
> return n;
> }
>
> +static int copyin_iovec(void *to, const void __user *from, size_t n,
> + struct iov_iter *i)
> +{
> + if (unlikely(iov_iter_copy_is_nt(i)))
> + return __copy_from_user_nocache(to, from, n);
> + else
> + return copyin(to, from, n);
> +}
Isn't this extra conditional going to have a measurable impact
on all the normal copy paths?
The additional costs of all the 'iovec' types is bad enough
already.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, Jun 13, 2022 at 07:53:19AM +0000, David Laight wrote:
> From: Joe Damato
> > Sent: 12 June 2022 09:58
> >
> > copyin_iovec is a helper which wraps copyin and selects the right copy
> > method based on the iter_copy_type.
>
> A pretty bad description.
Thanks, David. I'll be sure to fix the commit description in the next
revision.
> > Signed-off-by: Joe Damato <[email protected]>
> > ---
> > lib/iov_iter.c | 19 ++++++++++++++-----
> > 1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> > index d32d7e5..6720cb2 100644
> > --- a/lib/iov_iter.c
> > +++ b/lib/iov_iter.c
> > @@ -168,6 +168,15 @@ static int copyin(void *to, const void __user *from, size_t n)
> > return n;
> > }
> >
> > +static int copyin_iovec(void *to, const void __user *from, size_t n,
> > + struct iov_iter *i)
> > +{
> > + if (unlikely(iov_iter_copy_is_nt(i)))
> > + return __copy_from_user_nocache(to, from, n);
> > + else
> > + return copyin(to, from, n);
> > +}
>
> Isn't this extra conditional going to have a measurable impact
> on all the normal copy paths?
The kernel already does a conditional for tx-nocache-copy on TCP sockets
when copying skbs to check for the NETIF_F_NOCACHE_COPY bit, but I hear
what you are saying.
I suppose I could push the NT copy check logic out of iov_iter, but to do
that I think I'd probably have to significantly refactor the iov code to
break apart copy_page_from_iter_iovec.
I'll spend a bit more time thinking through this, but I'm open to
suggestions if you have one; the benefit of supporting non-temporal copies
in this path is pretty significant, so I hope a path forward can be found.
> The additional costs of all the 'iovec' types is bad enough
> already.
Do you have data you can share on this?
Thanks for taking a look!
From: Joe Damato
> Sent: 13 June 2022 15:43
...
> > The additional costs of all the 'iovec' types is bad enough
> > already.
>
> Do you have data you can share on this?
I was thinking of the performance drop noticed when (IIRC)
reads/writes of /dev/null were pushed through the iovec code.
But there is a lot of overhead code for the usual case
of a single user buffer being copied.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)