diff mbox series

[ovs-dev,v9,11/11] netdev-offload-tc: Add offload support for sFlow

Message ID 20201215033812.145975-12-cmi@nvidia.com
State Changes Requested
Headers show
Series Add offload support for sFlow | expand

Commit Message

Chris Mi Dec. 15, 2020, 3:38 a.m. UTC 
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@nvidia.com>
Reviewed-by: Eli Britstein <elibr@nvidia.com>
---
 lib/netdev-offload-tc.c | 171 +++++++++++++++++++++++++++++++++++++---
 lib/tc.c                |  61 +++++++++++++-
 lib/tc.h                |   9 ++-
 3 files changed, 226 insertions(+), 15 deletions(-)

Comments

Eelco Chaudron Jan. 12, 2021, 8:27 p.m. UTC | #1 
On 15 Dec 2020, at 4:38, 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@nvidia.com>
> Reviewed-by: Eli Britstein <elibr@nvidia.com>
> ---
>  lib/netdev-offload-tc.c | 171 
> +++++++++++++++++++++++++++++++++++++---
>  lib/tc.c                |  61 +++++++++++++-
>  lib/tc.h                |   9 ++-
>  3 files changed, 226 insertions(+), 15 deletions(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index e5bf719a4..0da16feed 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -988,6 +988,18 @@ 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 gid_node *node;
> +
> +                node = gid_find(action->sample.action_group_id);
> +                if (!node) {
> +                    VLOG_ERR_RL(&error_rl, "gid node is NULL, gid: 
> %d",
> +                                action->sample.action_group_id);

Think this should be a warning as the gid can be deleted, but the tc dp 
might have not been fully synced.

> +                    return ENOENT;
> +                }
> +                nl_msg_put(buf, node->sflow.sflow, 
> node->sflow.sflow->nla_len);
> +            }
> +            break;
>              case TC_ACT_VLAN_POP: {
>                  nl_msg_put_flag(buf, OVS_ACTION_ATTR_POP_VLAN);
>              }
> @@ -1667,6 +1679,119 @@ 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 int
> +parse_sflow_cookie(const struct nlattr *actions,
> +                   struct dpif_sflow_attr *sflow_attr)

Your not parsing an sflow cookie, but the userspace_attributes to 
extract the userspace cookie, so the name should be 
parse_userspace_attributes()

> +{
> +    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 = CONST_CAST(void *, 
> nl_attr_get(nla));
> +                sflow_attr->userdata_len = nl_attr_get_size(nla);
> +                return 0;
> +            }				 +        }

I assume that any other OVS_USERSPACE_ATTR_* attribute present will not 
withhold offloading the sflow part.

> +    }
> +
> +    VLOG_DBG_RL(&rl, "%s: cannot offload userspace action other than 
> sFlow",
> +                __func__);
> +    return EOPNOTSUPP;
> +}
> +
> +static int
> +parse_sample_action_userspace(const struct nlattr *actions,
> +                              struct dpif_sflow_attr *sflow_attr)

Rename this to parse_sample_actions_attribute() as you are parsing a 
list of actions, not a single action! Also, the code should be like 
this, because if any other attributes exist you can not offload them. 
See also comments on earlier versions:

1708 static int
1709 parse_sample_actions_attribute(const struct nlattr *actions,
1710                                struct dpif_sflow_attr *sflow_attr)
1711 {
1712     const struct nlattr *nla;
1713     unsigned int left;
1714     int err = EINVAL;
1715
1716     NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
1717         if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
1718             err = parse_userspace_attributes(nla, sflow_attr);
1719         } else {
1720             /* We can't offload other nested actions */
1721             VLOG_DBG_RL(&error_rl,
1722                         "%s: can only offload 
OVS_ACTION_ATTR_USERSPACE attribute",
1723                         __func__);
1724             return EINVAL;
1725         }
1726     }
1727
1728     if (err) {
1729         VLOG_DBG_RL(&error_rl, "%s: no OVS_ACTION_ATTR_USERSPACE 
attribute",
1730                     __func__);
1731     }
1732     return err;
1733 }

> +{
> +    const struct nlattr *nla;
> +    unsigned int left;
> +
> +    NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
> +        if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
> +            return parse_sflow_cookie(nla, sflow_attr);
> +        }
> +    }
> +
> +    VLOG_ERR_RL(&error_rl, "%s: no OVS_ACTION_ATTR_USERSPACE 
> attribute",
> +                __func__);
> +    return EINVAL;
> +}
> +
> +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)
> +{
> +    struct dpif_sflow_attr sflow_attr;
> +    const struct nlattr *nla;
> +    unsigned int left;
> +    int ret = EINVAL;
> +
> +    memset(&sflow_attr, 0, sizeof sflow_attr);
> +    sflow_attr.sflow = 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_action_userspace(nla, &sflow_attr);

Make this parse_sample_actions_attribute(nla, &sflow_attr) as you are 
not parsing userspace, but a list of actions!

> +        } else if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_PROBABILITY) 
> {
> +            tc_action->type = TC_ACT_SAMPLE;
> +            tc_action->sample.action_rate = UINT32_MAX / 
> nl_attr_get_u32(nla);
> +        } else {
> +            return EINVAL;
> +        }
> +    }
> +
> +    if (!tc_action->sample.action_rate || ret) {
> +        return EINVAL;
> +    }
> +
> +    *group_id = gid_alloc_ctx(&sflow_attr);

Add error check, as I think gid_alloc_ctx() can fail.

> +    tc_action->sample.action_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 *sample_action,

sample_action points not to sample_action attribute, but the userspace 
action, so please name it userspace_action

