[ovs-dev] [PATCH v17 7/8] netdev-offload-tc: Add offload support for sFlow

Eelco Chaudron echaudro at redhat.com
Fri Nov 5 12:26:24 UTC 2021


Hi Chris,

Only one thing you forgot to fix from the previous review.

If you make no other changes than the below, you can add my ack.

//Eelco

On 21 Oct 2021, at 10:01, Chris Mi wrote:

> Create a unique group ID to map the sFlow info when offloading sFlow
> action to TC. When showing the offloaded datapath flows, translate the
> group ID from TC sample action to sFlow info using the mapping.
>
> Signed-off-by: Chris Mi <cmi at nvidia.com>
> Reviewed-by: Eli Britstein <elibr at nvidia.com>
> ---
>  NEWS                        |   1 +
>  lib/dpif-offload-netlink.c  |   6 +
>  lib/dpif-offload-provider.h |   1 +
>  lib/netdev-offload-tc.c     | 228 +++++++++++++++++++++++++++++++++---
>  lib/tc.c                    |  61 +++++++++-
>  lib/tc.h                    |  15 ++-
>  6 files changed, 290 insertions(+), 22 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 90f4b1590..d34c87b3c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,6 +10,7 @@ Post-v2.16.0
>         limiting behavior.
>       * Add hardware offload support for matching IPv4/IPv6 frag types
>         (experimental).
> +   - Add sFlow offload support for kernel (netlink) datapath.
>
>
>  v2.16.0 - 16 Aug 2021
> diff --git a/lib/dpif-offload-netlink.c b/lib/dpif-offload-netlink.c
> index 7f53ed582..e72775e7a 100644
> --- a/lib/dpif-offload-netlink.c
> +++ b/lib/dpif-offload-netlink.c
> @@ -205,3 +205,9 @@ const struct dpif_offload_api dpif_offload_netlink = {
>      .sflow_recv_wait = dpif_offload_netlink_sflow_recv_wait,
>      .sflow_recv = dpif_offload_netlink_sflow_recv,
>  };
> +
> +bool
> +dpif_offload_netlink_psample_supported(void)
> +{
> +    return psample_sock != NULL;
> +}
> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h
> index 5ac419516..0947f3462 100644
> --- a/lib/dpif-offload-provider.h
> +++ b/lib/dpif-offload-provider.h
> @@ -65,6 +65,7 @@ int dpif_offload_sflow_recv(const struct dpif *dpif,
>  extern const struct dpif_offload_api dpif_offload_netlink;
>  int dpif_offload_netlink_init(void);
>  void dpif_offload_netlink_destroy(void);
> +bool dpif_offload_netlink_psample_supported(void);
>  #endif
>
>  #endif /* DPIF_OFFLOAD_PROVIDER_H */
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 204d1c833..f31471da9 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -20,6 +20,7 @@
>  #include <linux/if_ether.h>
>
>  #include "dpif.h"
> +#include "dpif-offload-provider.h"
>  #include "hash.h"
>  #include "openvswitch/hmap.h"
>  #include "openvswitch/match.h"
> @@ -1052,6 +1053,19 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>          action = flower->actions;
>          for (i = 0; i < flower->action_count; i++, action++) {
>              switch (action->type) {
> +            case TC_ACT_SAMPLE: {
> +                const struct sgid_node *node;
> +
> +                node = sgid_find(action->sample.group_id);
> +                if (!node) {
> +                    VLOG_WARN("Can't find sample group ID data for ID: %u",
> +                              action->sample.group_id);
> +                    return ENOENT;
> +                }
> +                nl_msg_put(buf, node->sflow.action,
> +                           node->sflow.action->nla_len);
> +            }
> +            break;
>              case TC_ACT_VLAN_POP: {
>                  nl_msg_put_flag(buf, OVS_ACTION_ATTR_POP_VLAN);
>              }
> @@ -1790,6 +1804,151 @@ parse_match_ct_state_to_flower(struct tc_flower *flower, struct match *match)
>      }
>  }
>
> +static int
> +parse_userspace_attributes(const struct nlattr *actions,
> +                           struct dpif_sflow_attr *sflow_attr)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +    const struct nlattr *nla;
> +    unsigned int left;
> +
> +    NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
> +        if (nl_attr_type(nla) == OVS_USERSPACE_ATTR_USERDATA) {
> +            struct user_action_cookie *cookie;
> +
> +            cookie = CONST_CAST(struct user_action_cookie *, nl_attr_get(nla));
> +            if (cookie->type == USER_ACTION_COOKIE_SFLOW) {
> +                sflow_attr->userdata = nla;
> +                return 0;
> +            }
> +        }
> +    }
> +
> +    VLOG_DBG_RL(&rl, "Can't offload userspace action other than sFlow");
> +    return EOPNOTSUPP;
> +}
> +
> +static int
> +parse_sample_actions_attribute(const struct nlattr *actions,
> +                               struct dpif_sflow_attr *sflow_attr)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +    const struct nlattr *nla;
> +    unsigned int left;
> +    int err = EINVAL;
> +
> +    NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
> +        if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
> +            err = parse_userspace_attributes(nla, sflow_attr);
> +        } else {
> +            /* We can't offload other nested actions */
> +            VLOG_DBG_RL(&rl, "Can only offload OVS_ACTION_ATTR_USERSPACE"
> +                             " attribute");
> +            return EINVAL;
> +        }
> +    }
> +
> +    if (err) {
> +        VLOG_ERR_RL(&error_rl, "No OVS_ACTION_ATTR_USERSPACE attribute");
> +    }
> +    return err;
> +}
> +
> +static int
> +parse_sample_action(struct tc_flower *flower, struct tc_action *tc_action,
> +                    const struct nlattr *sample_action,
> +                    const struct flow_tnl *tnl, uint32_t *group_id,
> +                    const ovs_u128 *ufid)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +    struct dpif_sflow_attr sflow_attr;
> +    const struct nlattr *nla;
> +    unsigned int left;
> +    int ret = EINVAL;
> +
> +    if (*group_id) {
> +        VLOG_ERR_RL(&error_rl,
> +                    "Only a single TC_SAMPLE action per flow is supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    memset(&sflow_attr, 0, sizeof sflow_attr);
> +    sflow_attr.ufid = *ufid;
> +    sflow_attr.action = sample_action;
> +
> +    if (flower->tunnel) {
> +        sflow_attr.tunnel = CONST_CAST(struct flow_tnl *, tnl);
> +    }
> +
> +    NL_NESTED_FOR_EACH_UNSAFE (nla, left, sample_action) {
> +        if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_ACTIONS) {
> +            ret = parse_sample_actions_attribute(nla, &sflow_attr);
> +        } else if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_PROBABILITY) {
> +            tc_action->type = TC_ACT_SAMPLE;
> +            tc_action->sample.rate = UINT32_MAX / nl_attr_get_u32(nla);
> +        } else {
> +            return EINVAL;
> +        }
> +    }
> +
> +    if (!tc_action->sample.rate || ret) {
> +        return EINVAL;
> +    }
> +
> +    *group_id = sgid_alloc_ctx(&sflow_attr);
> +    if (!*group_id) {
> +        VLOG_DBG_RL(&rl, "Failed allocating group id for sample action");
> +        return ENOENT;
> +    }
> +    tc_action->sample.group_id = *group_id;
> +    flower->action_count++;
> +
> +    return 0;
> +}
> +
> +static int
> +parse_userspace_action(struct tc_flower *flower, struct tc_action *tc_action,
> +                       const struct nlattr *userspace_action,
> +                       const struct flow_tnl *tnl, uint32_t *group_id,
> +                       const ovs_u128 *ufid)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +    struct dpif_sflow_attr sflow_attr;
> +    int err;
> +
> +    if (*group_id) {
> +        VLOG_ERR_RL(&error_rl, "Only a single TC_SAMPLE action per flow is"
> +                    " supported");

           VLOG_ERR_RL(&error_rl,
                       "Only a single TC_SAMPLE action per flow is supported");

> +        return EOPNOTSUPP;
> +    }
> +
> +    /* If sampling rate is 1, there is only a sFlow cookie inside of a
> +     * userspace action, but no sample attribute. That means we can
> +     * only offload userspace actions for sFlow.
> +     */
> +    memset(&sflow_attr, 0, sizeof sflow_attr);
> +    sflow_attr.ufid = *ufid;
> +    if (flower->tunnel) {
> +        sflow_attr.tunnel = CONST_CAST(struct flow_tnl *, tnl);
> +    }
> +    err = parse_userspace_attributes(userspace_action, &sflow_attr);
> +    if (err) {
> +        return err;
> +    }
> +    sflow_attr.action = userspace_action;
> +    *group_id = sgid_alloc_ctx(&sflow_attr);
> +    if (!*group_id) {
> +        VLOG_DBG_RL(&rl, "Failed allocating group id for sample action");
> +        return ENOENT;
> +    }
> +    tc_action->type = TC_ACT_SAMPLE;
> +    tc_action->sample.group_id = *group_id;
> +    tc_action->sample.rate = 1;
> +    flower->action_count++;
> +
> +    return 0;
> +}
> +
>  static int
>  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>                     struct nlattr *actions, size_t actions_len,
> @@ -1805,6 +1964,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>      const struct flow_tnl *tnl_mask = &mask->tunnel;
>      struct tc_action *action;
>      bool recirc_act = false;
> +    uint32_t sample_gid = 0;
>      uint32_t block_id = 0;
>      struct nlattr *nla;
>      struct tcf_id id;
> @@ -2057,7 +2217,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>      NL_ATTR_FOR_EACH(nla, left, actions, actions_len) {
>          if (flower.action_count >= TCA_ACT_MAX_NUM) {
>              VLOG_DBG_RL(&rl, "Can only support %d actions", TCA_ACT_MAX_NUM);
> -            return EOPNOTSUPP;
> +            err = EOPNOTSUPP;
> +            goto out;
>          }
>          action = &flower.actions[flower.action_count];
>          if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
> @@ -2067,7 +2228,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>
>              if (!outdev) {
>                  VLOG_DBG_RL(&rl, "Can't find netdev for output port %d", port);
> -                return ENODEV;
> +                err = ENODEV;
> +                goto out;
>              }
>              action->out.ifindex_out = netdev_get_ifindex(outdev);
>              action->out.ingress = is_internal_port(netdev_get_type(outdev));
> @@ -2105,7 +2267,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>
>              err = parse_put_flow_set_action(&flower, action, set, set_len);
>              if (err) {
> -                return err;
> +                goto out;
>              }
>              if (action->type == TC_ACT_ENCAP) {
>                  action->encap.tp_dst = info->tp_dst_port;
> @@ -2118,7 +2280,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>              err = parse_put_flow_set_masked_action(&flower, action, set,
>                                                     set_len, true);
>              if (err) {
> -                return err;
> +                goto out;
>              }
>          } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_CT) {
>              const struct nlattr *ct = nl_attr_get(nla);
> @@ -2130,7 +2292,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>
>              err = parse_put_flow_ct_action(&flower, action, ct, ct_len);
>              if (err) {
> -                return err;
> +                goto out;
>              }
>          } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_CT_CLEAR) {
>              action->type = TC_ACT_CT;
> @@ -2146,20 +2308,41 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>              action->chain = 0;  /* 0 is reserved and not used by recirc. */
>              flower.action_count++;
>          } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_SAMPLE) {
> -            struct dpif_sflow_attr sflow_attr;
> -
> -            memset(&sflow_attr, 0, sizeof sflow_attr);
> -            sgid_alloc_ctx(&sflow_attr);
> +            if (!dpif_offload_netlink_psample_supported()) {
> +                VLOG_DBG_RL(&rl, "Unsupported put action type: %d, psample is"
> +                            "not initialized successfully", nl_attr_type(nla));
> +                err = EOPNOTSUPP;
> +                goto out;
> +            }
> +            err = parse_sample_action(&flower, action, nla, tnl, &sample_gid,
> +                                      ufid);
> +            if (err) {
> +                goto out;
> +            }
> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
> +            if (!dpif_offload_netlink_psample_supported()) {
> +                VLOG_DBG_RL(&rl, "Unsupported put action type: %d, psample is"
> +                            "not initialized successfully", nl_attr_type(nla));
> +                err = EOPNOTSUPP;
> +                goto out;
> +            }
> +            err = parse_userspace_action(&flower, action, nla, tnl,
> +                                         &sample_gid, ufid);
> +            if (err) {
> +                goto out;
> +            }
>          } else {
>              VLOG_DBG_RL(&rl, "unsupported put action type: %d",
>                          nl_attr_type(nla));
> -            return EOPNOTSUPP;
> +            err = EOPNOTSUPP;
> +            goto out;
>          }
>      }
>
>      if ((chain || recirc_act) && !info->recirc_id_shared_with_tc) {
>          VLOG_ERR_RL(&error_rl, "flow_put: recirc_id sharing not supported");
> -        return EOPNOTSUPP;
> +        err = EOPNOTSUPP;
> +        goto out;
>      }
>
>      if (get_ufid_tc_mapping(ufid, &id) == 0) {
> @@ -2172,20 +2355,29 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>      prio = get_prio_for_tc_flower(&flower);
>      if (prio == 0) {
>          VLOG_ERR_RL(&rl, "couldn't get tc prio: %s", ovs_strerror(ENOSPC));
> -        return ENOSPC;
> +        err = ENOSPC;
> +        goto out;
>      }
>
>      flower.act_cookie.data = ufid;
>      flower.act_cookie.len = sizeof *ufid;
>
>      block_id = get_block_id_from_netdev(netdev);
> -    id = tc_make_tcf_id_chain(ifindex, block_id, chain, prio, hook);
> +    id = tc_make_tcf_id_all(ifindex, block_id, chain, prio, hook, sample_gid);
>      err = tc_replace_flower(&id, &flower);
> -    if (!err) {
> -        if (stats) {
> -            memset(stats, 0, sizeof *stats);
> -        }
> -        add_ufid_tc_mapping(netdev, ufid, &id);
> +    if (err) {
> +        goto out;
> +    }
> +
> +    if (stats) {
> +        memset(stats, 0, sizeof *stats);
> +    }
> +    add_ufid_tc_mapping(netdev, ufid, &id);
> +    return 0;
> +
> +out:
> +    if (sample_gid) {
> +        sgid_free(sample_gid);
>      }
>
>      return err;
> diff --git a/lib/tc.c b/lib/tc.c
> index 38a1dfc0e..a40954168 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -23,14 +23,15 @@
>  #include <linux/if_packet.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/tc_act/tc_csum.h>
> +#include <linux/tc_act/tc_ct.h>
>  #include <linux/tc_act/tc_gact.h>
>  #include <linux/tc_act/tc_mirred.h>
>  #include <linux/tc_act/tc_mpls.h>
>  #include <linux/tc_act/tc_pedit.h>
> +#include <linux/tc_act/tc_sample.h>
>  #include <linux/tc_act/tc_skbedit.h>
>  #include <linux/tc_act/tc_tunnel_key.h>
>  #include <linux/tc_act/tc_vlan.h>
> -#include <linux/tc_act/tc_ct.h>
>  #include <linux/gen_stats.h>
>  #include <net/if.h>
>  #include <unistd.h>
> @@ -1341,6 +1342,38 @@ nl_parse_act_gact(struct nlattr *options, struct tc_flower *flower)
>      return 0;
>  }
>
> +static const struct nl_policy sample_policy[] = {
> +    [TCA_SAMPLE_PARMS] = { .type = NL_A_UNSPEC,
> +                           .min_len = sizeof(struct tc_sample),
> +                           .optional = false, },
> +    [TCA_SAMPLE_PSAMPLE_GROUP] = { .type = NL_A_U32,
> +                                   .optional = false, },
> +    [TCA_SAMPLE_RATE] = { .type = NL_A_U32,
> +                          .optional = false, },
> +};
> +
> +static int
> +nl_parse_act_sample(struct nlattr *options, struct tc_flower *flower)
> +{
> +    struct nlattr *sample_attrs[ARRAY_SIZE(sample_policy)];
> +    struct tc_action *action;
> +
> +    if (!nl_parse_nested(options, sample_policy, sample_attrs,
> +                         ARRAY_SIZE(sample_policy))) {
> +        VLOG_ERR_RL(&error_rl, "Failed to parse sample action options");
> +        return EPROTO;
> +    }
> +
> +    action = &flower->actions[flower->action_count++];
> +    action->type = TC_ACT_SAMPLE;
> +    action->sample.group_id =
> +        nl_attr_get_u32(sample_attrs[TCA_SAMPLE_PSAMPLE_GROUP]);
> +    action->sample.rate =
> +        nl_attr_get_u32(sample_attrs[TCA_SAMPLE_RATE]);
> +
> +    return 0;
> +}
> +
>  static const struct nl_policy mirred_policy[] = {
>      [TCA_MIRRED_PARMS] = { .type = NL_A_UNSPEC,
>                             .min_len = sizeof(struct tc_mirred),
> @@ -1749,6 +1782,8 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower,
>          /* Added for TC rule only (not in OvS rule) so ignore. */
>      } else if (!strcmp(act_kind, "ct")) {
>          nl_parse_act_ct(act_options, flower);
> +    } else if (!strcmp(act_kind, "sample")) {
> +        nl_parse_act_sample(act_options, flower);
>      } else {
>          VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind);
>          err = EINVAL;
> @@ -2375,6 +2410,23 @@ nl_msg_put_act_mirred(struct ofpbuf *request, int ifindex, int action,
>      nl_msg_end_nested(request, offset);
>  }
>
> +static void
> +nl_msg_put_act_sample(struct ofpbuf *request, uint32_t rate, uint32_t group_id)
> +{
> +    size_t offset;
> +
> +    nl_msg_put_string(request, TCA_ACT_KIND, "sample");
> +    offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS | NLA_F_NESTED);
> +    {
> +        struct tc_sample parm = { .action = TC_ACT_PIPE };
> +
> +        nl_msg_put_unspec(request, TCA_SAMPLE_PARMS, &parm, sizeof parm);
> +        nl_msg_put_u32(request, TCA_SAMPLE_RATE, rate);
> +        nl_msg_put_u32(request, TCA_SAMPLE_PSAMPLE_GROUP, group_id);
> +    }
> +    nl_msg_end_nested(request, offset);
> +}
> +
>  static inline void
>  nl_msg_put_act_cookie(struct ofpbuf *request, struct tc_cookie *ck) {
>      if (ck->len) {
> @@ -2634,6 +2686,13 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>                  nl_msg_end_nested(request, act_offset);
>              }
>              break;
> +            case TC_ACT_SAMPLE: {
> +                act_offset = nl_msg_start_nested(request, act_index++);
> +                nl_msg_put_act_sample(request, action->sample.rate,
> +                                      action->sample.group_id);
> +                nl_msg_end_nested(request, act_offset);
> +            }
> +            break;
>              case TC_ACT_OUTPUT: {
>                  if (!released && flower->tunnel) {
>                      act_offset = nl_msg_start_nested(request, act_index++);
> diff --git a/lib/tc.h b/lib/tc.h
> index 2e4084f48..f764d7d1e 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -174,6 +174,7 @@ enum tc_action_type {
>      TC_ACT_MPLS_SET,
>      TC_ACT_GOTO,
>      TC_ACT_CT,
> +    TC_ACT_SAMPLE,
>  };
>
>  enum nat_type {
> @@ -256,6 +257,11 @@ struct tc_action {
>              bool force;
>              bool commit;
>          } ct;
> +
> +        struct {
> +            uint32_t rate;
> +            uint32_t group_id;
> +        } sample;
>       };
>
>       enum tc_action_type type;
> @@ -294,12 +300,14 @@ tc_make_tcf_id(int ifindex, uint32_t block_id, uint16_t prio,
>  }
>
>  static inline struct tcf_id
> -tc_make_tcf_id_chain(int ifindex, uint32_t block_id, uint32_t chain,
> -                     uint16_t prio, enum tc_qdisc_hook hook)
> +tc_make_tcf_id_all(int ifindex, uint32_t block_id, uint32_t chain,
> +                     uint16_t prio, enum tc_qdisc_hook hook,
> +                     uint32_t sample_group_id)
>  {
>      struct tcf_id id = tc_make_tcf_id(ifindex, block_id, prio, hook);
>
>      id.chain = chain;
> +    id.sample_group_id = sample_group_id;
>
>      return id;
>  }
> @@ -313,7 +321,8 @@ is_tcf_id_eq(struct tcf_id *id1, struct tcf_id *id2)
>             && id1->hook == id2->hook
>             && id1->block_id == id2->block_id
>             && id1->ifindex == id2->ifindex
> -           && id1->chain == id2->chain;
> +           && id1->chain == id2->chain
> +           && id1->sample_group_id == id2->sample_group_id;
>  }
>
>  enum tc_offload_policy {
> -- 
> 2.30.2



More information about the dev mailing list