On 12 Jan 2023, at 18:29, Michael Santana wrote:
> On 12/15/22 09:52, Eelco Chaudron wrote: >> When a flow gets modified, i.e. the actions are changes, the tc layer will >> remove, and re-add the flow. This is causing all the counters to be reset. >> >> This patch will remember the previous tc counters and adjust any requests >> for statistics. This is done in a similar way as the rte_flow implementation. >> >> It also updates the check_pkt_len tc test to purge the flows, so we do >> not use updated tc flows, with their counters. >> >> Signed-off-by: Eelco Chaudron <[email protected]> >> --- >> Please note that for now two copies of the test case exists, but I will clean >> this up once this gets applied by submitting a new revision of the >> 'tests: Add system-traffic.at tests to check-offloads' series. >> >> v2: Do not update the stats->used, as in terse dump they should be 0. >> >> lib/netdev-offload-tc.c | 92 >> ++++++++++++++++++++++++++++++++------ >> lib/tc.h | 1 >> tests/system-offloads-traffic.at | 65 +++++++++++++++++++++++++-- >> tests/system-traffic.at | 64 ++++++++++++++++++++++++++ >> 4 files changed, 201 insertions(+), 21 deletions(-) >> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index ce7f8ad97..03c25fc38 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -97,6 +97,12 @@ static int netdev_tc_parse_nl_actions(struct netdev >> *netdev, >> bool *recirc_act, bool more_actions, >> struct tc_action **need_jump_update); >> +static void parse_tc_flower_to_stats(struct tc_flower *flower, >> + struct dpif_flow_stats *stats); >> + >> +static int get_ufid_adjust_stats(const ovs_u128 *ufid, >> + struct dpif_flow_stats *stats); >> + >> static bool >> is_internal_port(const char *type) >> { >> @@ -193,6 +199,9 @@ static struct ovs_mutex ufid_lock = >> OVS_MUTEX_INITIALIZER; >> * @ufid: ufid assigned to the flow >> * @id: tc filter id (tcf_id) >> * @netdev: netdev associated with the tc rule >> + * @adjust_stats: When flow gets updated with new actions, we need to adjust >> + * the reported stats to include previous values as the >> hardware >> + * rule is removed and re-added. This stats copy is used for >> it. >> */ >> struct ufid_tc_data { >> struct hmap_node ufid_to_tc_node; >> @@ -200,6 +209,7 @@ struct ufid_tc_data { >> ovs_u128 ufid; >> struct tcf_id id; >> struct netdev *netdev; >> + struct dpif_flow_stats adjust_stats; >> }; >> static void >> @@ -233,12 +243,36 @@ del_ufid_tc_mapping(const ovs_u128 *ufid) >> ovs_mutex_unlock(&ufid_lock); >> } >> +static void >> +netdev_tc_adjust_stats(struct dpif_flow_stats *stats, >> + struct dpif_flow_stats *adjust_stats) >> +{ >> + /* Do not try to restore the stats->used, as in terse >> + * mode dumps we will always report them as 0. */ >> + stats->n_bytes += adjust_stats->n_bytes; >> + stats->n_packets += adjust_stats->n_packets; > Why not also update the other members of dpif_flow_stats? tcp_flags is not used by TC, so no need to use it. Will update the comment above. >> +} >> + >> /* Wrapper function to delete filter and ufid tc mapping */ >> static int >> -del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid) >> +del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid, >> + struct dpif_flow_stats *stats) >> { >> + struct tc_flower flower; >> int err; >> + if (stats) { >> + memset(stats, 0, sizeof *stats); >> + if (!tc_get_flower(id, &flower)) { >> + struct dpif_flow_stats adjust_stats; >> + >> + parse_tc_flower_to_stats(&flower, stats); >> + if (!get_ufid_adjust_stats(ufid, &adjust_stats)) { >> + netdev_tc_adjust_stats(stats, &adjust_stats); >> + } >> + } >> + } >> + >> err = tc_del_filter(id); >> if (!err) { >> del_ufid_tc_mapping(ufid); >> @@ -249,7 +283,7 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const >> ovs_u128 *ufid) >> /* Add ufid entry to ufid_to_tc hashmap. */ >> static void >> add_ufid_tc_mapping(struct netdev *netdev, const ovs_u128 *ufid, >> - struct tcf_id *id) >> + struct tcf_id *id, struct dpif_flow_stats *stats) >> { >> struct ufid_tc_data *new_data = xzalloc(sizeof *new_data); >> size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0); >> @@ -261,6 +295,9 @@ add_ufid_tc_mapping(struct netdev *netdev, const >> ovs_u128 *ufid, >> new_data->ufid = *ufid; >> new_data->id = *id; >> new_data->netdev = netdev_ref(netdev); >> + if (stats) { >> + new_data->adjust_stats = *stats; >> + } > lgtm >> ovs_mutex_lock(&ufid_lock); >> hmap_insert(&ufid_to_tc, &new_data->ufid_to_tc_node, ufid_hash); >> @@ -292,6 +329,25 @@ get_ufid_tc_mapping(const ovs_u128 *ufid, struct tcf_id >> *id) >> return ENOENT; >> } >> > /* Get adjust_stats from ufid_to_tc hashmap. > * > * Returns 0 if successful and fills stats with adjust_stats. > * Otherwise returns the error. > */ I’ll add this to the next version, however, we do not seem to be very consistent in adding functions description (except for this file maybe ;) >> +static int >> +get_ufid_adjust_stats(const ovs_u128 *ufid, struct dpif_flow_stats *stats) >> +{ >> + size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0); >> + struct ufid_tc_data *data; >> + >> + ovs_mutex_lock(&ufid_lock); >> + HMAP_FOR_EACH_WITH_HASH (data, ufid_to_tc_node, ufid_hash, &ufid_to_tc) >> { > This question isn't about your patch so feel free to ignore it. It's unclear > to me how data gets assigned. I get that the macro looks through the nodes of > ufid_to_tc with matching ufid_hash. What I dont get is how it converts the > hmap_node to ufid_tc_data You tell the iterator what the offset is in the data structure to the hmap node. So for the iterator to find a node it’s just data = hmap_node - offset_to_hmap_node, i.e. a CONTAINER_OF() like funtion. I think the ITER_VAR definition has some info. Let me know if this is what you where looking for. > Other than that this section looks identical to get_ufid_tc_mapping, so it > lgtm. But if we add more get_* functions we should probaby write a function > to return the matching ufid_tc_data structure instead That would be more convenient, but the problem is that in that case, we need to keep the mutex until we are done with the structure. >> + if (ovs_u128_equals(*ufid, data->ufid)) { >> + *stats = data->adjust_stats; >> + ovs_mutex_unlock(&ufid_lock); >> + return 0; >> + } >> + } >> + ovs_mutex_unlock(&ufid_lock); >> + >> + return ENOENT; >> +} >> + >> /* Find ufid entry in ufid_to_tc hashmap using tcf_id id. >> * The result is saved in ufid. >> * >> @@ -1193,6 +1249,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump, >> get_tc_qdisc_hook(netdev)); >> while (nl_dump_next(dump->nl_dump, &nl_flow, rbuffer)) { >> + struct dpif_flow_stats adjust_stats; >> struct tc_flower flower; >> if (parse_netlink_to_tc_flower(&nl_flow, &id, &flower, >> dump->terse)) { >> @@ -1210,6 +1267,10 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump >> *dump, >> continue; >> } >> + if (!get_ufid_adjust_stats(ufid, &adjust_stats)) { >> + netdev_tc_adjust_stats(stats, &adjust_stats); >> + } >> + >> match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX); >> match->flow.in_port.odp_port = dump->port; >> match_set_recirc_id(match, id.chain); >> @@ -2058,6 +2119,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match >> *match, >> struct flow *mask = &match->wc.masks; >> const struct flow_tnl *tnl = &match->flow.tunnel; >> struct flow_tnl *tnl_mask = &mask->tunnel; >> + struct dpif_flow_stats adjust_stats; >> bool recirc_act = false; >> uint32_t block_id = 0; >> struct tcf_id id; >> @@ -2351,10 +2413,12 @@ netdev_tc_flow_put(struct netdev *netdev, struct >> match *match, >> return EOPNOTSUPP; >> } >> + memset(&adjust_stats, 0, sizeof adjust_stats); >> if (get_ufid_tc_mapping(ufid, &id) == 0) { >> VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", >> id.handle, id.prio); >> - info->tc_modify_flow_deleted = !del_filter_and_ufid_mapping(&id, >> ufid); >> + info->tc_modify_flow_deleted = !del_filter_and_ufid_mapping( >> + &id, ufid, &adjust_stats); > It would had been nice to get rid of del_filter_and_ufid_mapping and call > del_filter_and_ufid_mapping instead since it is also calling > get_ufid_tc_mapping but this code snippet is doing something slightly > different You mention del_filter_and_ufid_mapping twice, not sure what you mean. >> } >> prio = get_prio_for_tc_flower(&flower); >> @@ -2372,8 +2436,9 @@ netdev_tc_flow_put(struct netdev *netdev, struct match >> *match, >> if (!err) { >> if (stats) { >> memset(stats, 0, sizeof *stats); >> + netdev_tc_adjust_stats(stats, &adjust_stats); > Why do we bother to do this? is the caller going to be looking at stats? Only the dpctl flow commands do if statistics are requested. The upcall handling does not. But if it changes in the future we better support this. There are already enough different behaviours in hw offload :( >> } >> - add_ufid_tc_mapping(netdev, ufid, &id); >> + add_ufid_tc_mapping(netdev, ufid, &id, &adjust_stats); >> } >> return err; >> @@ -2414,6 +2479,13 @@ netdev_tc_flow_get(struct netdev *netdev, >> parse_tc_flower_to_match(netdev, &flower, match, actions, >> stats, attrs, buf, false); >> + if (stats) { >> + struct dpif_flow_stats adjust_stats; >> + >> + if (!get_ufid_adjust_stats(ufid, &adjust_stats)) { >> + netdev_tc_adjust_stats(stats, &adjust_stats); >> + } >> + } > lgtm >> match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX); >> match->flow.in_port.odp_port = in_port; >> match_set_recirc_id(match, id.chain); >> @@ -2426,7 +2498,6 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED, >> const ovs_u128 *ufid, >> struct dpif_flow_stats *stats) >> { >> - struct tc_flower flower; >> struct tcf_id id; >> int error; >> @@ -2435,16 +2506,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED, >> return error; >> } >> - if (stats) { >> - memset(stats, 0, sizeof *stats); >> - if (!tc_get_flower(&id, &flower)) { >> - parse_tc_flower_to_stats(&flower, stats); >> - } >> - } >> - >> - error = del_filter_and_ufid_mapping(&id, ufid); >> - >> - return error; >> + return del_filter_and_ufid_mapping(&id, ufid, stats); >> } >> static int >> diff --git a/lib/tc.h b/lib/tc.h >> index a828fd3e3..404726f14 100644 >> --- a/lib/tc.h >> +++ b/lib/tc.h >> @@ -343,7 +343,6 @@ static inline bool >> is_tcf_id_eq(struct tcf_id *id1, struct tcf_id *id2) >> { >> return id1->prio == id2->prio >> - && id1->handle == id2->handle >> && id1->handle == id2->handle > Good catch! How did this ever get in? It has been there since day one, guess no one noticed :) Thanks for the review, will send a v3 with the changes in the comments. <SNIP> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
