[PATCH net-next] scm: optimize put_cmsg()

[PATCH net-next] scm: optimize put_cmsg()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Calling two copy_to_user() for very small regions has very high overhead.

Switch to inlined unsafe_put_user() to save one stac/clac sequence,
and avoid copy_to_user().

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
---
 net/core/scm.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/core/scm.c b/net/core/scm.c
index 8156d4fb8a3966122fdfcfd0ebc9e5520aa7b67c..bd96c922041d22a2f3b7ee73e4b3183316f9b616 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -228,14 +228,16 @@ int put_cmsg(struct msghdr * msg, int level, int type, int len, void *data)
 
 	if (msg->msg_control_is_user) {
 		struct cmsghdr __user *cm = msg->msg_control_user;
-		struct cmsghdr cmhdr;
 
-		cmhdr.cmsg_level = level;
-		cmhdr.cmsg_type = type;
-		cmhdr.cmsg_len = cmlen;
-		if (copy_to_user(cm, &cmhdr, sizeof cmhdr) ||
-		    copy_to_user(CMSG_USER_DATA(cm), data, cmlen - sizeof(*cm)))
-			return -EFAULT;
+		if (!user_write_access_begin(cm, cmlen))
+			goto efault;
+
+		unsafe_put_user(len, &cm->cmsg_len, efault_end);
+		unsafe_put_user(level, &cm->cmsg_level, efault_end);
+		unsafe_put_user(type, &cm->cmsg_type, efault_end);
+		unsafe_copy_to_user(CMSG_USER_DATA(cm), data,
+				    cmlen - sizeof(*cm), efault_end);
+		user_write_access_end();
 	} else {
 		struct cmsghdr *cm = msg->msg_control;
 
@@ -249,6 +251,11 @@ int put_cmsg(struct msghdr * msg, int level, int type, int len, void *data)
 	msg->msg_control += cmlen;
 	msg->msg_controllen -= cmlen;
 	return 0;
+
+efault_end:
+	user_write_access_end();
+efault:
+	return -EFAULT;
 }
 EXPORT_SYMBOL(put_cmsg);
 
-- 
2.31.1.368.gbe11c130af-goog

Re: [PATCH net-next] scm: optimize put_cmsg()

[Soheil Hassas Yeganeh]

On Thu, Apr 15, 2021 at 1:38 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
>
> Calling two copy_to_user() for very small regions has very high overhead.
>
> Switch to inlined unsafe_put_user() to save one stac/clac sequence,
> and avoid copy_to_user().
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>

Acked-by: Soheil Hassas Yeganeh <soheil@google.com>

Very nice! Thank you, Eric!

> ---
>  net/core/scm.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/net/core/scm.c b/net/core/scm.c
> index 8156d4fb8a3966122fdfcfd0ebc9e5520aa7b67c..bd96c922041d22a2f3b7ee73e4b3183316f9b616 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -228,14 +228,16 @@ int put_cmsg(struct msghdr * msg, int level, int type, int len, void *data)
>
>         if (msg->msg_control_is_user) {
>                 struct cmsghdr __user *cm = msg->msg_control_user;
> -               struct cmsghdr cmhdr;
>
> -               cmhdr.cmsg_level = level;
> -               cmhdr.cmsg_type = type;
> -               cmhdr.cmsg_len = cmlen;
> -               if (copy_to_user(cm, &cmhdr, sizeof cmhdr) ||
> -                   copy_to_user(CMSG_USER_DATA(cm), data, cmlen - sizeof(*cm)))
> -                       return -EFAULT;
> +               if (!user_write_access_begin(cm, cmlen))
> +                       goto efault;
> +
> +               unsafe_put_user(len, &cm->cmsg_len, efault_end);
> +               unsafe_put_user(level, &cm->cmsg_level, efault_end);
> +               unsafe_put_user(type, &cm->cmsg_type, efault_end);
> +               unsafe_copy_to_user(CMSG_USER_DATA(cm), data,
> +                                   cmlen - sizeof(*cm), efault_end);
> +               user_write_access_end();
>         } else {
>                 struct cmsghdr *cm = msg->msg_control;
>
> @@ -249,6 +251,11 @@ int put_cmsg(struct msghdr * msg, int level, int type, int len, void *data)
>         msg->msg_control += cmlen;
>         msg->msg_controllen -= cmlen;
>         return 0;
> +
> +efault_end:
> +       user_write_access_end();
> +efault:
> +       return -EFAULT;
>  }
>  EXPORT_SYMBOL(put_cmsg);
>
> --
> 2.31.1.368.gbe11c130af-goog
>

