On 3/16/2023 5:23 PM, Eelco Chaudron wrote:
On 1 Mar 2023, at 8:22, Chris Mi wrote:Create a unique group ID to map the sFlow info when offloading sample action to TC. When showing the offloaded datapath flows, translate the group ID from TC sample action to sFlow info using the mapping.See some comments inline below. //EelcoSigned-off-by: Chris Mi<c...@nvidia.com> Reviewed-by: Roi Dayan<r...@nvidia.com> --- lib/netdev-offload-tc.c | 267 +++++++++++++++++++++++++++++++++++++--- lib/netdev-offload.h | 1 + lib/tc.c | 62 +++++++++- lib/tc.h | 11 +- 4 files changed, 319 insertions(+), 22 deletions(-) diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 510ba922d..b9c4e2389 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -92,13 +92,23 @@ static struct hmap police_idx_to_meter_id OVS_GUARDED_BY(meter_mutex) static int meter_id_lookup(uint32_t meter_id, uint32_t *police_idx); static int police_idx_lookup(uint32_t police_idx, uint32_t *meter_id); +/* To avoid passing too many parameters when offloading sample action, + * aggregate them here. + */Comment should end on the same line.
Done.
+struct sample_param { + const struct flow_tnl *tnl; /* Tunnel info. */ + const ovs_u128 *ufid; /* Ufid. */ + uint32_t gid; /* Generated gid. */ +}; +See also Ilya's comments on this, it would be nice if we do not need this structure... I tried just adding the parameters to some functions, and it does not look too bad. I've added a raw diff at the end (it does compile but has some open ends ;). Also, ufid is already part of the flower struct, maybe we should make it a ufid structure and do the conversion in tc.c as we only use the cookie for ufid if I'm right.
Done. It is removed.
static int netdev_tc_parse_nl_actions(struct netdev *netdev, struct tc_flower *flower, struct offload_info *info, const struct nlattr *actions, size_t actions_len, bool *recirc_act, bool more_actions, - struct tc_action **need_jump_update); + struct tc_action **need_jump_update, + struct sample_param *param); static void parse_tc_flower_to_stats(struct tc_flower *flower, struct dpif_flow_stats *stats); @@ -109,6 +119,12 @@ static int get_ufid_adjust_stats(const ovs_u128 *ufid, static struct nl_sock *psample_sock; static int psample_family; +static bool +psample_supported(void) +{ + return psample_sock != NULL; +} + /* When offloading sample action to TC, userspace creates a unique ID * to map sFlow action and tunnel info and passes this ID to kernel * instead of the sFlow info. Psample will send this ID and sampled @@ -1002,6 +1018,19 @@ parse_tc_flower_to_actions__(struct tc_flower *flower, struct ofpbuf *buf, action = &flower->actions[i]; 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); } @@ -2107,6 +2136,174 @@ parse_match_ct_state_to_flower(struct tc_flower *flower, struct match *match) } } +static int +parse_userspace_attributes(const struct nlattr *actions, + struct offload_sflow *sflow) +{ + 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 = (struct user_action_cookie *) nl_attr_get(nla);See Ilya's comment on misaligned data.
Ilya replied before. He said there is no problem for this case. How about this diff to check the misaligned data:@@ -2024,8 +2024,12 @@ parse_userspace_attributes(const struct nlattr *actions,
NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
if (nl_attr_type(nla) == OVS_USERSPACE_ATTR_USERDATA) {
+ size_t userdata_len = nl_attr_get_size(nla);
struct user_action_cookie *cookie;
+ if (userdata_len != sizeof *cookie) {
+ continue;
+ }
cookie = (struct user_action_cookie *) nl_attr_get(nla);
if (cookie->type == USER_ACTION_COOKIE_SFLOW) {
sample->type = USER_ACTION_COOKIE_SFLOW;
+ if (cookie->type == USER_ACTION_COOKIE_SFLOW) { + sflow->userdata = (struct nlattr *) nla;Maybe the sflow->userdata can be const also? If not we should probably use the CONST_CAST() macro.
Done. Added CONST_CAST().
+ 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 offload_sflow *sflow) +{ + 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); + } 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 +offload_sflow_set(struct offload_sflow *sflow, const struct nlattr *actions, + size_t actions_len, bool tunnel, struct sample_param *param)We seem to use _init a lot, so I would suggest changing this function to something like offload_sflow_init().
Done.
+{ + if (param->gid) { + VLOG_ERR_RL(&error_rl, + "Only a single TC_SAMPLE action per flow is supported"); + return EOPNOTSUPP; + } + + memset(sflow, 0, sizeof *sflow); + sflow->ufid = *(param->ufid); + if (tunnel) { + sflow->tunnel = CONST_CAST(struct flow_tnl *, param->tnl); + } else { + sflow->actions = xmalloc(actions_len + NLA_HDRLEN);Do we really need to allocate this with the additional NLA_DHRLEN? Anyway, these are not the right set of actions. These are the actions following the sflow specific attr, and are not of interest to sflow offload they should/will be executed by tc in the datapath.
It is used to show output tunnel info.
+ nullable_memcpy((char *) sflow->actions + NLA_HDRLEN, actions, + actions_len); + sflow->actions->nla_len = actions_len + NLA_HDRLEN; + } + + return 0; +} + +static int +parse_sample_action(struct tc_flower *flower, struct tc_action *tc_action, + const struct nlattr *actions, size_t actions_len, + const struct nlattr *sample_action, + struct sample_param *param)The actions and actions_len are the remaining actions to be processed, they are not part of the sample action, and I do not understand why you pass them on. All you need is the sample_action, which is what you were using in v18. You execute those actions again in userspace, which is not what sflow should do. This could lead to duplicate packets.
It is used to show output tunnel info.
+{ + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); + struct offload_sflow sflow; + const struct nlattr *nla; + unsigned int left; + int err; + + err = offload_sflow_set(&sflow, actions, actions_len, flower->tunnel, + param); + if (err) { + return err; + } + sflow.action = (struct nlattr *) sample_action; +Here the err should be set to EINVAL, because if OVS_SAMPLE_ATTR_ACTIONS is not found we should return an error, see v18 for correct behavior.
Do you mean offload_sflow_set() return value or something else?
+ NL_NESTED_FOR_EACH_UNSAFE (nla, left, sample_action) { + if (nl_attr_type(nla) == OVS_SAMPLE_ATTR_ACTIONS) { + err = parse_sample_actions_attribute(nla, &sflow); + } 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 { + err = EINVAL; + goto out; + } + } + + if (!tc_action->sample.rate || err) {if (err) you will overwrite the previous errors from parse_sample_actions_attribute()
Done. I moved the error check inside of NL_NESTED_FOR_EACH_UNSAFE().
+ err = EINVAL; + goto out; + } + + param->gid = sgid_alloc_ctx(&sflow); + if (!param->gid) { + VLOG_DBG_RL(&rl, "Failed allocating group id for sample action"); + err = ENOENT; + goto out; + } + tc_action->sample.group_id = param->gid; + flower->action_count++; + +out: + if (sflow.actions) { + free(sflow.actions);This additional null check is not needed as free() will do this internally.
Done.
+ } + return err; +} + +static int +parse_userspace_action(struct tc_flower *flower, struct tc_action *tc_action, + const struct nlattr *actions, size_t actions_len, + const struct nlattr *userspace_action, + struct sample_param *param) +{Same as above, the actions and actions_len are the remaining action to be processed, they are not part of the sample action, and I do not understand why you pass them on. All you need is the userspace_action, which is what you were using in v18.
It is used to show output tunnel info.
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); + struct offload_sflow sflow; + int err; + + err = offload_sflow_set(&sflow, actions, actions_len, flower->tunnel, + param); + if (err) { + return 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. + */Comments should end on the same line.
Done.
+ err = parse_userspace_attributes(userspace_action, &sflow); + if (err) { + goto out; + } + sflow.action = (struct nlattr *) userspace_action; + param->gid = sgid_alloc_ctx(&sflow); + if (!param->gid) { + VLOG_DBG_RL(&rl, "Failed allocating group id for sample action"); + err = ENOENT; + goto out; + } + tc_action->type = TC_ACT_SAMPLE; + tc_action->sample.group_id = param->gid; + tc_action->sample.rate = 1; + flower->action_count++; + +out: + if (sflow.actions) { + free(sflow.actions);This additional null check is not needed as free() will do this internally.
Done.
+ } + return err; +} static int parse_check_pkt_len_action(struct netdev *netdev, struct tc_flower *flower, @@ -2154,7 +2351,7 @@ parse_check_pkt_len_action(struct netdev *netdev, struct tc_flower *flower, nl_attr_get(nl_actions), nl_attr_get_size(nl_actions), recirc_act, !last_action, - &ge_jump_update); + &ge_jump_update, NULL);Would this be a problem if an sflow action is embedded in check_pkt_len?
Maybe will crash. sample_param should pass as argument.
if (err) { return err; } @@ -2170,7 +2367,7 @@ parse_check_pkt_len_action(struct netdev *netdev, struct tc_flower *flower, nl_attr_get(nl_actions), nl_attr_get_size(nl_actions), recirc_act, !last_action, - &le_jump_update); + &le_jump_update, NULL);See above.
Done.
if (gt_offset == flower->action_count && last_action) { /* No le actions where added, fix gt offset. */ @@ -2221,7 +2418,8 @@ netdev_tc_parse_nl_actions(struct netdev *netdev, struct tc_flower *flower, struct offload_info *info, const struct nlattr *actions, size_t actions_len, bool *recirc_act, bool more_actions, - struct tc_action **need_jump_update) + struct tc_action **need_jump_update, + struct sample_param *param) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); const struct nlattr *nla; @@ -2358,10 +2556,27 @@ netdev_tc_parse_nl_actions(struct netdev *netdev, struct tc_flower *flower, return err; } } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_SAMPLE) { - struct offload_sflow sflow; - - memset(&sflow, 0, sizeof sflow); - sgid_alloc_ctx(&sflow); + if (!psample_supported()) { + VLOG_DBG_RL(&rl, "Unsupported put action type: %d, psample is " + "not initialized successfully", nl_attr_type(nla)); + return EOPNOTSUPP; + } + err = parse_sample_action(flower, action, actions, actions_len, + nla, param); + if (err) { + return err; + } + } else if (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; + } + err = parse_userspace_action(flower, action, actions, actions_len, + nla, param); + if (err) { + return err; + } } else { VLOG_DBG_RL(&rl, "unsupported put action type: %d", nl_attr_type(nla)); @@ -2385,6 +2600,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, const struct flow_tnl *tnl = &match->flow.tunnel; struct flow_tnl *tnl_mask = &mask->tunnel; struct dpif_flow_stats adjust_stats; + struct sample_param param; bool recirc_act = false; uint32_t block_id = 0; struct tcf_id id; @@ -2665,17 +2881,21 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, return err; } + param.tnl = tnl; + param.ufid = ufid; + param.gid = 0; /* Parse all (nested) actions. */ err = netdev_tc_parse_nl_actions(netdev, &flower, info, actions, actions_len, &recirc_act, - false, NULL); + false, NULL, ¶m); if (err) { - return err; + goto 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 out; } memset(&adjust_stats, 0, sizeof adjust_stats); @@ -2689,21 +2909,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_all(ifindex, block_id, chain, prio, hook, param.gid);I think this id should go in to ufid_to_tc_data as mentioned earlier. Also, we should think about supporting more than one sample id.
Done.
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 out; + } + + if (stats) { + memset(stats, 0, sizeof *stats); + netdev_tc_adjust_stats(stats, &adjust_stats); + } + add_ufid_tc_mapping(netdev, ufid, &id, &adjust_stats); + return 0; + +out:Maybe call this error_out: as this is for error cases only?
Done.
+ if (param.gid) { + sgid_free(param.gid); } return err; diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h index 09cdb6db2..1bf648c68 100644 --- a/lib/netdev-offload.h +++ b/lib/netdev-offload.h @@ -81,6 +81,7 @@ struct offload_info { bool tc_modify_flow_deleted; /* Indicate the tc modify flow put success * to delete the original flow. */ odp_port_t orig_in_port; /* Originating in_port for tnl flows. */ +This new newline can be removed.
Done.
}; DECLARE_EXTERN_PER_THREAD_DATA(unsigned int, netdev_offload_thread_id); diff --git a/lib/tc.c b/lib/tc.c index 4c07e2216..4bbecb26a 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; @@ -2789,6 +2824,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) { @@ -3101,6 +3153,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++; @@ -3308,6 +3361,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) { nl_msg_put_flower_acts_release(request, act_index++); diff --git a/lib/tc.h b/lib/tc.h index 0d9b1b8e7..1892b98d6 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; @@ -330,12 +335,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)See the earlier comment to use the ufid_to_tc_data data structure.
Done.
{ 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; } -- 2.26.3RAW diff of the earlier mentioned experiment. Please do not use it as is, it’s just to show how what parameters get added to each function, and it does not look to bad: git diff diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index b63b479b6..b3b3763c5 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -92,23 +92,14 @@ static struct hmap police_idx_to_meter_id OVS_GUARDED_BY(meter_mutex) static int meter_id_lookup(uint32_t meter_id, uint32_t *police_idx); static int police_idx_lookup(uint32_t police_idx, uint32_t *meter_id); -/* To avoid passing too many parameters when offloading sample action, - * aggregate them here. - */ -struct sample_param { - const struct flow_tnl *tnl; /* Tunnel info. */ - const ovs_u128 *ufid; /* Ufid. */ - uint32_t gid; /* Generated gid. */ -}; - 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, - struct sample_param *param); + struct tc_action **need_jump_update); static void parse_tc_flower_to_stats(struct tc_flower *flower, struct dpif_flow_stats *stats); @@ -2183,18 +2174,21 @@ parse_sample_actions_attribute(const struct nlattr *actions, static int offload_sflow_set(struct offload_sflow *sflow, const struct nlattr *actions, - size_t actions_len, bool tunnel, struct sample_param *param) + size_t actions_len, const struct flow_tnl *tnl, + const ovs_u128 *ufid) { - if (param->gid) { - VLOG_ERR_RL(&error_rl, - "Only a single TC_SAMPLE action per flow is supported"); - return EOPNOTSUPP; - } +// FIX SOMEWHERE ELSE MAYBE KEEP COUNT IN tc_flower and error out in +// before calling parse_xxx_action(). +// if (param->gid) { +// VLOG_ERR_RL(&error_rl, +// "Only a single TC_SAMPLE action per flow is supported"); +// return EOPNOTSUPP; +// } memset(sflow, 0, sizeof *sflow); - sflow->ufid = *(param->ufid); - if (tunnel) { - sflow->tunnel = CONST_CAST(struct flow_tnl *, param->tnl); + sflow->ufid = *ufid; + if (tnl) { + sflow->tunnel = CONST_CAST(struct flow_tnl *, tnl); } else { sflow->actions = xmalloc(actions_len + NLA_HDRLEN); nullable_memcpy((char *) sflow->actions + NLA_HDRLEN, actions, @@ -2207,18 +2201,20 @@ offload_sflow_set(struct offload_sflow *sflow, const struct nlattr *actions, static int parse_sample_action(struct tc_flower *flower, struct tc_action *tc_action, + const struct flow_tnl *tnl, const struct nlattr *actions, size_t actions_len, - const struct nlattr *sample_action, - struct sample_param *param) + const struct nlattr *sample_action) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); struct offload_sflow sflow; const struct nlattr *nla; unsigned int left; + uint32_t sgid; int err; - err = offload_sflow_set(&sflow, actions, actions_len, flower->tunnel, - param); + err = offload_sflow_set(&sflow, actions, actions_len, + flower->tunnel ? tnl : NULL, + (const ovs_u128 *) flower->act_cookie.data); if (err) { return err; } @@ -2241,13 +2237,13 @@ parse_sample_action(struct tc_flower *flower, struct tc_action *tc_action, goto out; } - param->gid = sgid_alloc_ctx(&sflow); - if (!param->gid) { + sgid = sgid_alloc_ctx(&sflow); + if (!sgid) { VLOG_DBG_RL(&rl, "Failed allocating group id for sample action"); err = ENOENT; goto out; } - tc_action->sample.group_id = param->gid; + tc_action->sample.group_id = sgid; flower->action_count++; out: @@ -2259,16 +2255,18 @@ out: static int parse_userspace_action(struct tc_flower *flower, struct tc_action *tc_action, + const struct flow_tnl *tnl, const struct nlattr *actions, size_t actions_len, - const struct nlattr *userspace_action, - struct sample_param *param) + const struct nlattr *userspace_action) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); struct offload_sflow sflow; + uint sgid; int err; - err = offload_sflow_set(&sflow, actions, actions_len, flower->tunnel, - param); + err = offload_sflow_set(&sflow, actions, actions_len, + flower->tunnel ? tnl : NULL, + (const ovs_u128 *) flower->act_cookie.data); if (err) { return err; } @@ -2282,14 +2280,14 @@ parse_userspace_action(struct tc_flower *flower, struct tc_action *tc_action, goto out; } sflow.action = (struct nlattr *) userspace_action; - param->gid = sgid_alloc_ctx(&sflow); - if (!param->gid) { + sgid = sgid_alloc_ctx(&sflow); + if (!sgid) { VLOG_DBG_RL(&rl, "Failed allocating group id for sample action"); err = ENOENT; goto out; } tc_action->type = TC_ACT_SAMPLE; - tc_action->sample.group_id = param->gid; + tc_action->sample.group_id = sgid; tc_action->sample.rate = 1; flower->action_count++; @@ -2302,7 +2300,9 @@ out: static int parse_check_pkt_len_action(struct netdev *netdev, struct tc_flower *flower, - struct offload_info *info, struct tc_action *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) @@ -2342,11 +2342,11 @@ 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, - &ge_jump_update, NULL); + &ge_jump_update); if (err) { return err; } @@ -2358,11 +2358,11 @@ 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, - &le_jump_update, NULL); + &le_jump_update); if (gt_offset == flower->action_count && last_action) { /* No le actions where added, fix gt offset. */ @@ -2411,10 +2411,10 @@ 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, - struct sample_param *param) + struct tc_action **need_jump_update) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); const struct nlattr *nla; @@ -2541,7 +2541,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, @@ -2556,8 +2557,8 @@ netdev_tc_parse_nl_actions(struct netdev *netdev, struct tc_flower *flower, "not initialized successfully", nl_attr_type(nla)); return EOPNOTSUPP; } - err = parse_sample_action(flower, action, actions, actions_len, - nla, param); + err = parse_sample_action(flower, action, tnl, + actions, actions_len, nla); if (err) { return err; } @@ -2567,8 +2568,8 @@ netdev_tc_parse_nl_actions(struct netdev *netdev, struct tc_flower *flower, "not initialized successfully", nl_attr_type(nla)); return EOPNOTSUPP; } - err = parse_userspace_action(flower, action, actions, actions_len, - nla, param); + err = parse_userspace_action(flower, action, tnl, actions, + actions_len, nla); if (err) { return err; } @@ -2595,7 +2596,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, const struct flow_tnl *tnl = &match->flow.tunnel; struct flow_tnl *tnl_mask = &mask->tunnel; struct dpif_flow_stats adjust_stats; - struct sample_param param; bool recirc_act = false; uint32_t block_id = 0; struct tcf_id id; @@ -2612,6 +2612,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, } memset(&flower, 0, sizeof flower); + flower.act_cookie.data = ufid; + flower.act_cookie.len = sizeof *ufid; chain = key->recirc_id; < mask->recirc_id = 0; @@ -2876,13 +2878,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, return err; } - param.tnl = tnl; - param.ufid = ufid; - param.gid = 0; /* Parse all (nested) actions. */ - err = netdev_tc_parse_nl_actions(netdev, &flower, info, - actions, actions_len, &recirc_act, - false, NULL, ¶m); + err = netdev_tc_parse_nl_actions(netdev, &flower, info, tnl, actions, + actions_len, &recirc_act, + false, NULL); if (err) { goto out; } @@ -2908,11 +2907,9 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, 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_all(ifindex, block_id, chain, prio, hook, param.gid); + id = tc_make_tcf_id_all(ifindex, block_id, chain, prio, hook, 0); + // Removed param.gid as we should store it somewhere else, see comments. err = tc_replace_flower(&id, &flower); if (err) { goto out; @@ -2926,9 +2923,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match, return 0; out: - if (param.gid) { - sgid_free(param.gid); - } + // We need to do something to clean up all on failure. + //if (param.gid) { + // sgid_free(param.gid); + //} return err; }
_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
