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

[Chris Mi via dev]

<SNIP>

+    if (err) {
+        VLOG_ERR_RL(&error_rl, "No OVS_ACTION_ATTR_USERSPACE attribute");
+    }
+    return err;
+}
+
+static void
+offload_sample_init(struct offload_sample *sample,
+                    const struct nlattr *next_actions,
+                    size_t next_actions_len,
+                    struct tc_flower *flower,
+                    const struct flow_tnl *tnl)
+{
+    memset(sample, 0, sizeof *sample);
+    if (flower->tunnel) {
+        sample->tunnel = CONST_CAST(struct flow_tnl *, tnl);
+    }
+    sample->userspace_actions = CONST_CAST(struct nlattr *, next_actions);
+    sample->userspace_actions_len = next_actions_len;

Here we should initialize the userspace_actions, to make it work :)
This was previously in the clone action in patch 3:

@@ -2148,8 +2148,11 @@ offload_sample_init(struct offload_sample *sample,
      if (flower->tunnel) {
          sample->tunnel = CONST_CAST(struct flow_tnl *, tnl);
      }
-    sample->userspace_actions = CONST_CAST(struct nlattr *, next_actions);
-    sample->userspace_actions_len = next_actions_len;
+
+    sample->userspace_actions = xmalloc(next_actions_len + NLA_HDRLEN);
+    nullable_memcpy((char *) sample->userspace_actions + NLA_HDRLEN,
+                    next_actions, next_actions_len);
+    sample->userspace_actions->nla_len = next_actions_len + NLA_HDRLEN;
      sample->ifindex = flower->ifindex;

One remaining question is, should we set the nla_type here to 
OVS_ACTION_ATTR_SAMPLE or OVS_ACTION_ATTR_USERSPACE as this what they are, or 
is it safe to not set this as the upcall handling will ignore the type?

According to function dpif_get_actions(), nla_type is ignored:

    if (upcall->actions) {
        /* Actions were passed up from datapath. */
        *actions = nl_attr_get(upcall->actions);
        actions_len = nl_attr_get_size(upcall->actions);
    }

+    sample->ifindex = flower->ifindex;
+}
+
+static int
+parse_sample_action(struct tc_flower *flower, struct tc_action *tc_action,
+                    const struct nlattr *next_actions, size_t next_actions_len,
+                    const struct nlattr *sample_action,
+                    const struct flow_tnl *tnl)
+{
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
+    struct offload_sample sample;
+    const struct nlattr *nla;
+    unsigned int left;
+    uint32_t sgid;
+    int err;

As you change the function, you should set err to EINVAL here :)

+
+    offload_sample_init(&sample, next_actions, next_actions_len, flower, tnl);
+    sample.action = CONST_CAST(struct nlattr *, sample_action);
+
+    err = EINVAL;

Remove this here, as you no longer override it with offload_sample_init()

+    NL_NESTED_FOR_EACH_UNSAFE (nla, left, sample_action) {
+        if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_ACTIONS) {
+            err = parse_sample_actions_attribute(nla, &sample);
+            if (err) {
+                break;
+            }
+        } 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);
+            if (!tc_action->sample.rate) {
+                break;
+            }
+        } else {

Here you removed err = EINVAL; but it should be here if you want to error out 
on unsupported attributes.

I re-wrote the function a bit, to clean it up, and made it look a bit more like 
the v18 version. I removed the conditional breaks in the if() branches, as it 
made it looks like we check for errors in these branch, but in fact the checks 
are there to verify the attributes where present.
Also made sure we do not trash the tc_action structure on failure.

static int
parse_sample_action(struct tc_flower *flower, struct tc_action *tc_action,
                     const struct nlattr *next_actions, size_t next_actions_len,
                     const struct nlattr *sample_action,
                     const struct flow_tnl *tnl)
{
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
     struct offload_sample sample;
     const struct nlattr *nla;
     unsigned int left;
     uint32_t rate = 0;
     int ret = EINVAL;
     uint32_t sgid;

     offload_sample_init(&sample, next_actions, next_actions_len, flower, tnl);
     sample.action = CONST_CAST(struct nlattr *, sample_action);

     NL_NESTED_FOR_EACH_UNSAFE (nla, left, sample_action) {
         if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_ACTIONS) {
             ret = parse_sample_actions_attribute(nla, &sample);
         } else if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_PROBABILITY) {
             rate = UINT32_MAX / nl_attr_get_u32(nla);
         } else {
             return EINVAL;
         }
     }

     /* This check makes sure both attributes above were present and valid. */
     if (!rate || ret) {
         return EINVAL;
     }

     sgid = sgid_alloc(&sample);
     if (!sgid) {
         VLOG_DBG_RL(&rl, "Failed allocating group id for sample action");
         return ENOENT;
     }

     tc_action->type = TC_ACT_SAMPLE;
     tc_action->sample.rate = rate;
     tc_action->sample.group_id = sgid;
     flower->action_count++;

     return 0;
}

