Cong Wang wrote: > On Mon, Feb 15, 2021 at 3:57 PM John Fastabend <john.fastabend@xxxxxxxxx> 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.
