[ovs-dev] [PATCH v3 07/10] netdev-offload-tc: Add recirculation support via tc chains

Ilya Maximets i.maximets at ovn.org
Tue Dec 3 15:14:50 UTC 2019


On 03.12.2019 14:45, Roi Dayan wrote:
> From: Paul Blakey <paulb at mellanox.com>
> 
> Each recirculation id will create a tc chain, and we translate
> the recirculation action to a tc goto chain action.
> 
> We check for kernel support for this by probing OvS Datapath for the
> tc recirc id sharing feature. If supported, we can offload rules
> that match on recirc_id, and recirculation action safely.
> ---
> 
> Changelog:
> V2->V3:
>     Merged part of probe for recirc_id support in here to help future git bisect.
>     Added tunnel released check to avoid bug with mirroring
>     Removed cascading condition in netdev_tc_flow_put() check of recirc_id support
> 
> V1->V2:
>     moved make_tc_id_chain helper to tc.h as static inline
>     updated is_tc_id_eq with chain compare instead of find_ufid
> 
> Signed-off-by: Paul Blakey <paulb at mellanox.com>

This tag should go before the '---', otherwise it'll not be part of a commit-message.
You may see that checkpatch complains about missing signed-off on some of the patches.

> ---
>  lib/dpif-netlink.c      |  1 +
>  lib/netdev-offload-tc.c | 61 +++++++++++++++++++++++++++++++++++++++++--------
>  lib/tc.c                | 49 ++++++++++++++++++++++++++++++++++-----
>  lib/tc.h                | 18 ++++++++++++++-
>  4 files changed, 112 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 92da918544d1..f0e870543ae5 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1638,6 +1638,7 @@ dpif_netlink_netdev_match_to_dpif_flow(struct match *match,
>          .mask = &match->wc.masks,
>          .support = {
>              .max_vlan_headers = 2,
> +            .recirc = true,
>          },
>      };
>      size_t offset;
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index bb0eccd79ceb..7af5ad683bb7 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -38,6 +38,7 @@
>  #include "tc.h"
>  #include "unaligned.h"
>  #include "util.h"
> +#include "dpif-provider.h"
>  
>  VLOG_DEFINE_THIS_MODULE(netdev_offload_tc);
>  
> @@ -206,9 +207,12 @@ static void
>  add_ufid_tc_mapping(struct netdev *netdev, const ovs_u128 *ufid,
>                      struct tcf_id *id)
>  {
> -    size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0);
> -    size_t tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex);
>      struct ufid_tc_data *new_data = xzalloc(sizeof *new_data);
> +    size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0);
> +    size_t tc_hash;
> +
> +    tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex);
> +    tc_hash = hash_int(id->chain, tc_hash);
>  
>      new_data->ufid = *ufid;
>      new_data->id = *id;
> @@ -252,8 +256,11 @@ get_ufid_tc_mapping(const ovs_u128 *ufid, struct tcf_id *id)
>  static bool
>  find_ufid(struct netdev *netdev, struct tcf_id *id, ovs_u128 *ufid)
>  {
> -    size_t tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex);
>      struct ufid_tc_data *data;
> +    size_t tc_hash;
> +
> +    tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex);
> +    tc_hash = hash_int(id->chain, tc_hash);
>  
>      ovs_mutex_lock(&ufid_lock);
>      HMAP_FOR_EACH_WITH_HASH (data, tc_to_ufid_node, tc_hash,  &tc_to_ufid) {
> @@ -739,6 +746,10 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>                  nl_msg_put_u32(buf, OVS_ACTION_ATTR_OUTPUT, odp_to_u32(outport));
>              }
>              break;
> +            case TC_ACT_GOTO: {
> +                nl_msg_put_u32(buf, OVS_ACTION_ATTR_RECIRC, action->chain);
> +            }
> +            break;
>              }
>          }
>      }
> @@ -799,6 +810,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
>  
>          match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX);
>          match->flow.in_port.odp_port = dump->port;
> +        match_set_recirc_id(match, id.chain);
>  
>          return true;
>      }
> @@ -983,12 +995,6 @@ test_key_and_mask(struct match *match)
>          return EOPNOTSUPP;
>      }
>  
> -    if (mask->recirc_id && key->recirc_id) {
> -        VLOG_DBG_RL(&rl, "offloading attribute recirc_id isn't supported");
> -        return EOPNOTSUPP;
> -    }
> -    mask->recirc_id = 0;
> -
>      if (mask->dp_hash) {
>          VLOG_DBG_RL(&rl, "offloading attribute dp_hash isn't supported");
>          return EOPNOTSUPP;
> @@ -1139,6 +1145,25 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl,
>      flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len;
>  }
>  
> +static bool
> +recirc_id_sharing_support(struct dpif *dpif)
> +{
> +    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +    static bool supported = false;
> +    int err;
> +
> +    if (ovsthread_once_start(&once)) {
> +        err = dpif_set_features(dpif, OVS_DP_F_TC_RECIRC_SHARING);

I don't think that it's the right thing to do to change datapath configuration
from the netdev-offload implementation.  This also requires you to have access
to the dpif-provider internals breaking the OVS architecture of modules. (You
may see that this patch set doesn't build at some point because you need to know
the internal structure of a dpif instance.)

I'd suggest to move this to the common feature probing code, i.e. to ofproto.
After that you may pass supported features via offload_info structure.

Thoughts?

Best regards, Ilya Maximets.


More information about the dev mailing list