+            break;
+        }
+    }
+
+    if (!tc_action->sample.rate || err) {
+        return EINVAL;
+    }
+
+    sgid = sgid_alloc(&sample);
+    if (!sgid) {
+        VLOG_DBG_RL(&rl, "Failed allocating group id for sample action");
+        return ENOENT;
+    }
+    tc_action->sample.group_id = sgid;
+    flower->action_count++;
+
+    return err;
+}
+
+static int
+parse_userspace_action(struct tc_flower *flower, struct tc_action *tc_action,
+                       const struct nlattr *next_actions,
+                       size_t next_actions_len,
+                       const struct nlattr *userspace_action,
+                       const struct flow_tnl *tnl)
+{
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
+    struct offload_sample sample;
+    uint32_t sgid;
+    int err;
+
+    offload_sample_init(&sample, next_actions, next_actions_len, flower, tnl);
+
+    /* 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. */
+    err = parse_userspace_attributes(userspace_action, &sample);
+    if (err) {
+        return err;
+    }
+    sample.action = (struct nlattr *) userspace_action;
+    sgid = sgid_alloc(&sample);
+    if (!sgid) {
+        VLOG_DBG_RL(&rl, "Failed allocating group id for sample action");
+        return ENOENT;
+    }
+    tc_action->type = TC_ACT_SAMPLE;
+    tc_action->sample.group_id = sgid;
+    tc_action->sample.rate = 1;
+    flower->action_count++;
+
+    return err;

Here we can just return 0.

+}

  static int
  parse_check_pkt_len_action(struct netdev *netdev, struct tc_flower *flower,
-                           struct offload_info *info, struct tc_action *action,
-                           const struct nlattr *nla, bool last_action,
+                           struct offload_info *info,
+                           const struct flow_tnl *tnl,
+                           struct tc_action *action, const struct nlattr *nla,
+                           bool last_action,
                             struct tc_action **need_jump_update,
                             bool *recirc_act)
  {
@@ -2070,7 +2283,7 @@ parse_check_pkt_len_action(struct netdev *netdev, struct 
tc_flower *flower,
       * NOTE: The last_action parameter means that there are no more actions
       *       after the if () then ... else () case. */
      nl_actions = a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER];
-    err = netdev_tc_parse_nl_actions(netdev, flower, info,
+    err = netdev_tc_parse_nl_actions(netdev, flower, info, tnl,
                                       nl_attr_get(nl_actions),
                                       nl_attr_get_size(nl_actions),
                                       recirc_act, !last_action,
@@ -2086,7 +2299,7 @@ parse_check_pkt_len_action(struct netdev *netdev, struct 
tc_flower *flower,

      /* Parse and add the less than action(s). */
      nl_actions = a[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
-    err = netdev_tc_parse_nl_actions(netdev, flower, info,
+    err = netdev_tc_parse_nl_actions(netdev, flower, info, tnl,
                                       nl_attr_get(nl_actions),
                                       nl_attr_get_size(nl_actions),
                                       recirc_act, !last_action,
@@ -2139,6 +2352,7 @@ parse_check_pkt_len_action(struct netdev *netdev, struct 
tc_flower *flower,
  static int
  netdev_tc_parse_nl_actions(struct netdev *netdev, struct tc_flower *flower,
                             struct offload_info *info,
+                           const struct flow_tnl *tnl,
                             const struct nlattr *actions, size_t actions_len,
                             bool *recirc_act, bool more_actions,
                             struct tc_action **need_jump_update)
@@ -2268,7 +2482,8 @@ netdev_tc_parse_nl_actions(struct netdev *netdev, struct 
tc_flower *flower,
              action->police.index = police_index;
              flower->action_count++;
          } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_CHECK_PKT_LEN) {
-            err = parse_check_pkt_len_action(netdev, flower, info, action, nla,
+            err = parse_check_pkt_len_action(netdev, flower, info, tnl,
+                                             action, nla,
                                               nl_attr_len_pad(nla,
                                                               left) >= left
                                               && !more_actions,
@@ -2277,14 +2492,28 @@ netdev_tc_parse_nl_actions(struct netdev *netdev, 
struct tc_flower *flower,
              if (err) {
                  return err;
              }
-        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_SAMPLE) {
-            struct offload_sample sample;
-            uint32_t sgid;
-
-            memset(&sample, 0, sizeof sample);
-            sgid = sgid_alloc(&sample);
-            sgid_free(sgid);
-            return EOPNOTSUPP;
+        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_SAMPLE ||
+                   nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
+            if (!psample_supported()) {
+                VLOG_DBG_RL(&rl, "Unsupported put action type: %d, psample is "
+                            "not initialized successfully", nl_attr_type(nla));
+                return EOPNOTSUPP;
+            }
+            if (get_flower_sgid_count(flower) >= MAX_TC_SAMPLES_PER_FLOW) {
+                VLOG_ERR_RL(&error_rl, "Only %u TC_SAMPLE action per "
+                            "flow is supported", MAX_TC_SAMPLES_PER_FLOW);
+                return EOPNOTSUPP;
+            }
+            if (nl_attr_type(nla) == OVS_ACTION_ATTR_SAMPLE) {
+                err = parse_sample_action(flower, action, actions, actions_len,
+                                          nla, tnl);
+            } else {
+                err = parse_userspace_action(flower, action, actions,
+                                             actions_len, nla, tnl);
+            }
+            if (err) {
+                return err;
+            }
          } else {
              VLOG_DBG_RL(&rl, "unsupported put action type: %d",
                          nl_attr_type(nla));
@@ -2324,6 +2553,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match 
*match,
      }

      memset(&flower, 0, sizeof flower);
+    flower.ifindex = ifindex;

      chain = key->recirc_id;
      mask->recirc_id = 0;
@@ -2589,16 +2819,17 @@ netdev_tc_flow_put(struct netdev *netdev, struct match 
*match,
      }

      /* Parse all (nested) actions. */
-    err = netdev_tc_parse_nl_actions(netdev, &flower, info,
+    err = netdev_tc_parse_nl_actions(netdev, &flower, info, tnl,
                                       actions, actions_len, &recirc_act,
                                       false, NULL);
      if (err) {
-        return err;
+        goto error_out;
      }

      if ((chain || recirc_act) && !info->recirc_id_shared_with_tc) {
          VLOG_DBG_RL(&rl, "flow_put: recirc_id sharing not supported");
-        return EOPNOTSUPP;
+        err = EOPNOTSUPP;
+        goto error_out;
      }

      memset(&adjust_stats, 0, sizeof adjust_stats);
@@ -2612,7 +2843,8 @@ 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 error_out;
      }

      flower.act_cookie.data = ufid;
@@ -2621,14 +2853,20 @@ netdev_tc_flow_put(struct netdev *netdev, struct match 
*match,
      block_id = get_block_id_from_netdev(netdev);
      id = tc_make_tcf_id_chain(ifindex, block_id, chain, prio, hook);
      err = tc_replace_flower(&id, &flower);
-    if (!err) {
-        if (stats) {
-            memset(stats, 0, sizeof *stats);
-            netdev_tc_adjust_stats(stats, &adjust_stats);
-        }
-        add_ufid_tc_mapping(netdev, ufid, &id, &adjust_stats);
+    if (err) {
+        goto error_out;
      }

+    if (stats) {
+        memset(stats, 0, sizeof *stats);
+        netdev_tc_adjust_stats(stats, &adjust_stats);
+    }
+
+    add_ufid_tc_mapping(netdev, ufid, &id, &adjust_stats, &flower);
+    return 0;
+
+error_out:
+    free_all_flower_sgids(&flower);
      return err;
  }

diff --git a/lib/tc.c b/lib/tc.c
index 5c32c6f97..28ca6325a 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>
@@ -1484,6 +1485,38 @@ nl_parse_act_police(const 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),
@@ -1999,6 +2032,8 @@ nl_parse_single_action(struct nlattr *action, struct 
tc_flower *flower,
          nl_parse_act_ct(act_options, flower);
      } else if (!strcmp(act_kind, "police")) {
          nl_parse_act_police(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;
@@ -2791,6 +2826,24 @@ 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,
+                      uint32_t action_pc)
+{
+    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 = action_pc };
+
+        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) {
@@ -3103,6 +3156,7 @@ get_action_index_for_tc_actions(struct tc_flower *flower, 
uint16_t act_index,
          case TC_ACT_MPLS_SET:
          case TC_ACT_GOTO:
          case TC_ACT_CT:
+        case TC_ACT_SAMPLE:
              /* Increase act_index by one if we are sure this type of action
               * will only add one tc action in the kernel. */
              act_index++;
@@ -3310,6 +3364,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, action_pc);
+                nl_msg_end_nested(request, act_offset);
+            }
+            break;
              case TC_ACT_OUTPUT: {
                  if (!released && flower->tunnel) {
                      nl_msg_put_flower_acts_release(request, act_index++);
diff --git a/lib/tc.h b/lib/tc.h
index cdd3b4f60..5f6e15d5c 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -192,6 +192,7 @@ enum tc_action_type {
      TC_ACT_CT,
      TC_ACT_POLICE,
      TC_ACT_POLICE_MTU,
+    TC_ACT_SAMPLE,
  };

  enum nat_type {
@@ -283,6 +284,10 @@ struct tc_action {
              uint32_t result_jump;
              uint16_t mtu;
          } police;
+        struct {
+            uint32_t rate;
+            uint32_t group_id;
+        } sample;
      };

      enum tc_action_type type;
@@ -380,6 +385,7 @@ struct tc_flower {
      enum tc_offloaded_state offloaded_state;
      /* Used to force skip_hw when probing tc features. */
      enum tc_offload_policy tc_policy;
+    uint16_t ifindex;
  };

  int tc_replace_flower(struct tcf_id *id, struct tc_flower *flower);
--
2.26.3

Here is a full diff of the suggested changes, if you sent a v28 with only these 
changes I can ack the patch (need an answer on one question):

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 14a4cb393..71ec8ef1f 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -2146,8 +2146,11 @@ offload_sample_init(struct offload_sample *sample,
      if (flower->tunnel) {
          sample->tunnel = CONST_CAST(struct flow_tnl *, tnl);
      }
-    sample->userspace_actions = CONST_CAST(struct nlattr *, next_actions);
-    sample->userspace_actions_len = next_actions_len;
+
+    sample->userspace_actions = xmalloc(next_actions_len + NLA_HDRLEN);
+    nullable_memcpy((char *) sample->userspace_actions + NLA_HDRLEN,
+                    next_actions, next_actions_len);
+    sample->userspace_actions->nla_len = next_actions_len + NLA_HDRLEN;
      sample->ifindex = flower->ifindex;
  }

@@ -2161,31 +2164,25 @@ parse_sample_action(struct tc_flower *flower, struct 
tc_action *tc_action,
      struct offload_sample sample;
      const struct nlattr *nla;
      unsigned int left;
+    uint32_t rate = 0;
+    int ret = EINVAL;
      uint32_t sgid;
-    int err;

      offload_sample_init(&sample, next_actions, next_actions_len, flower, tnl);
      sample.action = CONST_CAST(struct nlattr *, sample_action);

-    err = EINVAL;
      NL_NESTED_FOR_EACH_UNSAFE (nla, left, sample_action) {
          if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_ACTIONS) {
-            err = parse_sample_actions_attribute(nla, &sample);
-            if (err) {
-                break;
-            }
+            ret = parse_sample_actions_attribute(nla, &sample);
          } 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);
-            if (!tc_action->sample.rate) {
-                break;
-            }
+            rate = UINT32_MAX / nl_attr_get_u32(nla);
          } else {
-            break;
+            return EINVAL;
          }
      }

-    if (!tc_action->sample.rate || err) {
+    /* This check makes sure both attributes above were present and valid. */
+    if (!rate || ret) {
          return EINVAL;
      }

@@ -2194,10 +2191,13 @@ parse_sample_action(struct tc_flower *flower, struct 
tc_action *tc_action,
          VLOG_DBG_RL(&rl, "Failed allocating group id for sample action");
          return ENOENT;
      }
+
+    tc_action->type = TC_ACT_SAMPLE;
+    tc_action->sample.rate = rate;
      tc_action->sample.group_id = sgid;
      flower->action_count++;

-    return err;
+    return 0;
  }

  static int
@@ -2232,7 +2232,7 @@ parse_userspace_action(struct tc_flower *flower, struct 
tc_action *tc_action,
      tc_action->sample.rate = 1;
      flower->action_count++;

-    return err;
+    return 0;
  }

  static int

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev