Re: CONFIG_NET_TC_SKB_EXT

[Daniel Borkmann <daniel@iogearbox.net>]

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
>