[ovs-dev] [PATCH V9 07/31] other-config: Add tc-policy switch to control tc flower flag

[Simon Horman]

On Sun, May 28, 2017 at 02:59:49PM +0300, Roi Dayan wrote:
> From: Paul Blakey <paulb at mellanox.com>
> 
> Add a new configuration tc-policy option that controls tc
> flower flag. Possible options are none, skip_sw, skip_hw.
> The default is none which is to insert the rule both to sw and hw.
> This option is only relevant if hw-offload is enabled.
> 
> Signed-off-by: Paul Blakey <paulb at mellanox.com>
> Reviewed-by: Roi Dayan <roid at mellanox.com>
> Reviewed-by: Simon Horman <simon.horman at netronome.com>

Thanks, the comment below not withstanding this looks good to me
and I would be happy to apply it if someone provided a review.

> ---
>  lib/netdev.c         |  6 ++++++
>  lib/tc.c             | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  lib/tc.h             |  3 +++
>  vswitchd/vswitch.xml | 17 +++++++++++++++++
>  4 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 2807a9d..19d809b 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -54,6 +54,9 @@
>  #include "openvswitch/vlog.h"
>  #include "flow.h"
>  #include "util.h"
> +#ifdef __linux__
> +#include "tc.h"
> +#endif
>  
>  VLOG_DEFINE_THIS_MODULE(netdev);
>  
> @@ -2117,6 +2120,9 @@ netdev_set_flow_api_enabled(const struct smap *ovs_other_config)
>  
>              VLOG_INFO("netdev: Flow API Enabled");
>  
> +            tc_set_policy(smap_get_def(ovs_other_config, "tc-policy",
> +                                       TC_POLICY_DEFAULT));
> +
>              ovsthread_once_done(&once);
>          }
>      }

In v7 Flavio Leitner made the following comment regarding this.

  "Well, this enforces that the user sets tc-policy before enabling HW
   offloading, otherwise it won't get set, right?"

My comment, along the same lines, is that it looks like it would be better
if tc_set_policy() was called independent of netdev_set_flow_api_enabled()
to allow them to be configured independently.

I realise that at this point the patch-set has some limitations regarding
restarting ovs when offloading of flows is enabled and disabled. And
probably the code above plays well with that restriction in place. But I
think it would be good to look into ways to lift that restriction and thus
it would seems worthwhile revisiting where tc_set_policy() is called.