netdev-offload-tc: Preserve tc statistics when flow gets modified.

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 existing updated tc flow counters, but start with fresh installed
set of datapath flows.

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

2 files changed, 84 insertions, 16 deletions

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 6e1bbaa28..134c24157 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,38 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
     ovs_mutex_unlock(&ufid_lock);
 }
 
+static void
+netdev_tc_adjust_stats(struct dpif_flow_stats *stats,
+                       const struct dpif_flow_stats *adjust_stats)
+{
+    /* Do not try to restore the stats->used, as in terse mode dumps TC doesn't
+     * report TCA_ACT_OPTIONS, so the 'lastused' value is not available, hence
+     * we report used as 0.
+     * tcp_flags is not collected by tc, so no need to update it. */
+    stats->n_bytes += adjust_stats->n_bytes;
+    stats->n_packets += adjust_stats->n_packets;
+}
+
 /* 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_flower_filter(id);
     if (!err) {
         del_ufid_tc_mapping(ufid);
@@ -249,7 +285,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 +297,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;
+    }
 
     ovs_mutex_lock(&ufid_lock);
     hmap_insert(&ufid_to_tc, &new_data->ufid_to_tc_node, ufid_hash);
@@ -292,6 +331,30 @@ 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.
+*/
+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) {
+        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 +1256,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 +1274,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);
@@ -2059,6 +2127,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;
@@ -2352,10 +2421,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);
     }
 
     prio = get_prio_for_tc_flower(&flower);
@@ -2373,8 +2444,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);
         }
-        add_ufid_tc_mapping(netdev, ufid, &id);
+        add_ufid_tc_mapping(netdev, ufid, &id, &adjust_stats);
     }
 
     return err;
@@ -2415,6 +2487,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);
+        }
+    }
     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);
@@ -2427,7 +2506,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;
 
@@ -2436,16 +2514,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 ea4ce806b..cdd3b4f60 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -344,7 +344,6 @@ 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
            && id1->hook == id2->hook
            && id1->block_id == id2->block_id
            && id1->ifindex == id2->ifindex