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

[Gregory Rose]

On 9/28/2020 2:46 PM, Yi-Hung Wei wrote:
> 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
> 

Good catch!  Yes, I'll fix this one up.

- Greg