Message-ID: <665734cce5a6d_31b26729414@willemb.c.googlers.com.notmuch>
Date: Wed, 29 May 2024 09:59:40 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: zijianzhang@...edance.com, 
 netdev@...r.kernel.org
Cc: edumazet@...gle.com, 
 willemdebruijn.kernel@...il.com, 
 cong.wang@...edance.com, 
 xiaochun.lu@...edance.com, 
 Zijian Zhang <zijianzhang@...edance.com>
Subject: Re: [PATCH net-next v4 0/3] net: A lightweight zero-copy notification

zijianzhang@ wrote:
> From: Zijian Zhang <zijianzhang@...edance.com>
> 
> Original title is "net: socket sendmsg MSG_ZEROCOPY_UARG". 
> 
> Original notification mechanism needs poll + recvmmsg which is not
> easy for applcations to accommodate. And, it also incurs unignorable
> overhead including extra system calls and usage of socket optmem.
> 
> While making maximum reuse of the existing MSG_ZEROCOPY related code,
> this patch set introduces a new zerocopy socket notification mechanism.
> Users of sendmsg pass a control message as a placeholder for the incoming
> notifications. Upon returning, kernel embeds notifications directly into
> user arguments passed in. By doing so, we can significantly reduce the
> complexity and overhead for managing notifications. In an ideal pattern,
> the user will keep calling sendmsg with SCM_ZC_NOTIFICATION msg_control,
> and the notification will be delivered as soon as possible.
> 
> Users need to pass in a user space address pointing to an array of struct
> zc_info_elem, and the cmsg_len should be the memory size of the array
> instead of the size of the pointer itself.
> 
> As Willem commented,
> 
> > The main design issue with this series is this indirection, rather
> > than passing the array of notifications as cmsg.
> 
> > This trick circumvents having to deal with compat issues and having to
> > figure out copy_to_user in ____sys_sendmsg (as msg_control is an
> > in-kernel copy).
> 
> > This is quite hacky, from an API design PoV.
> 
> > As is passing a pointer, but expecting msg_controllen to hold the
> > length not of the pointer, but of the pointed to user buffer.
> 
> > I had also hoped for more significant savings. Especially with the
> > higher syscall overhead due to meltdown and spectre mitigations vs
> > when MSG_ZEROCOPY was introduced and I last tried this optimization.

Thanks for quoting this.

This revision does not address either of these concerns, right?

> 
> Changelog:
>   v1 -> v2:
>     - Reuse errormsg queue in the new notification mechanism,
>       users can actually use these two mechanisms in hybrid way
>       if they want to do so.
>     - Update case SCM_ZC_NOTIFICATION in __sock_cmsg_send
>       1. Regardless of 32-bit, 64-bit program, we will always handle
>       u64 type user address.
>       2. The size of data to copy_to_user is precisely calculated
>       in case of kernel stack leak.
>     - fix (kbuild-bot)
>       1. Add SCM_ZC_NOTIFICATION to arch-specific header files.
>       2. header file types.h in include/uapi/linux/socket.h
> 
>   v2 -> v3:
>     - 1. Users can now pass in the address of the zc_info_elem directly
>       with appropriate cmsg_len instead of the ugly user interface. Plus,
>       the handler is now compatible with MSG_CMSG_COMPAT and 32-bit
>       pointer.
>     - 2. Suggested by Willem, another strategy of getting zc info is
>       briefly taking the lock of sk_error_queue and move to a private
>       list, like net_rx_action. I thought sk_error_queue is protected by
>       sock_lock, so that it's impossible for the handling of zc info and
>       users recvmsg from the sk_error_queue at the same time.
>       However, sk_error_queue is protected by its own lock. I am afraid
>       that during the time it is handling the private list, users may
>       fail to get other error messages in the queue via recvmsg. Thus,
>       I don't implement the splice logic in this version. Any comments?
> 
>   v3 -> v4:
>     - 1. Change SOCK_ZC_INFO_MAX to 64 to avoid large stack frame size.
>     - 2. Fix minor typos.
>     - 3. Change cfg_zerocopy from int to enum in msg_zerocopy.c
> 
> * Performance
> 
> I extend the selftests/msg_zerocopy.c to accommodate the new mechanism,
> test result is as follows,
> 
> cfg_notification_limit = 1, in this case the original method approximately
> aligns with the semantics of new one. In this case, the new flag has
> around 13% cpu savings in TCP and 18% cpu savings in UDP.
> 
> +---------------------+---------+---------+---------+---------+
> | Test Type / Protocol| TCP v4  | TCP v6  | UDP v4  | UDP v6  |
> +---------------------+---------+---------+---------+---------+
> | ZCopy (MB)          | 5147    | 4885    | 7489    | 7854    |
> +---------------------+---------+---------+---------+---------+
> | New ZCopy (MB)      | 5859    | 5505    | 9053    | 9236    |
> +---------------------+---------+---------+---------+---------+
> | New ZCopy / ZCopy   | 113.83% | 112.69% | 120.88% | 117.59% |
> +---------------------+---------+---------+---------+---------+
> 
> 
> cfg_notification_limit = 32, the new mechanism performs 8% better in TCP.
> For UDP, no obvious performance gain is observed and sometimes may lead
> to degradation. Thus, if users don't need to retrieve the notification
> ASAP in UDP, the original mechanism is preferred.
> 
> +---------------------+---------+---------+---------+---------+
> | Test Type / Protocol| TCP v4  | TCP v6  | UDP v4  | UDP v6  |
> +---------------------+---------+---------+---------+---------+
> | ZCopy (MB)          | 6272    | 6138    | 12138   | 10055   |
> +---------------------+---------+---------+---------+---------+
> | New ZCopy (MB)      | 6774    | 6620    | 11504   | 10355   |
> +---------------------+---------+---------+---------+---------+
> | New ZCopy / ZCopy   | 108.00% | 107.85% | 94.78%  | 102.98% |
> +---------------------+---------+---------+---------+---------+
> 
> Zijian Zhang (3):
>   selftests: fix OOM problem in msg_zerocopy selftest
>   sock: add MSG_ZEROCOPY notification mechanism based on msg_control
>   selftests: add MSG_ZEROCOPY msg_control notification test
> 
>  arch/alpha/include/uapi/asm/socket.h        |   2 +
>  arch/mips/include/uapi/asm/socket.h         |   2 +
>  arch/parisc/include/uapi/asm/socket.h       |   2 +
>  arch/sparc/include/uapi/asm/socket.h        |   2 +
>  include/uapi/asm-generic/socket.h           |   2 +
>  include/uapi/linux/socket.h                 |  10 ++
>  net/core/sock.c                             |  68 ++++++++++++
>  tools/testing/selftests/net/msg_zerocopy.c  | 114 ++++++++++++++++++--
>  tools/testing/selftests/net/msg_zerocopy.sh |   1 +
>  9 files changed, 196 insertions(+), 7 deletions(-)
> 
> -- 
> 2.20.1
> 

Powered by blists - more mailing lists