Re: [PATCH net 1/1] net: openvswitch: Fix ct_state nat flags for conns arriving from tc

From: Paul Blakey <paulb@nvidia.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>, <dev@openvswitch.org>,
	<netdev@vger.kernel.org>, Cong Wang <xiyou.wangcong@gmail.com>,
	"Pravin B Shelar" <pshelar@ovn.org>, <davem@davemloft.net>,
	Jiri Pirko <jiri@nvidia.com>, Jakub Kicinski <kuba@kernel.org>,
	Saeed Mahameed <saeedm@nvidia.com>, Oz Shlomo <ozsh@nvidia.com>,
	Vlad Buslov <vladbu@nvidia.com>, Roi Dayan <roid@nvidia.com>,
	<john.fastabend@gmail.com>
Subject: Re: [PATCH net 1/1] net: openvswitch: Fix ct_state nat flags for conns arriving from tc
Date: Wed, 5 Jan 2022 18:18:06 +0200	[thread overview]
Message-ID: <5599f6-e02e-9ad5-3585-db252e946f43@nvidia.com> (raw)
In-Reply-To: <72368c25-83c5-565f-0512-ca5d58315685@iogearbox.net>

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

On Wed, 5 Jan 2022, Daniel Borkmann wrote:

> On 1/5/22 3:57 PM, Jamal Hadi Salim wrote:
> > On 2022-01-04 03:28, Paul Blakey wrote:
> > [..]
> >> --- a/include/linux/skbuff.h
> >> +++ b/include/linux/skbuff.h
> >> @@ -287,7 +287,9 @@ struct tc_skb_ext {
> >>       __u32 chain;
> >>       __u16 mru;
> >>       __u16 zone;
> >> -    bool post_ct;
> >> +    bool post_ct:1;
> >> +    bool post_ct_snat:1;
> >> +    bool post_ct_dnat:1;
> >>   };
> > 
> > is skb_ext intended only for ovs? If yes, why does it belong
> > in the core code? Ex: Looking at tcf_classify() which is such
> > a core function in the fast path any packet going via tc, it
> > is now encumbered with with checking presence of skb_ext.
> > I know passing around metadata is a paramount requirement
> > for programmability but this is getting messier with speacial
> > use cases for ovs and/or offload...
> 
> Full ack on the bloat for corner cases like ovs offload, especially
> given distros just enable most stuff anyway and therefore no light
> fast path as with !CONFIG_NET_TC_SKB_EXT. :(
> 
> Could this somehow be hidden behind static key or such if offloads
> are not used, so we can shrink it back to just calling into plain
> __tcf_classify() for sw-only use cases (like BPF)?
> 
> 

It is used for both tc -> ovs and driver -> tc path.

I think I can do what you suggest adn will work on something like
that,  but this specific patch  doesn't really change the ext 
allocation/derefences count (and probably  not the size as well).
So can  we take  this (not yet posted v2 after fixing what already 
mentioned) and I'll do a patch of what you suggest in net-next?