Re: [Patch bpf-next v7 00/13] sockmap: introduce BPF_SK_SKB_VERDICT and support UDP

From: Cong Wang <xiyou.wangcong@gmail.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>,
	duanxiongchun@bytedance.com,
	Dongdong Wang <wangdongdong.6@bytedance.com>,
	Jiang Wang <jiang.wang@bytedance.com>,
	Cong Wang <cong.wang@bytedance.com>
Subject: Re: [Patch bpf-next v7 00/13] sockmap: introduce BPF_SK_SKB_VERDICT and support UDP
Date: Mon, 29 Mar 2021 09:57:38 -0700	[thread overview]
Message-ID: <CAM_iQpV3d6_RMcuwF6BmR635og0=FUy7siN-0MNchR9_sXz2cg@mail.gmail.com> (raw)
In-Reply-To: <6061ec2bc63e0_3673a20848@john-XPS-13-9370.notmuch>

On Mon, Mar 29, 2021 at 8:03 AM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Alexei Starovoitov wrote:
> > On Sun, Mar 28, 2021 at 01:20:00PM -0700, Cong Wang wrote:
> > > From: Cong Wang <cong.wang@bytedance.com>
> > >
> > > We have thousands of services connected to a daemon on every host
> > > via AF_UNIX dgram sockets, after they are moved into VM, we have to
> > > add a proxy to forward these communications from VM to host, because
> > > rewriting thousands of them is not practical. This proxy uses an
> > > AF_UNIX socket connected to services and a UDP socket to connect to
> > > the host. It is inefficient because data is copied between kernel
> > > space and user space twice, and we can not use splice() which only
> > > supports TCP. Therefore, we want to use sockmap to do the splicing
> > > without going to user-space at all (after the initial setup).
> > >
> > > Currently sockmap only fully supports TCP, UDP is partially supported
> > > as it is only allowed to add into sockmap. This patchset, as the second
> > > part of the original large patchset, extends sockmap with:
> > > 1) cross-protocol support with BPF_SK_SKB_VERDICT; 2) full UDP support.
> > >
> > > On the high level, ->read_sock() is required for each protocol to support
> > > sockmap redirection, and in order to do sock proto update, a new ops
> > > ->psock_update_sk_prot() is introduced, which is also required. And the
> > > BPF ->recvmsg() is also needed to replace the original ->recvmsg() to
> > > retrieve skmsg. To make life easier, we have to get rid of lock_sock()
> > > in sk_psock_handle_skb(), otherwise we would have to implement
> > > ->sendmsg_locked() on top of ->sendmsg(), which is ugly.
> > >
> > > Please see each patch for more details.
> > >
> > > To see the big picture, the original patchset is available here:
> > > https://github.com/congwang/linux/tree/sockmap
> > > this patchset is also available:
> > > https://github.com/congwang/linux/tree/sockmap2
> > >
> > > ---
> > > v7: use work_mutex to protect psock->work
> > >     return err in udp_read_sock()
> > >     add patch 6/13
> > >     clean up test case
> >
> > The feature looks great to me.
> > I think the selftest is a bit light in terms of coverage, but it's acceptable.
>
> +1

Well, the first half of this patchset still focuses on the existing code, which
is already covered by existing test cases. The second half adds
BPF_SK_SKB_VERDICT and UDP support, which are already covered by
my new test case. And apparently UDP will never support other sockmap
programs like TCP, for example, BPF_SK_SKB_STREAM_PARSER, hence
it of course has much less test cases than TCP. Cross-protocol test case
will be added in the next patchset when AF_UNIX comes in.

If I miss anything, please be specific. Just saying the test case is light does
not help me to understand what I need to add.

Thanks.