Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a2527c63-1b74-fe10-a959-097ec7f68135@gmail.com>
Date:   Mon, 11 Jul 2022 13:20:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH net-next v4 06/27] net: Allow custom iter handler in
 msghdr
Content-Language: en-US
To:     io-uring@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Jonathan Lemon <jonathan.lemon@gmail.com>,
        Willem de Bruijn <willemb@google.com>,
        Jens Axboe <axboe@kernel.dk>, David Ahern <dsahern@kernel.org>,
        kernel-team@fb.com
References: <cover.1657194434.git.asml.silence@gmail.com>
 <968c344a59315ec5d0095584a95bb7dd5a3ac617.1657194434.git.asml.silence@gmail.com>
From:   Pavel Begunkov <asml.silence@gmail.com>
In-Reply-To: <968c344a59315ec5d0095584a95bb7dd5a3ac617.1657194434.git.asml.silence@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/7/22 12:49, Pavel Begunkov wrote:
> From: David Ahern <dsahern@kernel.org>
> 
> Add support for custom iov_iter handling to msghdr. The idea is that
> in-kernel subsystems want control over how an SG is split.
> 
> Signed-off-by: David Ahern <dsahern@kernel.org>
> [pavel: move callback into msghdr]
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>   include/linux/skbuff.h |  7 ++++---
>   include/linux/socket.h |  4 ++++
>   net/core/datagram.c    | 14 ++++++++++----
>   net/core/skbuff.c      |  2 +-
>   4 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 8e12b3b9ad6c..a8a2dd4cfdfd 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1776,13 +1776,14 @@ void msg_zerocopy_put_abort(struct ubuf_info *uarg, bool have_uref);
>   void msg_zerocopy_callback(struct sk_buff *skb, struct ubuf_info *uarg,
>   			   bool success);
>   
> -int __zerocopy_sg_from_iter(struct sock *sk, struct sk_buff *skb,
> -			    struct iov_iter *from, size_t length);
> +int __zerocopy_sg_from_iter(struct msghdr *msg, struct sock *sk,
> +			    struct sk_buff *skb, struct iov_iter *from,
> +			    size_t length);
>   
>   static inline int skb_zerocopy_iter_dgram(struct sk_buff *skb,
>   					  struct msghdr *msg, int len)
>   {
> -	return __zerocopy_sg_from_iter(skb->sk, skb, &msg->msg_iter, len);
> +	return __zerocopy_sg_from_iter(msg, skb->sk, skb, &msg->msg_iter, len);
>   }
>   
>   int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb,
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index 7bac9fc1cee0..3c11ef18a9cf 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -14,6 +14,8 @@ struct file;
>   struct pid;
>   struct cred;
>   struct socket;
> +struct sock;
> +struct sk_buff;
>   
>   #define __sockaddr_check_size(size)	\
>   	BUILD_BUG_ON(((size) > sizeof(struct __kernel_sockaddr_storage)))
> @@ -70,6 +72,8 @@ struct msghdr {
>   	__kernel_size_t	msg_controllen;	/* ancillary data buffer length */
>   	struct kiocb	*msg_iocb;	/* ptr to iocb for async requests */
>   	struct ubuf_info *msg_ubuf;
> +	int (*sg_from_iter)(struct sock *sk, struct sk_buff *skb,
> +			    struct iov_iter *from, size_t length);
>   };
>   
>   struct user_msghdr {
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index 50f4faeea76c..b3c05efd659f 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -613,10 +613,16 @@ int skb_copy_datagram_from_iter(struct sk_buff *skb, int offset,
>   }
>   EXPORT_SYMBOL(skb_copy_datagram_from_iter);
>   
> -int __zerocopy_sg_from_iter(struct sock *sk, struct sk_buff *skb,
> -			    struct iov_iter *from, size_t length)
> +int __zerocopy_sg_from_iter(struct msghdr *msg, struct sock *sk,
> +			    struct sk_buff *skb, struct iov_iter *from,
> +			    size_t length)
>   {
> -	int frag = skb_shinfo(skb)->nr_frags;
> +	int frag;
> +
> +	if (msg && msg->sg_from_iter && msg->msg_ubuf == skb_zcopy(skb))

I'm killing "msg->msg_ubuf == skb_zcopy(skb)", which I added with an
intention to make it less fragile, but it disables the optimisation for
TCP because skb_zerocopy_iter_stream() assigns ubuf to the skb only after
calling __zerocopy_sg_from_iter().


> +		return msg->sg_from_iter(sk, skb, from, length);
> +
> +	frag = skb_shinfo(skb)->nr_frags;
>   
>   	while (length && iov_iter_count(from)) {
>   		struct page *pages[MAX_SKB_FRAGS];
> @@ -702,7 +708,7 @@ int zerocopy_sg_from_iter(struct sk_buff *skb, struct iov_iter *from)
>   	if (skb_copy_datagram_from_iter(skb, 0, from, copy))
>   		return -EFAULT;
>   
> -	return __zerocopy_sg_from_iter(NULL, skb, from, ~0U);
> +	return __zerocopy_sg_from_iter(NULL, NULL, skb, from, ~0U);
>   }
>   EXPORT_SYMBOL(zerocopy_sg_from_iter);
>   
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index fc22b3d32052..f5a3ebbc1f7e 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1358,7 +1358,7 @@ int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb,
>   	if (orig_uarg && uarg != orig_uarg)
>   		return -EEXIST;
>   
> -	err = __zerocopy_sg_from_iter(sk, skb, &msg->msg_iter, len);
> +	err = __zerocopy_sg_from_iter(msg, sk, skb, &msg->msg_iter, len);
>   	if (err == -EFAULT || (err == -EMSGSIZE && skb->len == orig_len)) {
>   		struct sock *save_sk = skb->sk;
>   

-- 
Pavel Begunkov