| Message ID | 20210415173753.3404237-1-eric.dumazet@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] scm: optimize put_cmsg() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 3 maintainers not CCed: christian.brauner@ubuntu.com keescook@chromium.org sargun@sargun.me |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 35 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
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 >
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
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,
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,
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 !
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
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
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);