> +                       const struct flow_tnl *tnl, uint32_t 
> *group_id)
> +{
> +    struct dpif_sflow_attr sflow_attr;
> +    int err;
> +
> +    /* 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);
> +    if (flower->tunnel) {
> +        sflow_attr.tunnel = CONST_CAST(struct flow_tnl *, tnl);
> +    }
> +    err = parse_sflow_cookie(sample_action, &sflow_attr);

parse_userspace_attributes()

> +    if (err) {
> +        return err;
> +    }
> +    sflow_attr.sflow = sample_action;
> +    *group_id = gid_alloc_ctx(&sflow_attr);

Add error check, as I think gid_alloc_ctx() can fail.

> +    tc_action->type = TC_ACT_SAMPLE;
> +    tc_action->sample.action_group_id = *group_id;
> +    tc_action->sample.action_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,
> @@ -1683,6 +1808,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct 
> match *match,
>      struct tc_action *action;
>      bool recirc_act = false;
>      uint32_t block_id = 0; +    uint32_t group_id = 0;

Maybe call it sample_gid ? so we know it's used for sampling?

>      struct nlattr *nla;
>      struct tcf_id id;
>      uint32_t chain;
> @@ -1972,7 +2098,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) {
> @@ -1982,7 +2109,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));
> @@ -2020,7 +2148,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;
> @@ -2033,7 +2161,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);
> @@ -2041,7 +2169,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;
> @@ -2057,20 +2185,27 @@ 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);
> -            gid_alloc_ctx(&sflow_attr);
> +            err = parse_sample_action(&flower, action, nla, tnl, 
> &group_id);
> +            if (err) {
> +                goto out;
> +            }

For this case and the case below we should probably error out if the 
group_id is already used?
 From the command line you can probably add a dpath rule with two sample 
actions.

> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
> +            err = parse_userspace_action(&flower, action, nla, tnl, 
> &group_id);
> +            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) {
> @@ -2082,20 +2217,30 @@ 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_chain(ifindex, block_id, chain, prio, hook, 
> group_id);
>      err = tc_replace_flower(&id, &flower);
>      if (!err) {
>          if (stats) {
>              memset(stats, 0, sizeof *stats);
>          }
>          add_ufid_tc_mapping(netdev, ufid, &id);
> +    } else {
> +        goto out;
> +    }
> +
> +    return 0;
> +
> +out:
> +    if (group_id) {
> +        gid_free(group_id);
>      }
>
>      return err;
> diff --git a/lib/tc.c b/lib/tc.c
> index c2de78bfe..dd67ad6b0 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>
> @@ -1291,6 +1292,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.action_group_id =
> +        nl_attr_get_u32(sample_attrs[TCA_SAMPLE_PSAMPLE_GROUP]);
> +    action->sample.action_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),
> @@ -1699,6 +1732,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;
> @@ -2294,6 +2329,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) {
> @@ -2553,6 +2605,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.action_rate,
> +                                      
> action->sample.action_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 cc2ad025d..143e225d1 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -171,6 +171,7 @@ enum tc_action_type {
>      TC_ACT_MPLS_SET,
>      TC_ACT_GOTO,
>      TC_ACT_CT,
> +    TC_ACT_SAMPLE,
>  };
>
>  enum nat_type {
> @@ -253,6 +254,11 @@ struct tc_action {
>              bool force;
>              bool commit;
>          } ct;
> +
> +        struct {
> +            uint32_t action_rate;
> +            uint32_t action_group_id;
> +        } sample;
>       };
>
>       enum tc_action_type type;
> @@ -292,11 +298,12 @@ 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)
> +                     uint16_t prio, enum tc_qdisc_hook hook, uint32_t 
> group_id)
>  {
>      struct tcf_id id = tc_make_tcf_id(ifindex, block_id, prio, hook);
>
>      id.chain = chain;
> +    id.sflow_group_id = group_id;

I don't think overloading the tcf_id with the group ID for cleanup makes 
sense, or maybe it does, but then we should make it clear, that "struct 
tcf_id " is now also used to track flow state.
If we do this, rather than keep adding parameters to tc_make_tcf_id(),
we might want to set the optional fields in the structure ptr returned 
by it.

Also, I think the group_id should be called sample_group_id, as we might 
use this sample for other use cases (read dec_ttl). Talking about 
multiple use cases, how to handle multiple sample_group_ids in a single 
rule with this API?

>      return id;
>  }
> -- 
> 2.26.2

Guess this concludes my v9 review…
Chris Mi Jan. 22, 2021, 5:15 a.m. UTC | #2 
On 1/13/2021 4:27 AM, Eelco Chaudron wrote:
>
> On 15 Dec 2020, at 4:38, 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@nvidia.com>
>     Reviewed-by: Eli Britstein <elibr@nvidia.com>
>     ---
>     lib/netdev-offload-tc.c | 171
>     +++++++++++++++++++++++++++++++++++++---
>     lib/tc.c | 61 +++++++++++++-
>     lib/tc.h | 9 ++-
>     3 files changed, 226 insertions(+), 15 deletions(-)
>     diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>     index e5bf719a4..0da16feed 100644
>     --- a/lib/netdev-offload-tc.c
>     +++ b/lib/netdev-offload-tc.c
>     @@ -988,6 +988,18 @@ 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 gid_node *node;
>     +
>     + node = gid_find(action->sample.action_group_id);
>     + if (!node) {
>     + VLOG_ERR_RL(&error_rl, "gid node is NULL, gid: %d",
>     + action->sample.action_group_id);
>
> Think this should be a warning as the gid can be deleted, but the tc 
> dp might have not been fully synced.
>
Done.
>
>     + return ENOENT;
>     + }
>     + nl_msg_put(buf, node->sflow.sflow, node->sflow.sflow->nla_len);
>     + }
>     + break;
>     case TC_ACT_VLAN_POP: {
>     nl_msg_put_flag(buf, OVS_ACTION_ATTR_POP_VLAN);
>     }
>     @@ -1667,6 +1679,119 @@ 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 int
>     +parse_sflow_cookie(const struct nlattr *actions,
>     + struct dpif_sflow_attr *sflow_attr)
>
> Your not parsing an sflow cookie, but the userspace_attributes to 
> extract the userspace cookie, so the name should be 
> parse_userspace_attributes()
>
Done.
>
>     +{
>     + 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 = CONST_CAST(void *, nl_attr_get(nla));
>     + sflow_attr->userdata_len = nl_attr_get_size(nla);
>     + return 0;
>     + } + }
>
> I assume that any other OVS_USERSPACE_ATTR_* attribute present will 
> not withhold offloading the sflow part.
>
>     + }
>     +
>     + VLOG_DBG_RL(&rl, "%s: cannot offload userspace action other than
>     sFlow",
>     + __func__);
>     + return EOPNOTSUPP;
>     +}
>     +
>     +static int
>     +parse_sample_action_userspace(const struct nlattr *actions,
>     + struct dpif_sflow_attr *sflow_attr)
>
> Rename this to parse_sample_actions_attribute() as you are parsing a 
> list of actions, not a single action! Also, the code should be like 
> this, because if any other attributes exist you can not offload them. 
> See also comments on earlier versions:
>
> 1708 static int
> 1709 parse_sample_actions_attribute(const struct nlattr *actions,
> 1710 struct dpif_sflow_attr *sflow_attr)
> 1711 {
> 1712 const struct nlattr /nla;
> 1713 unsigned int left;
> 1714 int err = EINVAL;
> 1715
> 1716 NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
> 1717 if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
> 1718 err = parse_userspace_attributes(nla, sflow_attr);
> 1719 } else {
> 1720 // We can't offload other nested actions */
> 1721 VLOG_DBG_RL(&error_rl,
> 1722 "%s: can only offload OVS_ACTION_ATTR_USERSPACE attribute",
> 1723 *func*);
> 1724 return EINVAL;
> 1725 }
> 1726 }
> 1727
> 1728 if (err) {
> 1729 VLOG_DBG_RL(&error_rl, "%s: no OVS_ACTION_ATTR_USERSPACE attribute",
> 1730 *func*);
> 1731 }
> 1732 return err;
> 1733 }
>
Done.
>
>     +{
>     + const struct nlattr *nla;
>     + unsigned int left;
>     +
>     + NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
>     + if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
>     + return parse_sflow_cookie(nla, sflow_attr);
>     + }
>     + }
>     +
>     + VLOG_ERR_RL(&error_rl, "%s: no OVS_ACTION_ATTR_USERSPACE
>     attribute",
>     + __func__);
>     + return EINVAL;
>     +}
>     +
>     +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)
>     +{
>     + struct dpif_sflow_attr sflow_attr;
>     + const struct nlattr *nla;
>     + unsigned int left;
>     + int ret = EINVAL;
>     +
>     + memset(&sflow_attr, 0, sizeof sflow_attr);
>     + sflow_attr.sflow = 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_action_userspace(nla, &sflow_attr);
>
> Make this parse_sample_actions_attribute(nla, &sflow_attr) as you are 
> not parsing userspace, but a list of actions!
>
Done.
>
>     + } else if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_PROBABILITY) {
>     + tc_action->type = TC_ACT_SAMPLE;
>     + tc_action->sample.action_rate = UINT32_MAX / nl_attr_get_u32(nla);
>     + } else {
>     + return EINVAL;
>     + }
>     + }
>     +
>     + if (!tc_action->sample.action_rate || ret) {
>     + return EINVAL;
>     + }
>     +
>     + *group_id = gid_alloc_ctx(&sflow_attr);
>
> Add error check, as I think gid_alloc_ctx() can fail.
>
Done.
>
>     + tc_action->sample.action_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 *sample_action,
>
> sample_action points not to sample_action attribute, but the userspace 
> action, so please name it userspace_action
>
Done.
>
>     + const struct flow_tnl *tnl, uint32_t *group_id)
>     +{
>     + struct dpif_sflow_attr sflow_attr;
>     + int err;
>     +
>     + /* 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);
>     + if (flower->tunnel) {
>     + sflow_attr.tunnel = CONST_CAST(struct flow_tnl *, tnl);
>     + }
>     + err = parse_sflow_cookie(sample_action, &sflow_attr);
>
> parse_userspace_attributes()
>
Done.
>
>     + if (err) {
>     + return err;
>     + }
>     + sflow_attr.sflow = sample_action;
>     + *group_id = gid_alloc_ctx(&sflow_attr);
>
> Add error check, as I think gid_alloc_ctx() can fail.
>
Done.
>
>     + tc_action->type = TC_ACT_SAMPLE;
>     + tc_action->sample.action_group_id = *group_id;
>     + tc_action->sample.action_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,
>     @@ -1683,6 +1808,7 @@ netdev_tc_flow_put(struct netdev *netdev,
>     struct match *match,
>     struct tc_action *action;
>     bool recirc_act = false;
>     uint32_t block_id = 0; + uint32_t group_id = 0;
>
> Maybe call it sample_gid ? so we know it's used for sampling?
>
Done.
>
>     struct nlattr *nla;
>     struct tcf_id id;
>     uint32_t chain;
>     @@ -1972,7 +2098,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) {
>     @@ -1982,7 +2109,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));
>     @@ -2020,7 +2148,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;
>     @@ -2033,7 +2161,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);
>     @@ -2041,7 +2169,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;
>     @@ -2057,20 +2185,27 @@ 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);
>     - gid_alloc_ctx(&sflow_attr);
>     + err = parse_sample_action(&flower, action, nla, tnl, &group_id);
>     + if (err) {
>     + goto out;
>     + }
>
> For this case and the case below we should probably error out if the 
> group_id is already used?
>
I don't think the used group_id can be allocated.
>
> From the command line you can probably add a dpath rule with two 
> sample actions.
>
Yes, from tc command line, we can add two sample actions. But I believe 
ovs can't do it.
Maybe tc should prevent it if user specifies two sample actions. It 
doesn't help if we check it here.
>
>     + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
>     + err = parse_userspace_action(&flower, action, nla, tnl, &group_id);
>     + 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) {
>     @@ -2082,20 +2217,30 @@ 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_chain(ifindex, block_id, chain, prio, hook,
>     group_id);
>     err = tc_replace_flower(&id, &flower);
>     if (!err) {
>     if (stats) {
>     memset(stats, 0, sizeof *stats);
>     }
>     add_ufid_tc_mapping(netdev, ufid, &id);
>     + } else {
>     + goto out;
>     + }
>     +
>     + return 0;
>     +
>     +out:
>     + if (group_id) {
>     + gid_free(group_id);
>     }
>     return err;
>     diff --git a/lib/tc.c b/lib/tc.c
>     index c2de78bfe..dd67ad6b0 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>
>     @@ -1291,6 +1292,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.action_group_id =
>     + nl_attr_get_u32(sample_attrs[TCA_SAMPLE_PSAMPLE_GROUP]);
>     + action->sample.action_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),
>     @@ -1699,6 +1732,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;
>     @@ -2294,6 +2329,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) {
>     @@ -2553,6 +2605,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.action_rate,
>     + action->sample.action_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 cc2ad025d..143e225d1 100644
>     --- a/lib/tc.h
>     +++ b/lib/tc.h
>     @@ -171,6 +171,7 @@ enum tc_action_type {
>     TC_ACT_MPLS_SET,
>     TC_ACT_GOTO,
>     TC_ACT_CT,
>     + TC_ACT_SAMPLE,
>     };
>     enum nat_type {
>     @@ -253,6 +254,11 @@ struct tc_action {
>     bool force;
>     bool commit;
>     } ct;
>     +
>     + struct {
>     + uint32_t action_rate;
>     + uint32_t action_group_id;
>     + } sample;
>     };
>     enum tc_action_type type;
>     @@ -292,11 +298,12 @@ 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)
>     + uint16_t prio, enum tc_qdisc_hook hook, uint32_t group_id)
>     {
>     struct tcf_id id = tc_make_tcf_id(ifindex, block_id, prio, hook);
>     id.chain = chain;
>     + id.sflow_group_id = group_id;
>
> I don't think overloading the tcf_id with the group ID for cleanup 
> makes sense, or maybe it does, but then we should make it clear, that 
> "struct tcf_id " is now also used to track flow state.
> If we do this, rather than keep adding parameters to tc_make_tcf_id(),
> we might want to set the optional fields in the structure ptr returned 
> by it.
>
OK, I create a new function del_filter_and_sgid() to make it clear.
>
> Also, I think the group_id should be called sample_group_id, as we 
> might use this sample for other use cases (read dec_ttl). Talking 
> about multiple use cases, how to handle multiple sample_group_ids in a 
> single rule with this API?
>
OK, I renamed it to sample_group_id. I think struct tcf_id is used to 
hold all ids related to tc. I think
we can add another member for it if using sample for other use cases.
>
>     return id;
>     }
>     -- 
>     2.26.2
>
> Guess this concludes my v9 review…
>
Thanks for the review. I'll send v10.

Regards,
Chris
Eelco Chaudron Jan. 25, 2021, 9:22 a.m. UTC | #3 
On 22 Jan 2021, at 6:15, Chris Mi wrote:

> On 1/13/2021 4:27 AM, Eelco Chaudron wrote:
>>
>> On 15 Dec 2020, at 4:38, Chris Mi wrote:

<SNIP>

>>     struct nlattr *nla;
>>     struct tcf_id id;
>>     uint32_t chain;
>>     @@ -1972,7 +2098,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) {
>>     @@ -1982,7 +2109,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));
>>     @@ -2020,7 +2148,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;
>>     @@ -2033,7 +2161,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);
>>     @@ -2041,7 +2169,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;
>>     @@ -2057,20 +2185,27 @@ 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);
>>     - gid_alloc_ctx(&sflow_attr);
>>     + err = parse_sample_action(&flower, action, nla, tnl, 
>> &group_id);
>>     + if (err) {
>>     + goto out;
>>     + }
>>
>> For this case and the case below we should probably error out if the 
>> group_id is already used?
>>
> I don't think the used group_id can be allocated.
>>
>> From the command line you can probably add a dpath rule with two 
>> sample actions.
>>
> Yes, from tc command line, we can add two sample actions. But I 
> believe ovs can't do it.
> Maybe tc should prevent it if user specifies two sample actions. It 
> doesn't help if we check it here.

I think this is the perfect spot to add the check, it’s where it’s 
done for other TC related actions.
So you could just add:

	if (group_id) {
          VLOG_ERR_RL(&error_rl, “Only a single TC_SAMPLE action per 
flow is supported” );
          err = EOPNOTSUPP;
          goto out;
     }	

Or you add it to both parse_sample_action() and 
parse_userspace_action().
>>
>>     + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
>>     + err = parse_userspace_action(&flower, action, nla, tnl, 
>> &group_id);
>>     + if (err) {
>>     + goto out;
>>     + }
>>     } else {
>>     VLOG_DBG_RL(&rl, "unsupported put action type: %d",
>>     nl_attr_type(nla));
>>     - return EOPNOTSUPP;
>>     + err = EOPNOTSUPP;
>>     + goto out;
>>     }
>>     }

<SNIP>
diff mbox series

Patch

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index e5bf719a4..0da16feed 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -988,6 +988,18 @@  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 gid_node *node;
+
+                node = gid_find(action->sample.action_group_id);
+                if (!node) {
+                    VLOG_ERR_RL(&error_rl, "gid node is NULL, gid: %d",
+                                action->sample.action_group_id);
+                    return ENOENT;
+                }
+                nl_msg_put(buf, node->sflow.sflow, node->sflow.sflow->nla_len);
+            }
+            break;
             case TC_ACT_VLAN_POP: {
                 nl_msg_put_flag(buf, OVS_ACTION_ATTR_POP_VLAN);
             }
@@ -1667,6 +1679,119 @@  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 int
+parse_sflow_cookie(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 = CONST_CAST(void *, nl_attr_get(nla));
+                sflow_attr->userdata_len = nl_attr_get_size(nla);
+                return 0;
+            }
+        }
+    }
+
+    VLOG_DBG_RL(&rl, "%s: cannot offload userspace action other than sFlow",
+                __func__);
+    return EOPNOTSUPP;
+}
+
+static int
+parse_sample_action_userspace(const struct nlattr *actions,
+                              struct dpif_sflow_attr *sflow_attr)
+{
+    const struct nlattr *nla;
+    unsigned int left;
+
+    NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
+        if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
+            return parse_sflow_cookie(nla, sflow_attr);
+        }
+    }
+
+    VLOG_ERR_RL(&error_rl, "%s: no OVS_ACTION_ATTR_USERSPACE attribute",
+                __func__);
+    return EINVAL;
+}
+
+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)
+{
+    struct dpif_sflow_attr sflow_attr;
+    const struct nlattr *nla;
+    unsigned int left;
+    int ret = EINVAL;
+
+    memset(&sflow_attr, 0, sizeof sflow_attr);
+    sflow_attr.sflow = 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_action_userspace(nla, &sflow_attr);
+        } else if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_PROBABILITY) {
+            tc_action->type = TC_ACT_SAMPLE;
+            tc_action->sample.action_rate = UINT32_MAX / nl_attr_get_u32(nla);
+        } else {
+            return EINVAL;
+        }
+    }
+
+    if (!tc_action->sample.action_rate || ret) {
+        return EINVAL;
+    }
+
+    *group_id = gid_alloc_ctx(&sflow_attr);
+    tc_action->sample.action_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 *sample_action,
+                       const struct flow_tnl *tnl, uint32_t *group_id)
+{
+    struct dpif_sflow_attr sflow_attr;
+    int err;
+
+    /* 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);
+    if (flower->tunnel) {
+        sflow_attr.tunnel = CONST_CAST(struct flow_tnl *, tnl);
+    }
+    err = parse_sflow_cookie(sample_action, &sflow_attr);
+    if (err) {
+        return err;
+    }
+    sflow_attr.sflow = sample_action;
+    *group_id = gid_alloc_ctx(&sflow_attr);
+    tc_action->type = TC_ACT_SAMPLE;
+    tc_action->sample.action_group_id = *group_id;
+    tc_action->sample.action_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,
@@ -1683,6 +1808,7 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
     struct tc_action *action;
     bool recirc_act = false;
     uint32_t block_id = 0;
+    uint32_t group_id = 0;
     struct nlattr *nla;
     struct tcf_id id;
     uint32_t chain;
@@ -1972,7 +2098,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) {
@@ -1982,7 +2109,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));
@@ -2020,7 +2148,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;
@@ -2033,7 +2161,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);
@@ -2041,7 +2169,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;
@@ -2057,20 +2185,27 @@  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);
-            gid_alloc_ctx(&sflow_attr);
+            err = parse_sample_action(&flower, action, nla, tnl, &group_id);
+            if (err) {
+                goto out;
+            }
+        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
+            err = parse_userspace_action(&flower, action, nla, tnl, &group_id);
+            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) {
@@ -2082,20 +2217,30 @@  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_chain(ifindex, block_id, chain, prio, hook, group_id);
     err = tc_replace_flower(&id, &flower);
     if (!err) {
         if (stats) {
             memset(stats, 0, sizeof *stats);
         }
         add_ufid_tc_mapping(netdev, ufid, &id);
+    } else {
+        goto out;
+    }
+
+    return 0;
+
+out:
+    if (group_id) {
+        gid_free(group_id);
     }
 
     return err;
diff --git a/lib/tc.c b/lib/tc.c
index c2de78bfe..dd67ad6b0 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>
@@ -1291,6 +1292,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.action_group_id =
+        nl_attr_get_u32(sample_attrs[TCA_SAMPLE_PSAMPLE_GROUP]);
+    action->sample.action_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),
@@ -1699,6 +1732,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;
@@ -2294,6 +2329,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) {
@@ -2553,6 +2605,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.action_rate,
+                                      action->sample.action_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 cc2ad025d..143e225d1 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -171,6 +171,7 @@  enum tc_action_type {
     TC_ACT_MPLS_SET,
     TC_ACT_GOTO,
     TC_ACT_CT,
+    TC_ACT_SAMPLE,
 };
 
 enum nat_type {
@@ -253,6 +254,11 @@  struct tc_action {
             bool force;
             bool commit;
         } ct;
+
+        struct {
+            uint32_t action_rate;
+            uint32_t action_group_id;
+        } sample;
      };
 
      enum tc_action_type type;
@@ -292,11 +298,12 @@  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)
+                     uint16_t prio, enum tc_qdisc_hook hook, uint32_t group_id)
 {
     struct tcf_id id = tc_make_tcf_id(ifindex, block_id, prio, hook);
 
     id.chain = chain;
+    id.sflow_group_id = group_id;
 
     return id;
 }