ofproto-dpif-upcall: Wait for valid hw flow stats before applying min-revalidate-pps.

Depending on the driver implementation, it can take from 0.2 seconds
up to 2 seconds before offloaded flow statistics are updated. This is
true for both TC and rte_flow-based offloading. This is causing a
problem with min-revalidate-pps, as old statistic values are used
during this period.

This fix will wait for at least 2 seconds, by default, before assuming no
packets where received during this period.

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

4 files changed, 32 insertions, 10 deletions

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 4dab51dff..cac118c61 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2116,10 +2116,12 @@ ukey_delete(struct umap *umap, struct udpif_key *ukey)
 }
 
 static bool
-should_revalidate(const struct udpif *udpif, uint64_t packets,
-                  long long int used)
+should_revalidate(const struct udpif *udpif, const struct udpif_key *ukey,
+                  uint64_t packets)
+    OVS_REQUIRES(ukey->mutex)
 {
     long long int metric, now, duration;
+    long long int used = ukey->stats.used;
 
     if (!ofproto_min_revalidate_pps) {
         return true;
@@ -2150,8 +2152,12 @@ should_revalidate(const struct udpif *udpif, uint64_t packets,
     duration = now - used;
     metric = duration / packets;
 
-    if (metric < 1000 / ofproto_min_revalidate_pps) {
-        /* The flow is receiving more than min-revalidate-pps, so keep it. */
+    if (metric < 1000 / ofproto_min_revalidate_pps ||
+        (ukey->offloaded && duration < ofproto_offloaded_stats_delay)) {
+        /* The flow is receiving more than min-revalidate-pps, so keep it.
+         * Or it's a hardware offloaded flow that might take up to X seconds
+         * to update its statistics. Until we are sure the statistics had a
+         * chance to be updated, also keep it. */
         return true;
     }
     return false;
@@ -2355,7 +2361,7 @@ static enum reval_result
 revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
                 const struct dpif_flow_stats *stats,
                 struct ofpbuf *odp_actions, uint64_t reval_seq,
-                struct recirc_refs *recircs, bool offloaded)
+                struct recirc_refs *recircs)
     OVS_REQUIRES(ukey->mutex)
 {
     bool need_revalidate = ukey->reval_seq != reval_seq;
@@ -2381,7 +2387,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
     }
 
     if (need_revalidate) {
-        if (should_revalidate(udpif, push.n_packets, ukey->stats.used)) {
+        if (should_revalidate(udpif, ukey, push.n_packets)) {
             if (!ukey->xcache) {
                 ukey->xcache = xlate_cache_new();
             } else {
@@ -2397,7 +2403,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
 
     /* Stats for deleted flows will be attributed upon flow deletion. Skip. */
     if (result != UKEY_DELETE) {
-        xlate_push_stats(ukey->xcache, &push, offloaded);
+        xlate_push_stats(ukey->xcache, &push, ukey->offloaded);
         ukey->stats = *stats;
         ukey->reval_seq = reval_seq;
     }
@@ -2853,8 +2859,7 @@ revalidate(struct revalidator *revalidator)
                 result = UKEY_DELETE;
             } else {
                 result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
-                                         reval_seq, &recircs,
-                                         f->attrs.offloaded);
+                                         reval_seq, &recircs);
             }
             ukey->dump_seq = dump_seq;
 
@@ -2939,7 +2944,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
                     COVERAGE_INC(revalidate_missed_dp_flow);
                     memcpy(&stats, &ukey->stats, sizeof stats);
                     result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
-                                             reval_seq, &recircs, false);
+                                             reval_seq, &recircs);
                 }
                 if (result != UKEY_KEEP) {
                     /* Clears 'recircs' if filled by revalidate_ukey(). */
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index a84ddc1d0..143ded690 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -541,6 +541,11 @@ extern unsigned ofproto_max_revalidator;
  * duration exceeds half of max-revalidator config variable. */
 extern unsigned ofproto_min_revalidate_pps;
 
+/* Worst case delay (in ms) it might take before statistics of offloaded flows
+ * are updated. Offloaded flows younger than this delay will always be
+ * revalidated regardless of ofproto_min_revalidate_pps. */
+extern unsigned ofproto_offloaded_stats_delay;
+
 /* Number of upcall handler and revalidator threads. Only affects the
  * ofproto-dpif implementation. */
 extern uint32_t n_handlers, n_revalidators;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 863b34d25..11cc0c6f6 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -311,6 +311,7 @@ unsigned ofproto_flow_limit = OFPROTO_FLOW_LIMIT_DEFAULT;
 unsigned ofproto_max_idle = OFPROTO_MAX_IDLE_DEFAULT;
 unsigned ofproto_max_revalidator = OFPROTO_MAX_REVALIDATOR_DEFAULT;
 unsigned ofproto_min_revalidate_pps = OFPROTO_MIN_REVALIDATE_PPS_DEFAULT;
+unsigned ofproto_offloaded_stats_delay = OFPROTO_OFFLOADED_STATS_DELAY;
 
 uint32_t n_handlers, n_revalidators;
 
@@ -727,6 +728,15 @@ ofproto_set_min_revalidate_pps(unsigned min_revalidate_pps)
     ofproto_min_revalidate_pps = min_revalidate_pps;
 }
 
+/* Set worst case delay (in ms) it might take before statistics of offloaded
+ * flows are updated. Offloaded flows younger than this delay will always be
+ * revalidated regardless of ofproto_min_revalidate_pps. */
+void
+ofproto_set_offloaded_stats_delay(unsigned offloaded_stats_delay)
+{
+    ofproto_offloaded_stats_delay = offloaded_stats_delay;
+}
+
 /* If forward_bpdu is true, the NORMAL action will forward frames with
  * reserved (e.g. STP) destination Ethernet addresses. if forward_bpdu is false,
  * the NORMAL action will drop these frames. */
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index c79f372bc..8efdb20a0 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -320,6 +320,7 @@ int ofproto_port_dump_done(struct ofproto_port_dump *);
 #define OFPROTO_MAX_IDLE_DEFAULT 10000 /* ms */
 #define OFPROTO_MAX_REVALIDATOR_DEFAULT 500 /* ms */
 #define OFPROTO_MIN_REVALIDATE_PPS_DEFAULT 5
+#define OFPROTO_OFFLOADED_STATS_DELAY 2000 /* ms */
 
 const char *ofproto_port_open_type(const struct ofproto *,
                                    const char *port_type);
@@ -349,6 +350,7 @@ void ofproto_set_flow_limit(unsigned limit);
 void ofproto_set_max_idle(unsigned max_idle);
 void ofproto_set_max_revalidator(unsigned max_revalidator);
 void ofproto_set_min_revalidate_pps(unsigned min_revalidate_pps);
+void ofproto_set_offloaded_stats_delay(unsigned offloaded_stats_delay);
 void ofproto_set_forward_bpdu(struct ofproto *, bool forward_bpdu);
 void ofproto_set_mac_table_config(struct ofproto *, unsigned idle_time,
                                   size_t max_entries);