Re: [PATCH net-next] scm: optimize put_cmsg()

[Jakub Kicinski]

On Thu, 15 Apr 2021 10:37:53 -0700 Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Calling two copy_to_user() for very small regions has very high overhead.
> 
> Switch to inlined unsafe_put_user() to save one stac/clac sequence,
> and avoid copy_to_user().
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>

Hi Eric!

This appears to break boot on my systems.

IDK how exactly, looks like systemd gets stuck waiting for nondescript
services to start in initramfs. I have lots of debug enabled and didn't
spot anything of note in kernel logs.

I'll try to poke at this more, but LMK if you have any ideas. The
commit looks "obviously correct" :S

Re: [PATCH net-next] scm: optimize put_cmsg()

[Eric Dumazet]

On Fri, Apr 16, 2021 at 7:57 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 15 Apr 2021 10:37:53 -0700 Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Calling two copy_to_user() for very small regions has very high overhead.
> >
> > Switch to inlined unsafe_put_user() to save one stac/clac sequence,
> > and avoid copy_to_user().
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Soheil Hassas Yeganeh <soheil@google.com>
>
> Hi Eric!
>
> This appears to break boot on my systems.
>
> IDK how exactly, looks like systemd gets stuck waiting for nondescript
> services to start in initramfs. I have lots of debug enabled and didn't
> spot anything of note in kernel logs.
>
> I'll try to poke at this more, but LMK if you have any ideas. The
> commit looks "obviously correct" :S

Oops, my rebase went wong, sorry for that

Can you check this  patch (on top of the buggy one) ?

If that works, I'll submit a v2

diff --git a/net/core/scm.c b/net/core/scm.c
index bd96c922041d22a2f3b7ee73e4b3183316f9b616..ae3085d9aae8adb81d3bb42c8a915a205476a0ee
100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -232,7 +232,7 @@ int put_cmsg(struct msghdr * msg, int level, int
type, int len, void *data)
                if (!user_write_access_begin(cm, cmlen))
                        goto efault;

-               unsafe_put_user(len, &cm->cmsg_len, efault_end);
+               unsafe_put_user(cmlen, &cm->cmsg_len, efault_end);
                unsafe_put_user(level, &cm->cmsg_level, efault_end);
                unsafe_put_user(type, &cm->cmsg_type, efault_end);
                unsafe_copy_to_user(CMSG_USER_DATA(cm), data,

Re: [PATCH net-next] scm: optimize put_cmsg()

[Jakub Kicinski]

On Fri, 16 Apr 2021 20:28:40 +0200 Eric Dumazet wrote:
> On Fri, Apr 16, 2021 at 7:57 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu, 15 Apr 2021 10:37:53 -0700 Eric Dumazet wrote:  
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > > Calling two copy_to_user() for very small regions has very high overhead.
> > >
> > > Switch to inlined unsafe_put_user() to save one stac/clac sequence,
> > > and avoid copy_to_user().
> > >
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > Cc: Soheil Hassas Yeganeh <soheil@google.com>  
> >
> > Hi Eric!
> >
> > This appears to break boot on my systems.
> >
> > IDK how exactly, looks like systemd gets stuck waiting for nondescript
> > services to start in initramfs. I have lots of debug enabled and didn't
> > spot anything of note in kernel logs.
> >
> > I'll try to poke at this more, but LMK if you have any ideas. The
> > commit looks "obviously correct" :S  
> 
> Oops, my rebase went wong, sorry for that

Ah, my eyes failed to spot that :)

