[ovs-dev] [PATCH V3 05/24] datapath: Set OvS recirc_id from tc chain index

[Yi-Hung Wei]

On Wed, Sep 16, 2020 at 10:33 AM Greg Rose <gvrose8192 at gmail.com> wrote:
>
> From: Paul Blakey <paulb at mellanox.com>
>
> Upstream commit:
>     commit 95a7233c452a58a4c2310c456c73997853b2ec46
>     Author: Paul Blakey <paulb at mellanox.com>
>     Date:   Wed Sep 4 16:56:37 2019 +0300
>
>     net: openvswitch: Set OvS recirc_id from tc chain index
>
>     Offloaded OvS datapath rules are translated one to one to tc rules,
>     for example the following simplified OvS rule:
>
>     recirc_id(0),in_port(dev1),eth_type(0x0800),ct_state(-trk) actions:ct(),recirc(2)
>
>     Will be translated to the following tc rule:
>
>     $ tc filter add dev dev1 ingress \
>                 prio 1 chain 0 proto ip \
>                     flower tcp ct_state -trk \
>                     action ct pipe \
>                     action goto chain 2
>
>     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.
>
>     To support this, introduce a new skb extension for tc, which
>     will be used for translating tc chain to ovs recirc_id to
>     handle these miss cases. Last tc chain index will be set
>     by tc goto chain action and read by OvS datapath.
>
>     Signed-off-by: Paul Blakey <paulb at mellanox.com>
>     Signed-off-by: Vlad Buslov <vladbu at mellanox.com>
>     Acked-by: Jiri Pirko <jiri at mellanox.com>
>     Acked-by: Pravin B Shelar <pshelar at ovn.org>
>     Signed-off-by: David S. Miller <davem at davemloft.net>
>
> Backport the local datapath changes from this patch and add compat
> layer fixup for the DECLARE_STATIC_KEY_FALSE macro.
>
> Cc: Paul Blakey <paulb at mellanox.com>
> Signed-off-by: Greg Rose <gvrose8192 at gmail.com>
> ---
>  acinclude.m4                                  |  3 ++
>  datapath/datapath.c                           | 34 ++++++++++++++++---
>  datapath/datapath.h                           |  2 ++
>  datapath/flow.c                               | 13 +++++++
>  .../linux/compat/include/linux/static_key.h   |  7 ++++
>  5 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index 84f344da0..3d56510a0 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -631,6 +631,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>                    [OVS_DEFINE([HAVE_UPSTREAM_STATIC_KEY])])
>    OVS_GREP_IFELSE([$KSRC/include/linux/jump_label.h], [DEFINE_STATIC_KEY_FALSE],
>                    [OVS_DEFINE([HAVE_DEFINE_STATIC_KEY])])
> +  OVS_GREP_IFELSE([$KSRC/include/linux/jump_label.h],
> +                  [DECLARE_STATIC_KEY_FALSE],
> +                  [OVS_DEFINE([HAVE_DECLARE_STATIC_KEY])])
>
>    OVS_GREP_IFELSE([$KSRC/include/linux/etherdevice.h], [eth_hw_addr_random])
>    OVS_GREP_IFELSE([$KSRC/include/linux/etherdevice.h], [ether_addr_copy])
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index c8c21d774..36f1e7894 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -1635,10 +1635,34 @@ static void ovs_dp_reset_user_features(struct sk_buff *skb, struct genl_info *in
>         dp->user_features = 0;
>  }
>
> -static void ovs_dp_change(struct datapath *dp, struct nlattr *a[])
> +DEFINE_STATIC_KEY_FALSE(tc_recirc_sharing_support);
> +
> +static int ovs_dp_change(struct datapath *dp, struct nlattr *a[])
>  {
> -       if (a[OVS_DP_ATTR_USER_FEATURES])
> -               dp->user_features = nla_get_u32(a[OVS_DP_ATTR_USER_FEATURES]);
> +       u32 user_features = 0;
> +
> +       if (a[OVS_DP_ATTR_USER_FEATURES]) {
> +               user_features = nla_get_u32(a[OVS_DP_ATTR_USER_FEATURES]);
> +
> +               if (user_features & ~(OVS_DP_F_VPORT_PIDS |
> +                                     OVS_DP_F_UNALIGNED |
> +                                     OVS_DP_F_TC_RECIRC_SHARING))
> +                       return -EOPNOTSUPP;
> +
> +#if !IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
> +               if (user_features & OVS_DP_F_TC_RECIRC_SHARING)
> +                       return -EOPNOTSUPP;
> +#endif
> +       }
> +
> +       dp->user_features = user_features;
> +
> +       if (dp->user_features & OVS_DP_F_TC_RECIRC_SHARING)
> +               static_branch_enable(&tc_recirc_sharing_support);
> +       else
> +               static_branch_disable(&tc_recirc_sharing_support);
> +
> +       return 0;
>  }
>
>  static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
> @@ -1700,7 +1724,9 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
>         parms.port_no = OVSP_LOCAL;
>         parms.upcall_portids = a[OVS_DP_ATTR_UPCALL_PID];
>
> -       ovs_dp_change(dp, a);
> +       err = ovs_dp_change(dp, a);
> +       if (err)
> +               goto err_destroy_meters;
>
>         /* So far only local changes have been made, now need the lock. */
>         ovs_lock();

Hi Greg,

In the upstream commit, it also updates ovs_dp_cmd_set() which seems
to be missing in this patch.  Can you double check that?

@@ -1736,7 +1762,9 @@ static int ovs_dp_cmd_set(struct sk_buff *skb,
struct genl_info *info)
        if (IS_ERR(dp))
                goto err_unlock_free;

-       ovs_dp_change(dp, info->attrs);
+       err = ovs_dp_change(dp, info->attrs);
+       if (err)
+               goto err_unlock_free;

Thanks,

-Yi-Hung