* CONFIG_NET_TC_SKB_EXT @ 2019-09-19 11:21 David Miller 2019-09-19 15:13 ` CONFIG_NET_TC_SKB_EXT Vlad Buslov 0 siblings, 1 reply; 22+ messages in thread From: David Miller @ 2019-09-19 11:21 UTC (permalink / raw) To: paulb; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-19 11:21 CONFIG_NET_TC_SKB_EXT David Miller @ 2019-09-19 15:13 ` Vlad Buslov 2019-09-20 16:16 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski 0 siblings, 1 reply; 22+ messages in thread From: Vlad Buslov @ 2019-09-19 15:13 UTC (permalink / raw) To: David Miller; +Cc: Paul Blakey, netdev 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-19 15:13 ` CONFIG_NET_TC_SKB_EXT Vlad Buslov @ 2019-09-20 16:16 ` Jakub Kicinski 2019-09-20 22:15 ` CONFIG_NET_TC_SKB_EXT Daniel Borkmann 0 siblings, 1 reply; 22+ messages in thread From: Jakub Kicinski @ 2019-09-20 16:16 UTC (permalink / raw) To: Vlad Buslov Cc: David Miller, Paul Blakey, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar, Simon Horman, Daniel Borkmann, Alexei Starovoitov, Or Gerlitz 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-20 16:16 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski @ 2019-09-20 22:15 ` Daniel Borkmann 2019-09-20 22:56 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski 0 siblings, 1 reply; 22+ messages in thread From: Daniel Borkmann @ 2019-09-20 22:15 UTC (permalink / raw) To: Jakub Kicinski, Vlad Buslov Cc: David Miller, Paul Blakey, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar, Simon Horman, Alexei Starovoitov, Or Gerlitz 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 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-20 22:15 ` CONFIG_NET_TC_SKB_EXT Daniel Borkmann @ 2019-09-20 22:56 ` Jakub Kicinski 2019-09-20 23:40 ` CONFIG_NET_TC_SKB_EXT Daniel Borkmann 2019-09-21 0:06 ` CONFIG_NET_TC_SKB_EXT Daniel Borkmann 0 siblings, 2 replies; 22+ messages in thread From: Jakub Kicinski @ 2019-09-20 22:56 UTC (permalink / raw) To: Daniel Borkmann Cc: Vlad Buslov, David Miller, Paul Blakey, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar, Simon Horman, Alexei Starovoitov, Or Gerlitz 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? :( ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-20 22:56 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski @ 2019-09-20 23:40 ` Daniel Borkmann 2019-09-21 0:06 ` CONFIG_NET_TC_SKB_EXT Daniel Borkmann 1 sibling, 0 replies; 22+ messages in thread From: Daniel Borkmann @ 2019-09-20 23:40 UTC (permalink / raw) To: Jakub Kicinski Cc: Vlad Buslov, David Miller, Paul Blakey, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar, Simon Horman, Alexei Starovoitov, Or Gerlitz 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). ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-20 22:56 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski 2019-09-20 23:40 ` CONFIG_NET_TC_SKB_EXT Daniel Borkmann @ 2019-09-21 0:06 ` Daniel Borkmann 2019-09-22 3:18 ` CONFIG_NET_TC_SKB_EXT Pravin Shelar 1 sibling, 1 reply; 22+ messages in thread From: Daniel Borkmann @ 2019-09-21 0:06 UTC (permalink / raw) To: Jakub Kicinski Cc: Vlad Buslov, David Miller, Paul Blakey, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar, Simon Horman, Alexei Starovoitov, Or Gerlitz 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-21 0:06 ` CONFIG_NET_TC_SKB_EXT Daniel Borkmann @ 2019-09-22 3:18 ` Pravin Shelar 2019-09-22 4:51 ` CONFIG_NET_TC_SKB_EXT Alexei Starovoitov 0 siblings, 1 reply; 22+ messages in thread From: Pravin Shelar @ 2019-09-22 3:18 UTC (permalink / raw) To: Daniel Borkmann Cc: Jakub Kicinski, Vlad Buslov, David Miller, Paul Blakey, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Alexei Starovoitov, Or Gerlitz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-22 3:18 ` CONFIG_NET_TC_SKB_EXT Pravin Shelar @ 2019-09-22 4:51 ` Alexei Starovoitov 2019-09-22 11:51 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 0 siblings, 1 reply; 22+ messages in thread From: Alexei Starovoitov @ 2019-09-22 4:51 UTC (permalink / raw) To: Pravin Shelar Cc: Daniel Borkmann, Jakub Kicinski, Vlad Buslov, David Miller, Paul Blakey, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-22 4:51 ` CONFIG_NET_TC_SKB_EXT Alexei Starovoitov @ 2019-09-22 11:51 ` Paul Blakey 2019-09-22 21:47 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski 0 siblings, 1 reply; 22+ messages in thread From: Paul Blakey @ 2019-09-22 11:51 UTC (permalink / raw) To: Pravin Shelar, Daniel B^Ckmann, Jakub Kicinski, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz Cc: 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-22 11:51 ` CONFIG_NET_TC_SKB_EXT Paul Blakey @ 2019-09-22 21:47 ` Jakub Kicinski 2019-09-23 16:56 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 2019-09-23 18:46 ` CONFIG_NET_TC_SKB_EXT Marcelo Ricardo Leitner 0 siblings, 2 replies; 22+ messages in thread From: Jakub Kicinski @ 2019-09-22 21:47 UTC (permalink / raw) To: Paul Blakey Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-22 21:47 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski @ 2019-09-23 16:56 ` Paul Blakey 2019-09-23 17:17 ` CONFIG_NET_TC_SKB_EXT Edward Cree 2019-09-23 18:46 ` CONFIG_NET_TC_SKB_EXT Marcelo Ricardo Leitner 1 sibling, 1 reply; 22+ messages in thread From: Paul Blakey @ 2019-09-23 16:56 UTC (permalink / raw) To: Jakub Kicinski Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-23 16:56 ` CONFIG_NET_TC_SKB_EXT Paul Blakey @ 2019-09-23 17:17 ` Edward Cree 2019-09-24 11:48 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 0 siblings, 1 reply; 22+ messages in thread From: Edward Cree @ 2019-09-23 17:17 UTC (permalink / raw) To: Paul Blakey, Jakub Kicinski Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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... ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-23 17:17 ` CONFIG_NET_TC_SKB_EXT Edward Cree @ 2019-09-24 11:48 ` Paul Blakey 2019-09-25 17:01 ` CONFIG_NET_TC_SKB_EXT Edward Cree 0 siblings, 1 reply; 22+ messages in thread From: Paul Blakey @ 2019-09-24 11:48 UTC (permalink / raw) To: Edward Cree, Jakub Kicinski Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-24 11:48 ` CONFIG_NET_TC_SKB_EXT Paul Blakey @ 2019-09-25 17:01 ` Edward Cree 2019-09-26 7:30 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 0 siblings, 1 reply; 22+ messages in thread From: Edward Cree @ 2019-09-25 17:01 UTC (permalink / raw) To: Paul Blakey, Jakub Kicinski Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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.) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-25 17:01 ` CONFIG_NET_TC_SKB_EXT Edward Cree @ 2019-09-26 7:30 ` Paul Blakey 2019-09-26 13:09 ` CONFIG_NET_TC_SKB_EXT Edward Cree 0 siblings, 1 reply; 22+ messages in thread From: Paul Blakey @ 2019-09-26 7:30 UTC (permalink / raw) To: Edward Cree, Jakub Kicinski Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-26 7:30 ` CONFIG_NET_TC_SKB_EXT Paul Blakey @ 2019-09-26 13:09 ` Edward Cree 2019-09-26 13:56 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 0 siblings, 1 reply; 22+ messages in thread From: Edward Cree @ 2019-09-26 13:09 UTC (permalink / raw) To: Paul Blakey, Jakub Kicinski Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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 ;-) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-26 13:09 ` CONFIG_NET_TC_SKB_EXT Edward Cree @ 2019-09-26 13:56 ` Paul Blakey 2019-09-26 14:26 ` CONFIG_NET_TC_SKB_EXT Edward Cree 0 siblings, 1 reply; 22+ messages in thread From: Paul Blakey @ 2019-09-26 13:56 UTC (permalink / raw) To: Edward Cree, Jakub Kicinski Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-26 13:56 ` CONFIG_NET_TC_SKB_EXT Paul Blakey @ 2019-09-26 14:26 ` Edward Cree 2019-09-26 15:14 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 0 siblings, 1 reply; 22+ messages in thread From: Edward Cree @ 2019-09-26 14:26 UTC (permalink / raw) To: Paul Blakey, Jakub Kicinski Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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...) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-26 14:26 ` CONFIG_NET_TC_SKB_EXT Edward Cree @ 2019-09-26 15:14 ` Paul Blakey 2019-09-26 17:02 ` CONFIG_NET_TC_SKB_EXT Edward Cree 0 siblings, 1 reply; 22+ messages in thread From: Paul Blakey @ 2019-09-26 15:14 UTC (permalink / raw) To: Edward Cree, Jakub Kicinski Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-26 15:14 ` CONFIG_NET_TC_SKB_EXT Paul Blakey @ 2019-09-26 17:02 ` Edward Cree 0 siblings, 0 replies; 22+ messages in thread From: Edward Cree @ 2019-09-26 17:02 UTC (permalink / raw) To: Paul Blakey, Jakub Kicinski Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: CONFIG_NET_TC_SKB_EXT 2019-09-22 21:47 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski 2019-09-23 16:56 ` CONFIG_NET_TC_SKB_EXT Paul Blakey @ 2019-09-23 18:46 ` Marcelo Ricardo Leitner 1 sibling, 0 replies; 22+ messages in thread From: Marcelo Ricardo Leitner @ 2019-09-23 18:46 UTC (permalink / raw) To: Jakub Kicinski Cc: Paul Blakey, Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller, netdev, Jiri Pirko, Cong Wang, Jamal Hadi Salim, Simon Horman, Or Gerlitz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-09-26 17:02 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-19 11:21 CONFIG_NET_TC_SKB_EXT David Miller 2019-09-19 15:13 ` CONFIG_NET_TC_SKB_EXT Vlad Buslov 2019-09-20 16:16 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski 2019-09-20 22:15 ` CONFIG_NET_TC_SKB_EXT Daniel Borkmann 2019-09-20 22:56 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski 2019-09-20 23:40 ` CONFIG_NET_TC_SKB_EXT Daniel Borkmann 2019-09-21 0:06 ` CONFIG_NET_TC_SKB_EXT Daniel Borkmann 2019-09-22 3:18 ` CONFIG_NET_TC_SKB_EXT Pravin Shelar 2019-09-22 4:51 ` CONFIG_NET_TC_SKB_EXT Alexei Starovoitov 2019-09-22 11:51 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 2019-09-22 21:47 ` CONFIG_NET_TC_SKB_EXT Jakub Kicinski 2019-09-23 16:56 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 2019-09-23 17:17 ` CONFIG_NET_TC_SKB_EXT Edward Cree 2019-09-24 11:48 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 2019-09-25 17:01 ` CONFIG_NET_TC_SKB_EXT Edward Cree 2019-09-26 7:30 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 2019-09-26 13:09 ` CONFIG_NET_TC_SKB_EXT Edward Cree 2019-09-26 13:56 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 2019-09-26 14:26 ` CONFIG_NET_TC_SKB_EXT Edward Cree 2019-09-26 15:14 ` CONFIG_NET_TC_SKB_EXT Paul Blakey 2019-09-26 17:02 ` CONFIG_NET_TC_SKB_EXT Edward Cree 2019-09-23 18:46 ` CONFIG_NET_TC_SKB_EXT Marcelo Ricardo Leitner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).