> Can you check this  patch (on top of the buggy one) ?
> 
> If that works, I'll submit a v2

It's already merged. Let me try the fix now...

> diff --git a/net/core/scm.c b/net/core/scm.c
> index bd96c922041d22a2f3b7ee73e4b3183316f9b616..ae3085d9aae8adb81d3bb42c8a915a205476a0ee
> 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -232,7 +232,7 @@ int put_cmsg(struct msghdr * msg, int level, int
> type, int len, void *data)
>                 if (!user_write_access_begin(cm, cmlen))
>                         goto efault;
> 
> -               unsafe_put_user(len, &cm->cmsg_len, efault_end);
> +               unsafe_put_user(cmlen, &cm->cmsg_len, efault_end);
>                 unsafe_put_user(level, &cm->cmsg_level, efault_end);
>                 unsafe_put_user(type, &cm->cmsg_type, efault_end);
>                 unsafe_copy_to_user(CMSG_USER_DATA(cm), data,

Re: [PATCH net-next] scm: optimize put_cmsg()

[Eric Dumazet]

On Fri, Apr 16, 2021 at 8:29 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 16 Apr 2021 20:28:40 +0200 Eric Dumazet wrote:
> > On Fri, Apr 16, 2021 at 7:57 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Thu, 15 Apr 2021 10:37:53 -0700 Eric Dumazet wrote:
> > > > From: Eric Dumazet <edumazet@google.com>
> > > >
> > > > Calling two copy_to_user() for very small regions has very high overhead.
> > > >
> > > > Switch to inlined unsafe_put_user() to save one stac/clac sequence,
> > > > and avoid copy_to_user().
> > > >
> > > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > > Cc: Soheil Hassas Yeganeh <soheil@google.com>
> > >
> > > Hi Eric!
> > >
> > > This appears to break boot on my systems.
> > >
> > > IDK how exactly, looks like systemd gets stuck waiting for nondescript
> > > services to start in initramfs. I have lots of debug enabled and didn't
> > > spot anything of note in kernel logs.
> > >
> > > I'll try to poke at this more, but LMK if you have any ideas. The
> > > commit looks "obviously correct" :S
> >
> > Oops, my rebase went wong, sorry for that
>
> Ah, my eyes failed to spot that :)
>
> > Can you check this  patch (on top of the buggy one) ?
> >
> > If that works, I'll submit a v2
>
> It's already merged. Let me try the fix now...

I have sent the official patch, thanks for this fast feedback !

Re: [PATCH net-next] scm: optimize put_cmsg()

[Naresh Kamboju]

Hi Eric,

On Fri, 16 Apr 2021 at 23:27, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 15 Apr 2021 10:37:53 -0700 Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Calling two copy_to_user() for very small regions has very high overhead.
> >
> > Switch to inlined unsafe_put_user() to save one stac/clac sequence,
> > and avoid copy_to_user().
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Soheil Hassas Yeganeh <soheil@google.com>
>
> Hi Eric!
>
> This appears to break boot on my systems.

I have been noticing this problem.

>
> IDK how exactly, looks like systemd gets stuck waiting for nondescript
> services to start in initramfs. I have lots of debug enabled and didn't
> spot anything of note in kernel logs.

We (LKFT) are still seeing this problem only on arm architecture on
next-20210416 tag onwards. our bisect script points to this commit.

Steps to reproduce:
- build linux next latest next-20210423 tag with below config
     -  kernel-config:
https://builds.tuxbuild.com/1reqrnNLnHEX9uEZFngRfaoJa9E/config
- boot qemu-arm with below command
   -  /usr/bin/qemu-system-aarch64 -cpu host,aarch64=off -machine
