[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240601103701.GD491852@kernel.org>
Date: Sat, 1 Jun 2024 11:37:01 +0100
From: Simon Horman <horms@...nel.org>
To: zijianzhang@...edance.com
Cc: netdev@...r.kernel.org, edumazet@...gle.com,
willemdebruijn.kernel@...il.com, cong.wang@...edance.com,
xiaochun.lu@...edance.com
Subject: Re: [PATCH net-next v4 2/3] sock: add MSG_ZEROCOPY notification
mechanism based on msg_control
On Tue, May 28, 2024 at 09:21:02PM +0000, zijianzhang@...edance.com 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 SCM_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>
...
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 521e6373d4f7..21239469d75c 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2847,6 +2847,74 @@ int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
> case SCM_RIGHTS:
> case SCM_CREDENTIALS:
> break;
> + case SCM_ZC_NOTIFICATION: {
> + int ret, i = 0;
> + int cmsg_data_len, zc_info_elem_num;
> + void __user *usr_addr;
> + struct zc_info_elem zc_info_kern[SOCK_ZC_INFO_MAX];
> + unsigned long flags;
> + struct sk_buff_head *q, local_q;
> + struct sk_buff *skb, *tmp;
> + struct sock_exterr_skb *serr;
Hi Zijian Zhang, Xiaochun Lu, all,
When compiling on ARM (32bit) with multi_v7_defconfig using clang-18
I see the following warning:
.../sock.c:2808:5: warning: stack frame size (1664) exceeds limit (1024) in '__sock_cmsg_send' [-Wframe-larger-than]
2808 | int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
I expect this is mostly explained by the addition of zc_info_kern above.
> +
> + 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_elem))
> + return -EINVAL;
> +
> + zc_info_elem_num = cmsg_data_len / sizeof(struct zc_info_elem);
> + if (!zc_info_elem_num || zc_info_elem_num > SOCK_ZC_INFO_MAX)
> + return -EINVAL;
> +
> + if (in_compat_syscall())
> + usr_addr = compat_ptr(*(compat_uptr_t *)CMSG_DATA(cmsg));
> + else
> + usr_addr = (void __user *)*(void **)CMSG_DATA(cmsg);
> + if (!access_ok(usr_addr, cmsg_data_len))
> + return -EFAULT;
> +
> + 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_elem_num) {
> + 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[i].hi = serr->ee.ee_data;
> + zc_info_kern[i].lo = serr->ee.ee_info;
> + zc_info_kern[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);
> +
> + ret = copy_to_user(usr_addr,
> + zc_info_kern,
> + i * sizeof(struct zc_info_elem));
> +
> + if (unlikely(ret)) {
> + spin_lock_irqsave(&q->lock, flags);
> + skb_queue_reverse_walk_safe(&local_q, skb, tmp) {
> + __skb_unlink(skb, &local_q);
> + __skb_queue_head(q, skb);
> + }
> + spin_unlock_irqrestore(&q->lock, flags);
> + return -EFAULT;
> + }
> +
> + while ((skb = __skb_dequeue(&local_q)))
> + consume_skb(skb);
> + break;
> + }
> default:
> return -EINVAL;
> }
> --
> 2.20.1
>
>
Powered by blists - more mailing lists