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

Cong Wang wrote:

On Mon, Feb 15, 2021 at 3:57 PM John Fastabend [off-list ref] wrote:

quoted

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.

quoted

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.