virt-2.10,accel=kvm -nographic -net
nic,model=virtio,macaddr=BA:DD:AD:CC:09:04 -net tap -m 2048 -monitor
none -kernel zImage --append "console=ttyAMA0 root=/dev/vda rw" -hda
rpb-console-image-lkft-am57xx-evm-20201022181203-3085.rootfs.ext4 -m
4096 -smp 2 -nographic

- After the mount rootfs - the systemd gets stuck

>
> I'll try to poke at this more, but LMK if you have any ideas. The
> commit looks "obviously correct" :S

May I request to investigate this on arm architecture.
The qemu_arm boot failed link,
https://lkft.validation.linaro.org/scheduler/job/2565371#L540

The qemu_arm boot pass after the reverting this patch,
    commit 38ebcf5096a86762b82262e96b2c8b170fe79040
    scm: optimize put_cmsg()

on the latest linux next tags i have to revert two commits.
   "scm: fix a typo in put_cmsg()"
   "scm: optimize put_cmsg()"

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>

--
Linaro LKFT
https://lkft.linaro.org

Re: [PATCH net-next] scm: optimize put_cmsg()

[Eric Dumazet]

On Sun, Apr 25, 2021 at 9:59 PM Naresh Kamboju
<naresh.kamboju@linaro.org> wrote:
>
> Hi Eric,
>
> On Fri, 16 Apr 2021 at 23:27, Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Thu, 15 Apr 2021 10:37:53 -0700 Eric Dumazet wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > > Calling two copy_to_user() for very small regions has very high overhead.
> > >
> > > Switch to inlined unsafe_put_user() to save one stac/clac sequence,
> > > and avoid copy_to_user().
> > >
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > Cc: Soheil Hassas Yeganeh <soheil@google.com>
> >
> > Hi Eric!
> >
> > This appears to break boot on my systems.
>
> I have been noticing this problem.
>
> >
> > IDK how exactly, looks like systemd gets stuck waiting for nondescript
> > services to start in initramfs. I have lots of debug enabled and didn't
> > spot anything of note in kernel logs.
>
> We (LKFT) are still seeing this problem only on arm architecture on
> next-20210416 tag onwards. our bisect script points to this commit.
>
> Steps to reproduce:
> - build linux next latest next-20210423 tag with below config
>      -  kernel-config:
> https://builds.tuxbuild.com/1reqrnNLnHEX9uEZFngRfaoJa9E/config
> - boot qemu-arm with below command
>    -  /usr/bin/qemu-system-aarch64 -cpu host,aarch64=off -machine
> virt-2.10,accel=kvm -nographic -net
> nic,model=virtio,macaddr=BA:DD:AD:CC:09:04 -net tap -m 2048 -monitor
> none -kernel zImage --append "console=ttyAMA0 root=/dev/vda rw" -hda
> rpb-console-image-lkft-am57xx-evm-20201022181203-3085.rootfs.ext4 -m
> 4096 -smp 2 -nographic
>
> - After the mount rootfs - the systemd gets stuck
>
> >
> > I'll try to poke at this more, but LMK if you have any ideas. The
> > commit looks "obviously correct" :S
>
> May I request to investigate this on arm architecture.
> The qemu_arm boot failed link,
> https://lkft.validation.linaro.org/scheduler/job/2565371#L540
>
> The qemu_arm boot pass after the reverting this patch,
>     commit 38ebcf5096a86762b82262e96b2c8b170fe79040
>     scm: optimize put_cmsg()

Well, as already reported, this patch had an obvious typo.

Fixed later by "scm: fix a typo in put_cmsg()"

Can you trace put_cmsg() and check that systemd passes an aligned
control buffer ?

Kernel was indeed able to handle arbitrary alignment, but why the
application would
force slow copyout() (alignment mismatch between source/destination buffers)
is quite strange.



>
> on the latest linux next tags i have to revert two commits.
>    "scm: fix a typo in put_cmsg()"
>    "scm: optimize put_cmsg()"
>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>
> --
> Linaro LKFT
> https://lkft.linaro.org