CONFIG_NET_TC_SKB_EXT

CONFIG_NET_TC_SKB_EXT

[David Miller]

As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
is really not acceptable.

It should not be enabled by default at all.

Instead the actual users should turn it on or depend upon it, which in
this case seems to be OVS.

Please fix this, thank you.

Re: CONFIG_NET_TC_SKB_EXT

[Vlad Buslov]

On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:
> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
> is really not acceptable.
>
> It should not be enabled by default at all.
>
> Instead the actual users should turn it on or depend upon it, which in
> this case seems to be OVS.
>
> Please fix this, thank you.

Hi David,

We are working on it, but Paul is OoO today. Is it okay if we send the
fix early next week?

Thanks,
Vlad

Re: CONFIG_NET_TC_SKB_EXT

[Jakub Kicinski]

On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:
> On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:
> > As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
> > is really not acceptable.
> >
> > It should not be enabled by default at all.
> >
> > Instead the actual users should turn it on or depend upon it, which in
> > this case seems to be OVS.
> >
> > Please fix this, thank you.  
> 
> Hi David,
> 
> We are working on it, but Paul is OoO today. Is it okay if we send the
> fix early next week?

Doesn't really seem like we have too many ways forward here, right?

How about this?

------>8-----------------------------------

net: hide NET_TC_SKB_EXT as a config option

Linus points out the NET_TC_SKB_EXT config option looks suspicious.
Indeed, it should really be selected to ensure correct OvS operation
if TC offload is used. Hopefully those who care about TC-only and
OvS-only performance disable the other one at compilation time.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 net/openvswitch/Kconfig |  1 +
 net/sched/Kconfig       | 13 +++----------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
index 22d7d5604b4c..bd407ea7c263 100644
--- a/net/openvswitch/Kconfig
+++ b/net/openvswitch/Kconfig
@@ -15,6 +15,7 @@ config OPENVSWITCH
 	select NET_MPLS_GSO
 	select DST_CACHE
 	select NET_NSH
+	select NET_TC_SKB_EXT if NET_CLS_ACT
 	---help---
 	  Open vSwitch is a multilayer Ethernet switch targeted at virtualized
 	  environments.  In addition to supporting a variety of features
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index b3faafeafab9..f1062ef55098 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -719,6 +719,7 @@ config NET_EMATCH_IPT
 config NET_CLS_ACT
 	bool "Actions"
 	select NET_CLS
+	select NET_TC_SKB_EXT if OPENVSWITCH
 	---help---
 	  Say Y here if you want to use traffic control actions. Actions
 	  get attached to classifiers and are invoked after a successful
@@ -964,18 +965,10 @@ config NET_IFE_SKBTCINDEX
         depends on NET_ACT_IFE
 
 config NET_TC_SKB_EXT
-	bool "TC recirculation support"
-	depends on NET_CLS_ACT
-	default y if NET_CLS_ACT
+	bool
+	depends on NET_CLS_ACT && OPENVSWITCH
 	select SKB_EXTENSIONS
 
-	help
-	  Say Y here to allow tc chain misses to continue in OvS datapath in
-	  the correct recirc_id, and hardware chain misses to continue in
-	  the correct chain in tc software datapath.
-
-	  Say N here if you won't be using tc<->ovs offload or tc chains offload.
-
 endif # NET_SCHED
 
 config NET_SCH_FIFO
-- 
2.21.0

Re: CONFIG_NET_TC_SKB_EXT

[Daniel Borkmann]

On 9/20/19 6:16 PM, Jakub Kicinski wrote:
> On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:
>> On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:
>>> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
>>> is really not acceptable.
>>>
>>> It should not be enabled by default at all.
>>>
>>> Instead the actual users should turn it on or depend upon it, which in
>>> this case seems to be OVS.
>>>
>>> Please fix this, thank you.
>>
>> Hi David,
>>
>> We are working on it, but Paul is OoO today. Is it okay if we send the
>> fix early next week?
> 
> Doesn't really seem like we have too many ways forward here, right?
> 
> How about this?
> 
> ------>8-----------------------------------
> 
> net: hide NET_TC_SKB_EXT as a config option
> 
> Linus points out the NET_TC_SKB_EXT config option looks suspicious.
> Indeed, it should really be selected to ensure correct OvS operation
> if TC offload is used. Hopefully those who care about TC-only and
> OvS-only performance disable the other one at compilation time.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>   net/openvswitch/Kconfig |  1 +
>   net/sched/Kconfig       | 13 +++----------
>   2 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> index 22d7d5604b4c..bd407ea7c263 100644
> --- a/net/openvswitch/Kconfig
> +++ b/net/openvswitch/Kconfig
> @@ -15,6 +15,7 @@ config OPENVSWITCH
>   	select NET_MPLS_GSO
>   	select DST_CACHE
>   	select NET_NSH
> +	select NET_TC_SKB_EXT if NET_CLS_ACT
>   	---help---
>   	  Open vSwitch is a multilayer Ethernet switch targeted at virtualized
>   	  environments.  In addition to supporting a variety of features
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index b3faafeafab9..f1062ef55098 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -719,6 +719,7 @@ config NET_EMATCH_IPT
>   config NET_CLS_ACT
>   	bool "Actions"
>   	select NET_CLS
> +	select NET_TC_SKB_EXT if OPENVSWITCH

