Re: [PATCH net-next v7 2/3] sock: add MSG_ZEROCOPY notification mechanism based on msg_control

[Mina Almasry <almasrymina@google.com>]

On Mon, Jul 08, 2024 at 09:04:04PM +0000, zijianzhang@bytedance.com wrote:
> From: Zijian Zhang <zijianzhang@bytedance.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. We try to mitigate this overhead
> with a new notification mechanism based on msg_control. Leveraging the
> general framework to copy cmsgs to the user space, we copy zerocopy
> notifications to the user upon returning of sendmsgs.
>
> Signed-off-by: Zijian Zhang <zijianzhang@bytedance.com>
> Signed-off-by: Xiaochun Lu <xiaochun.lu@bytedance.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/linux/socket.h                |  2 +-
>  include/uapi/asm-generic/socket.h     |  2 ++
>  include/uapi/linux/socket.h           | 13 ++++++++
>  net/core/sock.c                       | 46 +++++++++++++++++++++++++++
>  8 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
> index e94f621903fe..7c32d9dbe47f 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 SCM_ZC_NOTIFICATION	78
> +
>  #if !defined(__KERNEL__)
>
>  #if __BITS_PER_LONG == 64
> diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h
> index 60ebaed28a4c..3f7fade998cb 100644
> --- a/arch/mips/include/uapi/asm/socket.h
> +++ b/arch/mips/include/uapi/asm/socket.h
> @@ -151,6 +151,8 @@
>  #define SO_PASSPIDFD		76
>  #define SO_PEERPIDFD		77
>
> +#define SCM_ZC_NOTIFICATION	78
> +
>  #if !defined(__KERNEL__)
>
>  #if __BITS_PER_LONG == 64
> diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h
> index be264c2b1a11..77f5bee0fdc9 100644
> --- a/arch/parisc/include/uapi/asm/socket.h
> +++ b/arch/parisc/include/uapi/asm/socket.h
> @@ -132,6 +132,8 @@
>  #define SO_PASSPIDFD		0x404A
>  #define SO_PEERPIDFD		0x404B
>
> +#define SCM_ZC_NOTIFICATION	0x404C
> +
>  #if !defined(__KERNEL__)
>
>  #if __BITS_PER_LONG == 64
> diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h
> index 682da3714686..eb44fc515b45 100644
> --- a/arch/sparc/include/uapi/asm/socket.h
> +++ b/arch/sparc/include/uapi/asm/socket.h
> @@ -133,6 +133,8 @@
>  #define SO_PASSPIDFD             0x0055
>  #define SO_PEERPIDFD             0x0056
>
> +#define SCM_ZC_NOTIFICATION      0x0057
> +
>  #if !defined(__KERNEL__)
>
>
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index 75461812a7a3..6f1b791e2de8 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -171,7 +171,7 @@ static inline struct cmsghdr * cmsg_nxthdr (struct msghdr *__msg, struct cmsghdr
>
>  static inline bool cmsg_copy_to_user(struct cmsghdr *__cmsg)
>  {
> -	return 0;
> +	return __cmsg->cmsg_type == SCM_ZC_NOTIFICATION;
>  }
>
>  static inline size_t msg_data_left(struct msghdr *msg)
> diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
> index 8ce8a39a1e5f..02e9159c7944 100644
> --- a/include/uapi/asm-generic/socket.h
> +++ b/include/uapi/asm-generic/socket.h
> @@ -135,6 +135,8 @@
>  #define SO_PASSPIDFD		76
>  #define SO_PEERPIDFD		77
>
> +#define SCM_ZC_NOTIFICATION	78
> +
>  #if !defined(__KERNEL__)
>
>  #if __BITS_PER_LONG == 64 || (defined(__x86_64__) && defined(__ILP32__))
> diff --git a/include/uapi/linux/socket.h b/include/uapi/linux/socket.h
> index d3fcd3b5ec53..ab361f30f3a6 100644
> --- a/include/uapi/linux/socket.h
> +++ b/include/uapi/linux/socket.h
> @@ -2,6 +2,8 @@
>  #ifndef _UAPI_LINUX_SOCKET_H
>  #define _UAPI_LINUX_SOCKET_H
>
> +#include <linux/types.h>
> +
>  /*
>   * Desired design of maximum size and alignment (see RFC2553)
>   */
> @@ -35,4 +37,15 @@ struct __kernel_sockaddr_storage {
>  #define SOCK_TXREHASH_DISABLED	0
>  #define SOCK_TXREHASH_ENABLED	1
>
> +struct zc_info_elem {
> +	__u32 lo;
> +	__u32 hi;
> +	__u8 zerocopy;

Some docs please on what each of these are, if possible. Sorry if the repeated
requests are annoying.

In particular I'm a bit confused why the zerocopy field is there. Looking at
the code, is this always set to 1?

> +};
> +
> +struct zc_info {
> +	__u32 size;
> +	struct zc_info_elem arr[];
> +};
> +
>  #endif /* _UAPI_LINUX_SOCKET_H */
> diff --git a/net/core/sock.c b/net/core/sock.c
> index efb30668dac3..e0b5162233d3 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2863,6 +2863,52 @@ int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
>  	case SCM_RIGHTS:
>  	case SCM_CREDENTIALS:
>  		break;
> +	case SCM_ZC_NOTIFICATION: {
> +		struct zc_info *zc_info = CMSG_DATA(cmsg);
> +		struct zc_info_elem *zc_info_arr;
> +		struct sock_exterr_skb *serr;
> +		int cmsg_data_len, i = 0;
> +		struct sk_buff_head *q;
> +		unsigned long flags;
> +		struct sk_buff *skb;
> +		u32 zc_info_size;
> +
> +		if (!sock_flag(sk, SOCK_ZEROCOPY) || sk->sk_family == PF_RDS)
> +			return -EINVAL;
> +
> +		cmsg_data_len = cmsg->cmsg_len - sizeof(struct cmsghdr);
> +		if (cmsg_data_len < sizeof(struct zc_info))
> +			return -EINVAL;
> +
> +		zc_info_size = zc_info->size;
> +		zc_info_arr = zc_info->arr;

Annoying nit: To be honest zc_info->size isn't much longer to type than
zc_info_size, so I would have not added local variables.

> +		if (cmsg_data_len != sizeof(struct zc_info) +
> +				     zc_info_size * sizeof(struct zc_info_elem))
> +			return -EINVAL;
> +
> +		q = &sk->sk_error_queue;
> +		spin_lock_irqsave(&q->lock, flags);
> +		skb = skb_peek(q);
> +		while (skb && i < zc_info_size) {
> +			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_arr[i].hi = serr->ee.ee_data;
> +				zc_info_arr[i].lo = serr->ee.ee_info;
> +				zc_info_arr[i].zerocopy = !(serr->ee.ee_code
> +							  & SO_EE_CODE_ZEROCOPY_COPIED);
> +				__skb_unlink(skb, q);
> +				consume_skb(skb);
> +				i++;
> +			}
> +			skb = skb_next;
> +		}
> +		spin_unlock_irqrestore(&q->lock, flags);

I wonder if you should drop the spin lock in the middle of this loop somehow,
otherwise you may end up spinning for a very long time while the spinlock held
and irq disabled.

IIRC zc_info_size is user input, right? Maybe you should limit zc_info_size to
16 entries or something. So the user doesn't end up passing 100000 as
   zc_info_size and making the kernel loop for a long time here.

> +		zc_info->size = i;
> +		break;
> +	}
>  	default:
>  		return -EINVAL;
>  	}
> --
> 2.20.1
>