Re: [Patch bpf-next v3 4/5] skmsg: use skb ext instead of TCP_SKB_CB

[John Fastabend <john.fastabend@gmail.com>]

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.

> 
> >
> > The use in tcf_classify_ingress is a miss case so not the common path. If
> > it is/was in the common path I would suggest we rip it out.
> >
> 
> Excellent point, what about nf_bridge_unshare()? It is a common path
> for bridge netfilter, which is also probably why skb ext was introduced
> (IIRC). secpath_set() seems on a common path for XFRM too.

Yeah not nice, but we don't use nf_bridge so doesn't bother me.

> 
> Are you suggesting to remove them all? ;)

From the hotpath where I care about perfromance yes. 

> 
> Thanks.