But how would that make much of a difference :( Distros are still going to
enable all of this blindlessly. Given discussion in [0], could we just get
rid of this tasteless hack altogether which is for such a narrow use case
anyway?

I thought idea of stuffing things into skb extensions are only justified if
it's not enabled by default for everyone. :(

   [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u

>   	---help---
>   	  Say Y here if you want to use traffic control actions. Actions
>   	  get attached to classifiers and are invoked after a successful
> @@ -964,18 +965,10 @@ config NET_IFE_SKBTCINDEX
>           depends on NET_ACT_IFE
>   
>   config NET_TC_SKB_EXT
> -	bool "TC recirculation support"
> -	depends on NET_CLS_ACT
> -	default y if NET_CLS_ACT
> +	bool
> +	depends on NET_CLS_ACT && OPENVSWITCH
>   	select SKB_EXTENSIONS
>   
> -	help
> -	  Say Y here to allow tc chain misses to continue in OvS datapath in
> -	  the correct recirc_id, and hardware chain misses to continue in
> -	  the correct chain in tc software datapath.
> -
> -	  Say N here if you won't be using tc<->ovs offload or tc chains offload.
> -
>   endif # NET_SCHED
>   
>   config NET_SCH_FIFO
> 

Re: CONFIG_NET_TC_SKB_EXT

[Jakub Kicinski]

On Sat, 21 Sep 2019 00:15:16 +0200, Daniel Borkmann wrote:
> On 9/20/19 6:16 PM, Jakub Kicinski wrote:
> > On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:  
> >> On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:  
> >>> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
> >>> is really not acceptable.
> >>>
> >>> It should not be enabled by default at all.
> >>>
> >>> Instead the actual users should turn it on or depend upon it, which in
> >>> this case seems to be OVS.
> >>>
> >>> Please fix this, thank you.  
> >>
> >> Hi David,
> >>
> >> We are working on it, but Paul is OoO today. Is it okay if we send the
> >> fix early next week?  
> > 
> > Doesn't really seem like we have too many ways forward here, right?
> > 
> > How about this?
> >   
> > ------>8-----------------------------------  
> > 
> > net: hide NET_TC_SKB_EXT as a config option
> > 
> > Linus points out the NET_TC_SKB_EXT config option looks suspicious.
> > Indeed, it should really be selected to ensure correct OvS operation
> > if TC offload is used. Hopefully those who care about TC-only and
> > OvS-only performance disable the other one at compilation time.
> > 
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> >   net/openvswitch/Kconfig |  1 +
> >   net/sched/Kconfig       | 13 +++----------
> >   2 files changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> > index 22d7d5604b4c..bd407ea7c263 100644
> > --- a/net/openvswitch/Kconfig
> > +++ b/net/openvswitch/Kconfig
> > @@ -15,6 +15,7 @@ config OPENVSWITCH
> >   	select NET_MPLS_GSO
> >   	select DST_CACHE
> >   	select NET_NSH
> > +	select NET_TC_SKB_EXT if NET_CLS_ACT
> >   	---help---
> >   	  Open vSwitch is a multilayer Ethernet switch targeted at virtualized
> >   	  environments.  In addition to supporting a variety of features
> > diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> > index b3faafeafab9..f1062ef55098 100644
> > --- a/net/sched/Kconfig
> > +++ b/net/sched/Kconfig
> > @@ -719,6 +719,7 @@ config NET_EMATCH_IPT
> >   config NET_CLS_ACT
> >   	bool "Actions"
> >   	select NET_CLS
> > +	select NET_TC_SKB_EXT if OPENVSWITCH  
> 
> But how would that make much of a difference :( Distros are still going to
> enable all of this blindlessly. Given discussion in [0], could we just get
> rid of this tasteless hack altogether which is for such a narrow use case
> anyway?

Agreed.  I take it you're opposed to the use of skb extensions here 
in general?  Distros would have enabled NET_TC_SKB_EXT even when it 
was a config option.

> I thought idea of stuffing things into skb extensions are only justified if
> it's not enabled by default for everyone. :(
> 
>    [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u

The skb ext allocation is only done with GOTO_CHAIN, which AFAIK only
has practical use for offload.  We could perhaps add another static
branch there or move the OvS static branch out of the OvS module so
there are no linking issues?

I personally have little sympathy for this piece of code, it is perhaps
the purest form of a wobbly narrow-use construct pushed into TC for HW
offload.

Any suggestions on the way forward? :(

Re: CONFIG_NET_TC_SKB_EXT

[Daniel Borkmann]

On 9/21/19 12:56 AM, Jakub Kicinski wrote:
> On Sat, 21 Sep 2019 00:15:16 +0200, Daniel Borkmann wrote:
>> On 9/20/19 6:16 PM, Jakub Kicinski wrote:
>>> On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:
>>>> On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:
>>>>> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
>>>>> is really not acceptable.
>>>>>
>>>>> It should not be enabled by default at all.
>>>>>
>>>>> Instead the actual users should turn it on or depend upon it, which in
>>>>> this case seems to be OVS.
>>>>>
>>>>> Please fix this, thank you.
>>>>
>>>> Hi David,
>>>>
>>>> We are working on it, but Paul is OoO today. Is it okay if we send the
>>>> fix early next week?
>>>
>>> Doesn't really seem like we have too many ways forward here, right?
>>>
>>> How about this?
>>>    
>>> ------>8-----------------------------------
>>>
>>> net: hide NET_TC_SKB_EXT as a config option
>>>
>>> Linus points out the NET_TC_SKB_EXT config option looks suspicious.
>>> Indeed, it should really be selected to ensure correct OvS operation
>>> if TC offload is used. Hopefully those who care about TC-only and
>>> OvS-only performance disable the other one at compilation time.
>>>
>>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> ---
>>>    net/openvswitch/Kconfig |  1 +
>>>    net/sched/Kconfig       | 13 +++----------
>>>    2 files changed, 4 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
>>> index 22d7d5604b4c..bd407ea7c263 100644
>>> --- a/net/openvswitch/Kconfig
>>> +++ b/net/openvswitch/Kconfig
>>> @@ -15,6 +15,7 @@ config OPENVSWITCH
>>>    	select NET_MPLS_GSO
>>>    	select DST_CACHE
>>>    	select NET_NSH
>>> +	select NET_TC_SKB_EXT if NET_CLS_ACT
>>>    	---help---
>>>    	  Open vSwitch is a multilayer Ethernet switch targeted at virtualized
>>>    	  environments.  In addition to supporting a variety of features
>>> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
>>> index b3faafeafab9..f1062ef55098 100644
>>> --- a/net/sched/Kconfig
>>> +++ b/net/sched/Kconfig
>>> @@ -719,6 +719,7 @@ config NET_EMATCH_IPT
>>>    config NET_CLS_ACT
>>>    	bool "Actions"
>>>    	select NET_CLS
>>> +	select NET_TC_SKB_EXT if OPENVSWITCH
>>
>> But how would that make much of a difference :( Distros are still going to
>> enable all of this blindlessly. Given discussion in [0], could we just get
>> rid of this tasteless hack altogether which is for such a narrow use case
>> anyway?
> 
> Agreed.  I take it you're opposed to the use of skb extensions here
> in general?  Distros would have enabled NET_TC_SKB_EXT even when it
> was a config option.

Yeah, as it stands, motivation for this extension is to tie tc [HW] offload
and OVS to work together in case of recirculation ... from commit:

   Received packets will first travel though tc, and if they aren't stolen
   by it, like in the above rule, they will continue to OvS datapath.

   Since we already did some actions (action ct in this case) which might
   modify the packets, and updated action stats, we would like to continue
   the proccessing with the correct recirc_id in OvS (here recirc_id(2))
   where we left off.

I would perhaps see more of a point in an skb extension if there is rather
generic use and also out of the SW datapath (and I really doubt anyone is
seriously using tc + OVS combo for non-HW workloads). Right now this feels
like duck taping.

Adding new skb extensions should really have a strong justification behind
it (close but slightly less strict to how we treat adding new members to
skb itself).

Re: CONFIG_NET_TC_SKB_EXT

[Daniel Borkmann]

On 9/21/19 12:56 AM, Jakub Kicinski wrote:
[...]
>> I thought idea of stuffing things into skb extensions are only justified if
>> it's not enabled by default for everyone. :(
>>
>>     [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u
> 
> The skb ext allocation is only done with GOTO_CHAIN, which AFAIK only
> has practical use for offload.  We could perhaps add another static
> branch there or move the OvS static branch out of the OvS module so
> there are no linking issues?
> 
> I personally have little sympathy for this piece of code, it is perhaps
> the purest form of a wobbly narrow-use construct pushed into TC for HW
> offload.
> 
> Any suggestions on the way forward? :(

Presumably there are no clean solutions here, but on the top of my head for
this use case, you'd need to /own/ the underlying datapath anyway, so couldn't
you program the OVS key->recirc_id based on skb->mark (or alternatively via
skb->tc_index) which was previously set by tc ingress?

Thanks,
Daniel

Re: CONFIG_NET_TC_SKB_EXT

[Pravin Shelar]

On Fri, Sep 20, 2019 at 5:06 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 9/21/19 12:56 AM, Jakub Kicinski wrote:
> [...]
> >> I thought idea of stuffing things into skb extensions are only justified if
> >> it's not enabled by default for everyone. :(
> >>
> >>     [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u
> >
> > The skb ext allocation is only done with GOTO_CHAIN, which AFAIK only
> > has practical use for offload.  We could perhaps add another static
> > branch there or move the OvS static branch out of the OvS module so
> > there are no linking issues?
> >
> > I personally have little sympathy for this piece of code, it is perhaps
> > the purest form of a wobbly narrow-use construct pushed into TC for HW
> > offload.
> >
> > Any suggestions on the way forward? :(
>
> Presumably there are no clean solutions here, but on the top of my head for
> this use case, you'd need to /own/ the underlying datapath anyway, so couldn't
> you program the OVS key->recirc_id based on skb->mark (or alternatively via
> skb->tc_index) which was previously set by tc ingress?
>

If we are going down this path, tc_index should be used. skb->mark is
already used in OVS flow match, overloading it for this purpose would
be complicated.

Re: CONFIG_NET_TC_SKB_EXT

[Alexei Starovoitov]

On Sat, Sep 21, 2019 at 8:15 PM Pravin Shelar <pshelar@ovn.org> wrote:
> > > Any suggestions on the way forward? :(

since there is no clear and agreed by everyone path forward the simple
revert is the best option.
Next release cycle you can come up with something that works for everyone.

Re: CONFIG_NET_TC_SKB_EXT

[Paul Blakey]

The skb extension is currently used for miss path of software offloading OvS rules with recirculation to tc.
However, we are also preparing patches to support the hardware side of things.

After the userspace OvS patches to support connection tracking, we'll have two users for
tc multi chain rules, one from those actually using tc and those translated from OvS rules.

With both of these use cases, there is similar issue of 'miss', from hardware to tc and tc to OvS and the skb
extension will serve to recover from both.

Consider, for example, the following multi chain tc rules:

tc filter add dev1 ... chain 0 flower dst_mac aa:bb:cc:dd:ee:ff action pedit munge ex set mac dst 02:a0:98:4e:8f:d1 pipe action goto chain 1
tc filter add dev1 ... chain 1 flower ip_dst 1.1.1.1 action mirred egress redirect dev2
tc filter add dev1 ... chain 1 flower ip_dst 2.2.2.2 action mirred egress redirect dev2

It's possible we offload the first two rules, but fail to offload the third rule, because of some hardware failure (e.g unsupported match or action).
If a packet with (dst_mac=aa:bb:cc:dd:ee:ff) and (dst ip=2.2.2.2) arrives,
we execute the goto chain 1 in hardware (and the pedit), and continue in chain 1, where we miss.

Currently we re-start the processing in tc from chain 0, even though we already did part of the processing in hardware.
The match on the dst_mac will fail as we already modified it, and we won't execute the third rule action.
In addition, if we did manage to execute any software tc rules, the packet will be counted twice.

We'll re-use this extension to solve this issue that currently exists by letting drivers tell tc on which chain to start the classification.

Regarding the config, we suggest changing the default to N and letting users decide to enable it, see attached patch.

Thanks,
Paul.


------------------------------------------------------------

Subject: [PATCH net-next] net/sched: Set default of CONFIG_NET_TC_SKB_EXT to N

This a new feature, it is prefered that it defaults to N.

Fixes: 95a7233c452a ('net: openvswitch: Set OvS recirc_id from tc chain index')
Signed-off-by: Paul Blakey <paulb@mellanox.com>
---
 net/sched/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index b3faafe..4bb10b7 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -966,7 +966,6 @@ config NET_IFE_SKBTCINDEX
 config NET_TC_SKB_EXT
 	bool "TC recirculation support"
 	depends on NET_CLS_ACT
-	default y if NET_CLS_ACT
 	select SKB_EXTENSIONS
 
 	help
-- 
1.8.3.1

Re: CONFIG_NET_TC_SKB_EXT

[Jakub Kicinski]

On Sun, 22 Sep 2019 14:51:44 +0300, Paul Blakey wrote:
> The skb extension is currently used for miss path of software offloading OvS rules with recirculation to tc.
> However, we are also preparing patches to support the hardware side of things.
> 
> After the userspace OvS patches to support connection tracking, we'll have two users for
> tc multi chain rules, one from those actually using tc and those translated from OvS rules.
> 
> With both of these use cases, there is similar issue of 'miss', from hardware to tc and tc to OvS and the skb
> extension will serve to recover from both.
> 
> Consider, for example, the following multi chain tc rules:
> 
> tc filter add dev1 ... chain 0 flower dst_mac aa:bb:cc:dd:ee:ff action pedit munge ex set mac dst 02:a0:98:4e:8f:d1 pipe action goto chain 1
> tc filter add dev1 ... chain 1 flower ip_dst 1.1.1.1 action mirred egress redirect dev2
> tc filter add dev1 ... chain 1 flower ip_dst 2.2.2.2 action mirred egress redirect dev2
> 
> It's possible we offload the first two rules, but fail to offload the third rule, because of some hardware failure (e.g unsupported match or action).
> If a packet with (dst_mac=aa:bb:cc:dd:ee:ff) and (dst ip=2.2.2.2) arrives,
> we execute the goto chain 1 in hardware (and the pedit), and continue in chain 1, where we miss.
> 
> Currently we re-start the processing in tc from chain 0, even though we already did part of the processing in hardware.
> The match on the dst_mac will fail as we already modified it, and we won't execute the third rule action.
> In addition, if we did manage to execute any software tc rules, the packet will be counted twice.
> 
> We'll re-use this extension to solve this issue that currently exists by letting drivers tell tc on which chain to start the classification.
> 
> Regarding the config, we suggest changing the default to N and letting users decide to enable it, see attached patch.

Partial offloads are very hard. Could we possibly take a page out of
routing offload's book and do a all or nothing offload in presence of
multiple chains?

> ------------------------------------------------------------
> 
> Subject: [PATCH net-next] net/sched: Set default of CONFIG_NET_TC_SKB_EXT to N
> 
> This a new feature, it is prefered that it defaults to N.
> 
> Fixes: 95a7233c452a ('net: openvswitch: Set OvS recirc_id from tc chain index')
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> ---
>  net/sched/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index b3faafe..4bb10b7 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -966,7 +966,6 @@ config NET_IFE_SKBTCINDEX
>  config NET_TC_SKB_EXT
>  	bool "TC recirculation support"
>  	depends on NET_CLS_ACT
> -	default y if NET_CLS_ACT
>  	select SKB_EXTENSIONS
>  
>  	help

 - Linus suggested we hide this option from user and autoselect if
   possible.
 - As Daniel said all distros will enable this.
 - If correctness is really our concern, giving users an option to
   select whether they want something to behave correctly seems like
   a hack of lowest kind. Perhaps it's time to add a CONFIG for TC
   offload in general? That's a meaningful set of functionality.

Re: CONFIG_NET_TC_SKB_EXT

[Paul Blakey]

On 9/23/2019 12:47 AM, Jakub Kicinski wrote:
> On Sun, 22 Sep 2019 14:51:44 +0300, Paul Blakey wrote:
>> The skb extension is currently used for miss path of software offloading OvS rules with recirculation to tc.
>> However, we are also preparing patches to support the hardware side of things.
>>
>> After the userspace OvS patches to support connection tracking, we'll have two users for
>> tc multi chain rules, one from those actually using tc and those translated from OvS rules.
>>
>> With both of these use cases, there is similar issue of 'miss', from hardware to tc and tc to OvS and the skb
>> extension will serve to recover from both.
>>
>> Consider, for example, the following multi chain tc rules:
>>
>> tc filter add dev1 ... chain 0 flower dst_mac aa:bb:cc:dd:ee:ff action pedit munge ex set mac dst 02:a0:98:4e:8f:d1 pipe action goto chain 1
>> tc filter add dev1 ... chain 1 flower ip_dst 1.1.1.1 action mirred egress redirect dev2
>> tc filter add dev1 ... chain 1 flower ip_dst 2.2.2.2 action mirred egress redirect dev2
>>
>> It's possible we offload the first two rules, but fail to offload the third rule, because of some hardware failure (e.g unsupported match or action).
>> If a packet with (dst_mac=aa:bb:cc:dd:ee:ff) and (dst ip=2.2.2.2) arrives,
>> we execute the goto chain 1 in hardware (and the pedit), and continue in chain 1, where we miss.
>>
>> Currently we re-start the processing in tc from chain 0, even though we already did part of the processing in hardware.
>> The match on the dst_mac will fail as we already modified it, and we won't execute the third rule action.
>> In addition, if we did manage to execute any software tc rules, the packet will be counted twice.
>>
>> We'll re-use this extension to solve this issue that currently exists by letting drivers tell tc on which chain to start the classification.
>>
>> Regarding the config, we suggest changing the default to N and letting users decide to enable it, see attached patch.
> Partial offloads are very hard. Could we possibly take a page out of
> routing offload's book and do a all or nothing offload in presence of
> multiple chains?

Hi,

Routing is a narrow scenario which this approach might work, but the 
other way of offloading everything or nothing has it's disadvantages too.

Even following this approach in tc only is challenging for some 
scenarios, consider the following tc rules:

tc filter add dev1 ... chain 0 flower <match1> action goto chain 1
tc filter add dev1 ... chain 0 flower <match2> action goto chain 1
tc filter add dev1 ... chain 0 flower <match3> action goto chain 1
..
..
tc filter add dev1 ... chain 0 flower ip_dst <match1000> action goto chain 1

tc filter add dev1 ... chain 1 flower ip_proto tcp action tunnel_key set dst_ip 8.8.8.8 action mirred egress redirect dev2


Now, you could offload 1000 'merged' rules that do the equivalent of:
tc filter add dev1 ... chain 0 flower <match1> ip_proto tcp action tunnel_key set dst_ip 8.8.8.8 action mirred egress redirect dev2
tc filter add dev1 ... chain 0 flower <match2> ip_proto tcp action tunnel_key set dst_ip 8.8.8.8 action mirred egress redirect dev2
...
tc filter add dev1 ... chain 0 flower <match1000> ip_proto tcp action tunnel_key set dst_ip 8.8.8.8 action mirred egress redirect dev2

And each of this hardware rule will update the correct software action counter, so the first hardware rule updates both the chain 1 rule counter and
chain 0 match1 rule counter.
This will work, but now you take dependencies on each of the children rules, so if you delete the chain 1 rule, you need
to invalidate all the 1000 hardware rules. And what about route updates or neigh updates for the encap header dst_ip 8.8.8.8?
Any change to that, and you update all the 1000 rules with the new mac address (hardware encapsulation needs to set outer mac address not supplied but tunnel key).
What if we had a depth of 10 chains?


You'd also have to keep track of what fields were originally on the packet, and not a match resulting from a modifications,
See the following set of rules:
tc filter add dev1 ... chain 0 prio 1 flower src_mac <mac1> action pedit munge ex set dst mac <mac2> pipe action goto chain 1
tc filter add dev1 ... chain 0 prio 2 flower dst_mac <mac2> action goto chain 1
tc filter add dev1 ... chain 1 prio 1 flower dst_mac <mac3> action <action3>
tc filter add dev1 ... chain 1 prio 2 flower src_mac <mac1> action <action4>


How would you offload that at the time of adding this rules, and not tracing the actual packet route?
can you tell that the third rule won't be hit? Will you flatten all possible routes and offload that?

Now take what other datapath (ovs, bridges, netfilter) blocks besides tc can do, or actions like ct that need software backing (unless you merge the ct table) and add them to the mix.
A single point of unsupported match/action between those, and we won't be able to offload the entire path, we might
even spend a lot of resources (trying to merge) before we even know it can't be offloaded, instead of doing what we can support.


>> ------------------------------------------------------------
>>
>> Subject: [PATCH net-next] net/sched: Set default of CONFIG_NET_TC_SKB_EXT to N
>>
>> This a new feature, it is prefered that it defaults to N.
>>
>> Fixes: 95a7233c452a ('net: openvswitch: Set OvS recirc_id from tc chain index')
>> Signed-off-by: Paul Blakey<paulb@mellanox.com>
>> ---
>>   net/sched/Kconfig | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
>> index b3faafe..4bb10b7 100644
>> --- a/net/sched/Kconfig
>> +++ b/net/sched/Kconfig
>> @@ -966,7 +966,6 @@ config NET_IFE_SKBTCINDEX
>>   config NET_TC_SKB_EXT
>>   	bool "TC recirculation support"
>>   	depends on NET_CLS_ACT
>> -	default y if NET_CLS_ACT
>>   	select SKB_EXTENSIONS
>>   
>>   	help
>   - Linus suggested we hide this option from user and autoselect if
>     possible.
>   - As Daniel said all distros will enable this.
>   - If correctness is really our concern, giving users an option to
>     select whether they want something to behave correctly seems like
>     a hack of lowest kind. Perhaps it's time to add a CONFIG for TC
>     offload in general? That's a meaningful set of functionality.

As I understand, Linus prefers this new feature to be default N and 
that's the what the attached patch.

The changes to the OVS datapath (static key and DP_SET command) with the 
userspace probe will ensure the correctness when the extension is disabled,

patch that I've yet to send is here:

https://www.spinics.net/lists/netdev/msg594175.html

This was going to be a part OvS userspace connection tracking offload 
patches to OvS userspace.

Paul.

Re: CONFIG_NET_TC_SKB_EXT

[Edward Cree]

On 23/09/2019 17:56, Paul Blakey wrote:
> Even following this approach in tc only is challenging for some 
> scenarios, consider the following tc rules:
>
> tc filter add dev1 ... chain 0 flower <match1> action goto chain 1
> tc filter add dev1 ... chain 0 flower <match2> action goto chain 1
> tc filter add dev1 ... chain 0 flower <match3> action goto chain 1
> ..
> ..
> tc filter add dev1 ... chain 0 flower ip_dst <match1000> action goto chain 1
>
> tc filter add dev1 ... chain 1 flower ip_proto tcp action tunnel_key set dst_ip 8.8.8.8 action mirred egress redirect dev2
This one is easy, if a packet gets to the end of a chain without matching any rules then it just gets delivered to the uplink (PF), and software TC starts over from the beginning of chain 0.  AFAICT this is the natural hardware behaviour for any design of offload, and it's inherently all-or-nothing.

> You'd also have to keep track of what fields were originally on the packet, and not a match resulting from a modifications,
> See the following set of rules:
> tc filter add dev1 ... chain 0 prio 1 flower src_mac <mac1> action pedit munge ex set dst mac <mac2> pipe action goto chain 1
> tc filter add dev1 ... chain 0 prio 2 flower dst_mac <mac2> action goto chain 1
> tc filter add dev1 ... chain 1 prio 1 flower dst_mac <mac3> action <action3>
> tc filter add dev1 ... chain 1 prio 2 flower src_mac <mac1> action <action4>
This one is slightly harder in that you have to either essentially 'gather up' actions as you go and only 'commit' them at the end of your processing pipeline (in which case you have to deal with the data hazard you cleverly included in your example), or keep a copy of the original packet around.  But I don't see the data-hazard case as having any realistic use-cases (does anyone actually put actions other than ct before a goto chain?) and it's easy enough to detect it in SW at rule-insertion time.

FWIW I think the only way this infra will ever be used is: chain 0 rules match <blah> action ct zone <n> goto chain <m>; chain <m> rules match ct_state +xyz <blah> action <actually do stuff to packet>.  This can be supported with a hardware pipeline with three tables in series in a fairly natural way — and since all the actions that modify the packet (rather than just directing subsequent matching) are at the end, the natural miss behaviour is deliver-unmodified.

I tried to explain this back on your [v3] net: openvswitch: Set OvS recirc_id from tc chain index, but you seemed set on your approach so I didn't persist.  Now it looks like maybe I should have...

Re: CONFIG_NET_TC_SKB_EXT

[Marcelo Ricardo Leitner]

On Sun, Sep 22, 2019 at 02:47:15PM -0700, Jakub Kicinski wrote:
> On Sun, 22 Sep 2019 14:51:44 +0300, Paul Blakey wrote:
...
> > ------------------------------------------------------------
> > 
> > Subject: [PATCH net-next] net/sched: Set default of CONFIG_NET_TC_SKB_EXT to N
> > 
> > This a new feature, it is prefered that it defaults to N.
> > 
> > Fixes: 95a7233c452a ('net: openvswitch: Set OvS recirc_id from tc chain index')
> > Signed-off-by: Paul Blakey <paulb@mellanox.com>
> > ---
> >  net/sched/Kconfig | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> > index b3faafe..4bb10b7 100644
> > --- a/net/sched/Kconfig
> > +++ b/net/sched/Kconfig
> > @@ -966,7 +966,6 @@ config NET_IFE_SKBTCINDEX
> >  config NET_TC_SKB_EXT
> >  	bool "TC recirculation support"
> >  	depends on NET_CLS_ACT
> > -	default y if NET_CLS_ACT
> >  	select SKB_EXTENSIONS
> >  
> >  	help
> 
>  - Linus suggested we hide this option from user and autoselect if
>    possible.
>  - As Daniel said all distros will enable this.
>  - If correctness is really our concern, giving users an option to
>    select whether they want something to behave correctly seems like
>    a hack of lowest kind. Perhaps it's time to add a CONFIG for TC
>    offload in general? That's a meaningful set of functionality.

Now that we have the static branch in place on OvS code, maybe we can
remove this option and rely just on NET_CLS_ACT instead (such as in
cls_api.c and skbuff.h).

Probing by OvS userspace is still somewhat possible, as the flag
OVS_DP_F_TC_RECIRC_SHARING wouldn't be known and be rejected
otherwise. 'somewhat because there is a limitation, as prior versions
(prior to this commit) didn't validate if the flags being set were at
least known.  But this is not news.

General OvS performance won't suffer, thanks to the static branch.

Drivers won't allocate the skb extension just because. They will only
do it when tc offloading is being done and, as already explained in
the thread and other places, it's needed for correct operation. So
general performance is also ok here.

For tc sw datapath, maybe we can have another static branch in there,
more specifically at tcf_classify(). Not sure how to control/toggle
it, though.

Re: CONFIG_NET_TC_SKB_EXT

[Paul Blakey]

On 9/23/2019 8:17 PM, Edward Cree wrote:

> On 23/09/2019 17:56, Paul Blakey wrote:
>> Even following this approach in tc only is challenging for some
>> scenarios, consider the following tc rules:
>>
>> tc filter add dev1 ... chain 0 flower <match1> action goto chain 1
>> tc filter add dev1 ... chain 0 flower <match2> action goto chain 1
>> tc filter add dev1 ... chain 0 flower <match3> action goto chain 1
>> ..
>> ..
>> tc filter add dev1 ... chain 0 flower ip_dst <match1000> action goto chain 1
>>
>> tc filter add dev1 ... chain 1 flower ip_proto tcp action tunnel_key set dst_ip 8.8.8.8 action mirred egress redirect dev2
> This one is easy, if a packet gets to the end of a chain without matching any rules then it just gets delivered to the uplink (PF), and software TC starts over from the beginning of chain 0.  AFAICT this is the natural hardware behaviour for any design of offload, and it's inherently all-or-nothing.
>> You'd also have to keep track of what fields were originally on the packet, and not a match resulting from a modifications,
>> See the following set of rules:
>> tc filter add dev1 ... chain 0 prio 1 flower src_mac <mac1> action pedit munge ex set dst mac <mac2> pipe action goto chain 1
>> tc filter add dev1 ... chain 0 prio 2 flower dst_mac <mac2> action goto chain 1
>> tc filter add dev1 ... chain 1 prio 1 flower dst_mac <mac3> action <action3>
>> tc filter add dev1 ... chain 1 prio 2 flower src_mac <mac1> action <action4>
> This one is slightly harder in that you have to either essentially 'gather up' actions as you go and only 'commit' them at the end of your processing pipeline (in which case you have to deal with the data hazard you cleverly included in your example), or keep a copy of the original packet around.  But I don't see the data-hazard case as having any realistic use-cases (does anyone actually put actions other than ct before a goto chain?) and it's easy enough to detect it in SW at rule-insertion time.

The 'miss' for all or nothing is easy, but the hard part is combining 
all the paths a packet can take in software to a single 'all or nothing' 
rule in hardware.

And when you do that, any change to any part of the path requires 
updating all the resulting rules. In the above case an update to the 
route of 8.8.8.8 affects the 1000 rules before it.

I can see two approaches to do the all of nothing approach, one is to 
statically analyze the software policy (tc rules in this case), and 
carefully offload

all possible paths. For that, you (who? the driver? tc? ovs?) would need 
to construct a graph, which even if you could do, there isn't any 
infrastructure for it, it would only valid while the configuration stays 
the same,

as any change to an edge on the graph requires re-analyzing it and 
possible changing tens of thousands of rules. In case of connection 
tracking, maybe hundred thousands...

Or the second approach, to save the expensive analysis, and unused 
hardware rules in the above, trace the path of processing the packet 
(gather and commit), save it somewhere on the SKB or per CPU, and 
creating new rules - basically a hardware cache.

But again, any change to the policy, and large chunks of this cache 
would be invalid.



Regarding the use-case, For OvS, as far as I know, it uses 
re-circulation for ct action, and dp hash actions, since it needs kernel 
help.

This extension is first introduced for ovs->tc offloads of connection 
tracking, but as we said, we are going to re-use it for tc->hardware in 
the same manner.

And that can take on many use-cases.  The above is a valid use-case if 
the controller isn't OvS.

The controller learn rules and insert the first 1000 rules matching on 
some 5-tuple in chain 0, but then when you want to encap the packets, 
you jump to the encap chain.

This allows you to update the encap action for all rules, and segment 
the logic for your policy.

tc chains are always used this way for a lot of cases which is 
segmenting policy logic.


> FWIW I think the only way this infra will ever be used is: chain 0 rules match <blah> action ct zone <n> goto chain <m>; chain <m> rules match ct_state +xyz <blah> action <actually do stuff to packet>.  This can be supported with a hardware pipeline with three tables in series in a fairly natural way — and since all the actions that modify the packet (rather than just directing subsequent matching) are at the end, the natural miss behaviour is deliver-unmodified.
>
> I tried to explain this back on your [v3] net: openvswitch: Set OvS recirc_id from tc chain index, but you seemed set on your approach so I didn't persist.  Now it looks like maybe I should have...

What you describe with the three tables (chain 0, chain X, and ct table 
for each ct zone), follows the software model and requires continuing 
from some miss point

and is what we plan on doing.

We offload something like:

chain 0 dst_mac aa:bb:cc:dd:ee:ff ct_state -trk  action ct (goto special 
CT table), action goto chain X

chain X ct_state +trk+est action forward to port

And a table that replicates the ct zone table,  which has something like

<match on tuple 1.... > set metadata ct_state = +est, action continue

<match on tuple 2.... > set metadata ct_state = +est, action continue

...

Lots tuples...

What if you 'miss' on the match for the tuple? You already did some 
processing in hardware, so either you revert those, or you continue in 
software where you left off  (the action ct).

We want to preserve the current software model, just as tc can do part 
of the processing and it will continue to the rest of the pipepline, be 
it OvS, bridge, or loopback. And in hardware we would do the same.


The all or nothing approach will require changing the software model to 
allow

merging the ct zone table matches into the hardware rules and offload 
something like:

dst_mac aa:bb:cc:dd:ee:ff  < match on tuple 1> fwd port

dst_mac aa:bb:cc:dd:ee:ff  < match on tuple 2> fwd port

...

Lots of 'merged' rules.

You delete the rule with action ct, and you have to delete all this 
merged, instead of just one rule.


Tracing the packet, or merging rules will require new infra to support this.

Re: CONFIG_NET_TC_SKB_EXT

[Edward Cree]

On 24/09/2019 12:48, Paul Blakey wrote:
> The 'miss' for all or nothing is easy, but the hard part is combining 
> all the paths a packet can take in software to a single 'all or nothing' 
> rule in hardware.
But you don't combine them to a single rule in hardware, because you
 have multiple sequential tables.  (I just spent the last few weeks
 telling our hardware guys that no, they can't just give us one big
 table and expect the driver to do all that combining, because as you
 say, it's 'the hard part'.)

> What if you 'miss' on the match for the tuple? You already did some 
> processing in hardware, so either you revert those, or you continue in 
> software where you left off  (the action ct).
But the only processing you did was to match stuff and generate metadata
 in the form of lookup keys (e.g. a ct_zone) for the next round of
 matching.  There's nothing to "revert" unless you've actually modified
 the packet before sending it to CT, and as I said I don't believe that's
 worth supporting.

> The all or nothing approach will require changing the software model to 
> allow
>
> merging the ct zone table matches into the hardware rules
I don't know how much more clearly I can say this: all-or-nothing does not
 require merging.  It just requires any actions that come before a matching
 stage (and that the hw doesn't have the capability to revert) to put a
 rule straight in the 'nothing' bucket.
So if you write
  chain 0 dst_mac aa:bb:cc:dd:ee:ff ct_state -trk  action vlan push blah action ct action goto chain X
 the driver can say -EOPNOTSUPP because you pushed a VLAN and might still
 miss in chain X.  But if you write
  chain 0 dst_mac aa:bb:cc:dd:ee:ff ct_state -trk  action ct action goto chain X
 then the driver will happily offload that because if you miss in the later
 lookups you've not altered the packet — the chain0-rule is *idempotent* so
 it doesn't matter if HW and SW both perform it.  (Or even all three of HW,
 tc and OvS.)

Re: CONFIG_NET_TC_SKB_EXT

[Paul Blakey]

On 9/25/2019 8:01 PM, Edward Cree wrote:
> On 24/09/2019 12:48, Paul Blakey wrote:
>> The 'miss' for all or nothing is easy, but the hard part is combining
>> all the paths a packet can take in software to a single 'all or nothing'
>> rule in hardware.
> But you don't combine them to a single rule in hardware, because you
>   have multiple sequential tables.  (I just spent the last few weeks
>   telling our hardware guys that no, they can't just give us one big
>   table and expect the driver to do all that combining, because as you
>   say, it's 'the hard part'.)
>
>> What if you 'miss' on the match for the tuple? You already did some
>> processing in hardware, so either you revert those, or you continue in
>> software where you left off  (the action ct).
> But the only processing you did was to match stuff and generate metadata
>   in the form of lookup keys (e.g. a ct_zone) for the next round of
>   matching.  There's nothing to "revert" unless you've actually modified
>   the packet before sending it to CT, and as I said I don't believe that's
>   worth supporting.
>
>> The all or nothing approach will require changing the software model to
>> allow
>>
>> merging the ct zone table matches into the hardware rules
> I don't know how much more clearly I can say this: all-or-nothing does not
>   require merging.  It just requires any actions that come before a matching
>   stage (and that the hw doesn't have the capability to revert) to put a
>   rule straight in the 'nothing' bucket.
> So if you write
>    chain 0 dst_mac aa:bb:cc:dd:ee:ff ct_state -trk  action vlan push blah action ct action goto chain X
>   the driver can say -EOPNOTSUPP because you pushed a VLAN and might still
>   miss in chain X.  But if you write
>    chain 0 dst_mac aa:bb:cc:dd:ee:ff ct_state -trk  action ct action goto chain X
>   then the driver will happily offload that because if you miss in the later
>   lookups you've not altered the packet — the chain0-rule is *idempotent* so
>   it doesn't matter if HW and SW both perform it.  (Or even all three of HW,
>   tc and OvS.)


Ok, I thought you meant merging the rules because we do want to support 
those modifications use-cases.

In nat scenarios the packet will be modified, and then there can be a miss:

            -trk .... CT(zone X, Restore NAT),goto chain 1

            +trk+est, match on ipv4, CT(zone Y), goto chain 2

            +trk+est, output..


In tunneling scenarios, the tunnel device decapsulates the packet before 
it even reaches OvS/Tc, which is another  modification.

Also, there are stats issues if we already accounted for some actions in 
hardware.


Paul.

Re: CONFIG_NET_TC_SKB_EXT

[Edward Cree]

On 26/09/2019 08:30, Paul Blakey wrote:
> Ok, I thought you meant merging the rules because we do want to support 
> those modifications use-cases.
I think the point is that your use-case is sufficiently weird and
 obscure that code in the core to support it needs to be unintrusive;
 and this clearly wasn't (you managed to piss off Linus...) so it
 should be reverted, and held off until a more palatable solution can
 be produced.  I agree with Alexei on this.
Neither currently-supported-by-drivers cases nor the first step that's
 likely to be added (the simple conntrack with modifications only at
 the end) needs this, it's for capabilities that are farther in the
 future, so there's really no need for it to be in the tree when it's
 not ready, which appears to be the case at present.
> In nat scenarios the packet will be modified, and then there can be a miss:
>
>             -trk .... CT(zone X, Restore NAT),goto chain 1
>
>             +trk+est, match on ipv4, CT(zone Y), goto chain 2
>
>             +trk+est, output..
I'm confused, I thought the usual nat scenario looked more like
    0: -trk ... action ct(zone x), goto chain 1
    1: +trk+new ... action ct(commit, nat=foo) # sw only
    1: +trk+est ... action ct(nat), mirred eth1
i.e. the NAT only happens after conntrack has matched (and thus provided
 the saved NAT metadata), at the end of the pipe.  I don't see how you
 can NAT a -trk packet.

> Also, there are stats issues if we already accounted for some actions in 
> hardware.
AFAICT only 'deliverish' actions (i.e. mirred and drop) in TC have stats.
So stats are unlikely to be a problem unless you've got (say) a mirred
 mirror before you send to ct and goto chain, in which case the extra
 copy of the packet is a rather bigger problem for idempotency than mere
 stats ;-)

Re: CONFIG_NET_TC_SKB_EXT

[Paul Blakey]

On 9/26/2019 4:09 PM, Edward Cree wrote:
> On 26/09/2019 08:30, Paul Blakey wrote:
>> Ok, I thought you meant merging the rules because we do want to support
>> those modifications use-cases.
> I think the point is that your use-case is sufficiently weird and
>   obscure that code in the core to support it needs to be unintrusive;
>   and this clearly wasn't (you managed to piss off Linus...) so it
>   should be reverted, and held off until a more palatable solution can
>   be produced.  I agree with Alexei on this.
> Neither currently-supported-by-drivers cases nor the first step that's
>   likely to be added (the simple conntrack with modifications only at
>   the end) needs this, it's for capabilities that are farther in the
>   future, so there's really no need for it to be in the tree when it's
>   not ready, which appears to be the case at present.

The use-case isn't weird, as I've just shown, even nat needs that. See 
below that even OvS has selftests that test that.

That's normal OvS nat, which you can see in many use-cases. You can take 
a look at different OvS controllers

and you would see the same behavior.


This is the first step that was required to support offloading 
connection tracking from OvS.

We are in progress of submitting the userspace patches, and would have 
done it today if it weren't'

for this CONFIG discussion. It was already submitted as RFC already.

Our driver implementation is ready and we will submit it once userspace 
is accepted.

>> In nat scenarios the packet will be modified, and then there can be a miss:
>>
>>              -trk .... CT(zone X, Restore NAT),goto chain 1
>>
>>              +trk+est, match on ipv4, CT(zone Y), goto chain 2
>>
>>              +trk+est, output..
> I'm confused, I thought the usual nat scenario looked more like
>      0: -trk ... action ct(zone x), goto chain 1
>      1: +trk+new ... action ct(commit, nat=foo) # sw only
>      1: +trk+est ... action ct(nat), mirred eth1
> i.e. the NAT only happens after conntrack has matched (and thus provided
>   the saved NAT metadata), at the end of the pipe.  I don't see how you
>   can NAT a -trk packet.

Both are valid, Nat in the first hop, executes the nat stored on the 
connection if available (configured by commit).

You can see it in multiple uses in ovs tests:  git grep -pni 
"table=0.*ct.*nat.*" tests/system-traffic.at

Even after you restore nat, you can jump to different zones, or even 
again to another table....

I've seen much deeper re-circulation chains (which usually means CT()) 
action.

See ovs self-tests as reference.


>
>> Also, there are stats issues if we already accounted for some actions in
>> hardware.
> AFAICT only 'deliverish' actions (i.e. mirred and drop) in TC have stats.
> So stats are unlikely to be a problem unless you've got (say) a mirred
>   mirror before you send to ct and goto chain, in which case the extra
>   copy of the packet is a rather bigger problem for idempotency than mere
>   stats ;-)

All tc actions have software stats, and at least one (goto, mirred, 
drop) per OvS generated rule will have hardware stats.

All OvS datapath rules have stats, and in turn the translated TC rules 
all have stats. OvS ages each rule independently.

Re: CONFIG_NET_TC_SKB_EXT

[Edward Cree]

On 26/09/2019 14:56, Paul Blakey wrote:
>>> In nat scenarios the packet will be modified, and then there can be a miss:
>>>
>>>              -trk .... CT(zone X, Restore NAT),goto chain 1
>>>
>>>              +trk+est, match on ipv4, CT(zone Y), goto chain 2
>>>
>>>              +trk+est, output..
>> I'm confused, I thought the usual nat scenario looked more like
>>      0: -trk ... action ct(zone x), goto chain 1
>>      1: +trk+new ... action ct(commit, nat=foo) # sw only
>>      1: +trk+est ... action ct(nat), mirred eth1
>> i.e. the NAT only happens after conntrack has matched (and thus provided
>>   the saved NAT metadata), at the end of the pipe.  I don't see how you
>>   can NAT a -trk packet.
> Both are valid, Nat in the first hop, executes the nat stored on the 
> connection if available (configured by commit).
This still isn't making sense to me.
Until you've done a conntrack lookup and found the connection, you can't
 use NAT information that's stored in the connection.
So the NAT can only happen after a conntrack match is found.

And all the rest of your stuff (like doing conntrack twice, in different
 zones X and Y) is 'weird' inasmuch as it's beyond the basic minimum
 functionality for a useful offload, and inherently doesn't map to a
 fixed-layout (non-loopy) HW pipeline.  You may want to support it in
 your driver, you may be able to support it in your hardware, but it's
 not true that "even nat needs that" (the nat scenario I described above
 is entirely reasonable and is perfectly workable in an all-or-nothing
 offload world), so if your changes are causing problems, they should be
 reverted for this cycle.

>> AFAICT only 'deliverish' actions (i.e. mirred and drop) in TC have stats.
>> So stats are unlikely to be a problem unless you've got (say) a mirred
>>   mirror before you send to ct and goto chain, in which case the extra
>>   copy of the packet is a rather bigger problem for idempotency than mere
>>   stats ;-)
> All tc actions have software stats, and at least one (goto, mirred, 
> drop) per OvS generated rule will have hardware stats.
Ooh, goto has hardware stats?  That's something I hadn't spotted *re-
 draws hardware design slightly*

> All OvS datapath rules have stats, and in turn the translated TC rules 
> all have stats. OvS ages each rule independently.
TC rules do not have stats.  Only TC actions have stats.  (The offload
 API currently gets confused about this and it's really annoyed me...)

Re: CONFIG_NET_TC_SKB_EXT

[Paul Blakey]

On 9/26/2019 5:26 PM, Edward Cree wrote:
> On 26/09/2019 14:56, Paul Blakey wrote:
>>>> In nat scenarios the packet will be modified, and then there can be a miss:
>>>>
>>>>               -trk .... CT(zone X, Restore NAT),goto chain 1
>>>>
>>>>               +trk+est, match on ipv4, CT(zone Y), goto chain 2
>>>>
>>>>               +trk+est, output..
>>> I'm confused, I thought the usual nat scenario looked more like
>>>       0: -trk ... action ct(zone x), goto chain 1
>>>       1: +trk+new ... action ct(commit, nat=foo) # sw only
>>>       1: +trk+est ... action ct(nat), mirred eth1
>>> i.e. the NAT only happens after conntrack has matched (and thus provided
>>>    the saved NAT metadata), at the end of the pipe.  I don't see how you
>>>    can NAT a -trk packet.
>> Both are valid, Nat in the first hop, executes the nat stored on the
>> connection if available (configured by commit).
> This still isn't making sense to me.
> Until you've done a conntrack lookup and found the connection, you can't
>   use NAT information that's stored in the connection.
> So the NAT can only happen after a conntrack match is found.

That's how it works, CT action restores the metadata (nf_conn on the 
SKB), you can then, if needed,  execute the nat,

That is stored implicitly on this metadata (by using the reverse of the 
reply tuple). It's why nf_conn status has two status bits 
(IPS_SRC_NAT_DONE_BIT  and IPS_SRC_NAT).

After you execute it, IPS_SRC_NAT_DONE_BIT  is set, instead of just 
IPS_SRC_NAT (nat needed bit).

The usage is straight from ovs testsuite, please see it in ovs git.

>
> And all the rest of your stuff (like doing conntrack twice, in different
>   zones X and Y) is 'weird' inasmuch as it's beyond the basic minimum
>   functionality for a useful offload, and inherently doesn't map to a
>   fixed-layout (non-loopy) HW pipeline.  You may want to support it in
>   your driver, you may be able to support it in your hardware, but it's
>   not true that "even nat needs that" (the nat scenario I described above
>   is entirely reasonable and is perfectly workable in an all-or-nothing
>   offload world), so if your changes are causing problems, they should be
>   reverted for this cycle.

The change didn't cause any problem, and this doesn't contradict any 
other vendor, doing what you suggest - offloading just a subset of the 
rules.

As we are discussing the config default, I've sent a patch to set the 
config to N.

Re: CONFIG_NET_TC_SKB_EXT

[Edward Cree]

On 26/09/2019 16:14, Paul Blakey wrote:
> On 9/26/2019 5:26 PM, Edward Cree wrote:
>> On 26/09/2019 14:56, Paul Blakey wrote:
>>>>> In nat scenarios the packet will be modified, and then there can be a miss:
>>>>>
>>>>>               -trk .... CT(zone X, Restore NAT),goto chain 1
>>>>>
>>>>>               +trk+est, match on ipv4, CT(zone Y), goto chain 2
>>>>>
>>>>>               +trk+est, output..
>>>> I'm confused, I thought the usual nat scenario looked more like
>>>>       0: -trk ... action ct(zone x), goto chain 1
>>>>       1: +trk+new ... action ct(commit, nat=foo) # sw only
>>>>       1: +trk+est ... action ct(nat), mirred eth1
>>>> i.e. the NAT only happens after conntrack has matched (and thus provided
>>>>    the saved NAT metadata), at the end of the pipe.  I don't see how you
>>>>    can NAT a -trk packet.
>>> Both are valid, Nat in the first hop, executes the nat stored on the
>>> connection if available (configured by commit).
>> This still isn't making sense to me.
>> Until you've done a conntrack lookup and found the connection, you can't
>>   use NAT information that's stored in the connection.
>> So the NAT can only happen after a conntrack match is found.
> That's how it works, CT action restores the metadata (nf_conn on the 
> SKB), you can then, if needed,  execute the nat,
Yes, and 'setting metadata' is idempotent (or reversible, if you prefer)
 so it doesn't matter if HW does it and then says "oops, can't do this"
 and software path starts from chain 0.
Problems only appear if you have _more matching_ after executing the NAT,
 in which case you're not a "simple NAT use-case" any more but rather the
 more complicated thing.

> The change didn't cause any problem
I mean, this whole thread is here because it *did* cause a problem:
 skb-extensions being turned on when not really needed.

> As we are discussing the config default, I've sent a patch to set the
> config to N.
As others have pointed out upthread, distros will just enable it anyway,
 regardless of the default.  As Daniel said,
> Adding new skb extensions should really have a strong justification behind
> it (close but slightly less strict to how we treat adding new members to
> skb itself). 
I'd also like to see a clear explanation of why you need extra out-of-skb
 storage space for this and can't use skb->tc_index as suggested by Pravin.