Message-ID: <6625349824651_1dff99294db@willemb.c.googlers.com.notmuch>
Date: Sun, 21 Apr 2024 11:45:28 -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, 
 davem@...emloft.net, 
 kuba@...nel.org, 
 cong.wang@...edance.com, 
 xiaochun.lu@...edance.com, 
 Zijian Zhang <zijianzhang@...edance.com>
Subject: Re: [PATCH net-next v2 2/3] sock: add MSG_ZEROCOPY notification
 mechanism based on msg_control

zijianzhang@ wrote:
> From: Zijian Zhang <zijianzhang@...edance.com>
> 
> The MSG_ZEROCOPY flag enables copy avoidance for socket send calls.
> However, zerocopy is not a free lunch. Apart from the management of user
> pages, the combination of poll + recvmsg to receive notifications incurs
> unignorable overhead in the applications. The overhead of such sometimes
> might be more than the CPU savings from zerocopy. We try to solve this
> problem with a new notification mechanism based on msgcontrol.
> This new mechanism aims to reduce the overhead associated with receiving
> notifications by embedding them directly into user arguments passed with
> each sendmsg control message. 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 SO_ZC_NOTIFICATION
> msg_control, and the notification will be delivered as soon as possible.
> 
> Signed-off-by: Zijian Zhang <zijianzhang@...edance.com>
> Signed-off-by: Xiaochun Lu <xiaochun.lu@...edance.com>
> ---
>  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           | 16 ++++++
>  net/core/sock.c                       | 70 +++++++++++++++++++++++++++
>  7 files changed, 96 insertions(+)
> 
> diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
> index e94f621903fe..b24622a9cd47 100644
> --- a/arch/alpha/include/uapi/asm/socket.h
> +++ b/arch/alpha/include/uapi/asm/socket.h
> @@ -140,6 +140,8 @@
>  #define SO_PASSPIDFD		76
>  #define SO_PEERPIDFD		77
>  
> +#define SO_ZC_NOTIFICATION 78
> +

SCM_ for cmsgs

>  /*
>   * Desired design of maximum size and alignment (see RFC2553)
>   */
> @@ -35,4 +37,18 @@ struct __kernel_sockaddr_storage {
>  #define SOCK_TXREHASH_DISABLED	0
>  #define SOCK_TXREHASH_ENABLED	1
>  
> +#define SOCK_ZC_INFO_MAX 256
> +
> +struct zc_info_elem {
> +	__u32 lo;
> +	__u32 hi;
> +	__u8 zerocopy;
> +};
> +
> +struct zc_info_usr {
> +	__u64 usr_addr;
> +	unsigned int length;
> +	struct zc_info_elem info[];
> +};
> +

Don't pass a pointer to user memory, just have msg_control point to an
array of zc_info_elem.

>  #endif /* _UAPI_LINUX_SOCKET_H */
> diff --git a/net/core/sock.c b/net/core/sock.c
> index fe9195186c13..13f06480f2d8 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2809,6 +2809,13 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>  		     struct sockcm_cookie *sockc)
>  {
>  	u32 tsflags;
> +	int ret, zc_info_size, i = 0;
> +	unsigned long flags;
> +	struct sk_buff_head *q, local_q;
> +	struct sk_buff *skb, *tmp;
> +	struct sock_exterr_skb *serr;
> +	struct zc_info_usr *zc_info_usr_p, *zc_info_kern_p;
> +	void __user	*usr_addr;

Please wrap the case in parentheses and define variables in that scope
(Since there are so many variables for this case only.)

>  
>  	switch (cmsg->cmsg_type) {
>  	case SO_MARK:
> @@ -2842,6 +2849,69 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>  	case SCM_RIGHTS:
>  	case SCM_CREDENTIALS:
>  		break;
> +	case SO_ZC_NOTIFICATION:
> +		if (!sock_flag(sk, SOCK_ZEROCOPY) || sk->sk_family == PF_RDS)
> +			return -EINVAL;
> +

Why allow PF_RDS without the sock flag set?

> +		zc_info_usr_p = (struct zc_info_usr *)CMSG_DATA(cmsg);
> +		if (zc_info_usr_p->length <= 0 || zc_info_usr_p->length > SOCK_ZC_INFO_MAX)
> +			return -EINVAL;
> +
> +		zc_info_size = struct_size(zc_info_usr_p, info, zc_info_usr_p->length);
> +		if (cmsg->cmsg_len != CMSG_LEN(zc_info_size))
> +			return -EINVAL;

By passing a straightforward array, the array len can be inferred from
cmsg_len, simplifying all these checks.

See for instance how SO_DEVMEM_DONTNEED returns an array of tokens to
the kernel.

> +
> +		usr_addr = (void *)(uintptr_t)(zc_info_usr_p->usr_addr);
> +		if (!access_ok(usr_addr, zc_info_size))
> +			return -EFAULT;
> +
> +		zc_info_kern_p = kmalloc(zc_info_size, GFP_KERNEL);
> +		if (!zc_info_kern_p)
> +			return -ENOMEM;
> +
> +		q = &sk->sk_error_queue;
> +		skb_queue_head_init(&local_q);
> +		spin_lock_irqsave(&q->lock, flags);
> +		skb = skb_peek(q);
> +		while (skb && i < zc_info_usr_p->length) {
> +			struct sk_buff *skb_next = skb_peek_next(skb, q);
> +
> +			serr = SKB_EXT_ERR(skb);
> +			if (serr->ee.ee_errno == 0 &&
> +			    serr->ee.ee_origin == SO_EE_ORIGIN_ZEROCOPY) {
> +				zc_info_kern_p->info[i].hi = serr->ee.ee_data;
> +				zc_info_kern_p->info[i].lo = serr->ee.ee_info;
> +				zc_info_kern_p->info[i].zerocopy = !(serr->ee.ee_code
> +								& SO_EE_CODE_ZEROCOPY_COPIED);
> +				__skb_unlink(skb, q);
> +				__skb_queue_tail(&local_q, skb);
> +				i++;
> +			}
> +			skb = skb_next;
> +		}
> +		spin_unlock_irqrestore(&q->lock, flags);

In almost all sane cases, all outstanding notifications can be passed
to userspace.

It may be interesting to experiment with briefly taking the lock to
move to a private list. See for instance net_rx_action.

Then if userspace cannot handle all notifications, the rest have to be
spliced back. This can reorder notifications. But rare reordering is
not a correctness issue.

I would choose the more complex splice approach only if it shows
benefit, i.e., if taking the lock does contend with error enqueue
events.

> +
> +		zc_info_kern_p->usr_addr = zc_info_usr_p->usr_addr;
> +		zc_info_kern_p->length = i;
> +
> +		ret = copy_to_user(usr_addr,
> +				   zc_info_kern_p,
> +					struct_size(zc_info_kern_p, info, i));

You'll still need to support the gnarly MSG_CMSG_COMPAT version too.

Wait, is this the reason to pass a usr_addr explicitly? To get around
any compat issues?

Or even the entire issue of having to copy msg_sys->msg_control to
user if !msg_control_is_user.

I suppose this simplifies a lot in terms of development. If making the
user interface uglier.

IMHO the sane interface should be used eventually. There may also be
other uses of passing msg_control data up to userspace from sendmsg.

But this approach works for now for evaluation and discussion.

Powered by blists - more mailing lists