From: Cong Wang <xiyou.wangcong@gmail.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
bpf <bpf@vger.kernel.org>,
duanxiongchun@bytedance.com,
Dongdong Wang <wangdongdong.6@bytedance.com>,
jiang.wang@bytedance.com, Cong Wang <cong.wang@bytedance.com>,
Daniel Borkmann <daniel@iogearbox.net>,
Jakub Sitnicki <jakub@cloudflare.com>,
Lorenz Bauer <lmb@cloudflare.com>
Subject: Re: [Patch bpf-next v3 4/5] skmsg: use skb ext instead of TCP_SKB_CB
Date: Mon, 15 Feb 2021 17:04:00 -0800 [thread overview]
Message-ID: <CAM_iQpWzRpfWwZHPK=+KWbu+nLxJ=GKRHNC+97NT2DoN0qRc2A@mail.gmail.com> (raw)
In-Reply-To: <602b17b0492a8_3ed41208f2@john-XPS-13-9370.notmuch>
On Mon, Feb 15, 2021 at 4:54 PM John Fastabend <john.fastabend@gmail.com> wrote:
>
> Cong Wang wrote:
> > On Mon, Feb 15, 2021 at 3:57 PM John Fastabend <john.fastabend@gmail.com> wrote:
> > >
> > > For TCP case we can continue to use CB and not pay the price. For UDP
> > > and AF_UNIX we can do the extra alloc.
> >
> > I see your point, but specializing TCP case does not give much benefit
> > here, the skmsg code would have to check skb->protocol etc. to decide
> > whether to use TCP_SKB_CB() or skb_ext:
> >
> > if (skb->protocol == ...)
> > TCP_SKB_CB(skb) = ...;
> > else
> > ext = skb_ext_find(skb);
> >
> > which looks ugly to me. And I doubt skb->protocol alone is sufficient to
> > distinguish TCP, so we may end up having more checks above.
> >
> > So do you really want to trade code readability with an extra alloc?
>
> Above is ugly. So I look at where the patch replaces things,
>
> sk_psock_tls_strp_read(), this is TLS specific read hook so can't really
> work in generic case anyways.
>
> sk_psock_strp_read(), will you have UDP, AF_UNIX stream parsers? Do these
> even work outside TCP cases.
>
> For these ones: sk_psock_verdict_apply(), sk_psock_verdict_recv(),
> sk_psock_backlog(), can't we just do some refactoring around their
> hook points so we know the context. For example sk_psock_tls_verdict_apply
> is calling sk_psock_skb_redirect(). Why not have a sk_psock_unix_redirect()
> and a sk_psock_udp_redirect(). There are likely some optimizations we can
> deploy this way. We've already don this for tls and sk_msg types for example.
>
> Then the helpers will know their types by program type, just use the right
> variants.
>
> So not suggestiong if/else the checks so much as having per type hooks.
>
Hmm, but sk_psock_backlog() is still the only one that handles all three
above cases, right? It uses TCP_SKB_CB() too and more importantly it
is also why we can't use a per-cpu struct here (see bpf_redirect_info).
Thanks.
next prev parent reply other threads:[~2021-02-16 1:05 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-13 21:44 [Patch bpf-next v3 0/5] sock_map: clean up and refactor code for BPF_SK_SKB_VERDICT Cong Wang
2021-02-13 21:44 ` [Patch bpf-next v3 1/5] bpf: clean up sockmap related Kconfigs Cong Wang
2021-02-15 10:34 ` Lorenz Bauer
2021-02-15 18:34 ` John Fastabend
2021-02-13 21:44 ` [Patch bpf-next v3 2/5] skmsg: get rid of struct sk_psock_parser Cong Wang
2021-02-15 18:56 ` John Fastabend
2021-02-15 19:03 ` Jakub Sitnicki
2021-02-13 21:44 ` [Patch bpf-next v3 3/5] bpf: compute data_end dynamically with JIT code Cong Wang
2021-02-15 19:03 ` John Fastabend
2021-02-15 19:06 ` Jakub Sitnicki
2021-02-13 21:44 ` [Patch bpf-next v3 4/5] skmsg: use skb ext instead of TCP_SKB_CB Cong Wang
2021-02-15 19:20 ` John Fastabend
2021-02-15 22:24 ` Cong Wang
2021-02-15 23:57 ` John Fastabend
2021-02-16 0:28 ` Cong Wang
2021-02-16 0:54 ` John Fastabend
2021-02-16 1:04 ` Cong Wang [this message]
2021-02-16 1:50 ` John Fastabend
2021-02-16 4:06 ` Cong Wang
2021-02-16 8:56 ` Lorenz Bauer
2021-02-17 19:19 ` John Fastabend
2021-02-13 21:44 ` [Patch bpf-next v3 5/5] sock_map: rename skb_parser and skb_verdict Cong Wang
2021-02-15 19:09 ` John Fastabend
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAM_iQpWzRpfWwZHPK=+KWbu+nLxJ=GKRHNC+97NT2DoN0qRc2A@mail.gmail.com' \
--to=xiyou.wangcong@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=cong.wang@bytedance.com \
--cc=daniel@iogearbox.net \
--cc=duanxiongchun@bytedance.com \
--cc=jakub@cloudflare.com \
--cc=jiang.wang@bytedance.com \
--cc=john.fastabend@gmail.com \
--cc=lmb@cloudflare.com \
--cc=netdev@vger.kernel.org \
--cc=wangdongdong.6@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).