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 <[email protected]> Signed-off-by: Eelco Chaudron <[email protected]> --- Changes: - v2: Use existing ukey->offloaded so the revalidate_missed_dp_flow case is also covered. - v3: Added OVS_REQUIRES(ukey->mutex) to should_revalidate() to keep thread-safety-analysis happy. - v4: Add a configurable option. After looking at multiple vendor implementation for both TC and rte_flow I came to the conclusion that the delay is roughly between 0.2 and 2 seconds. Updated commit message. - v5: Rebased on latest upstream master. Made the key parameter const in should_revalidate(). - v6: Updated comments and documentation to be more clear. - v7: Rebased on latest OVS master. ofproto/ofproto-dpif-upcall.c | 25 +++++++++++++++---------- ofproto/ofproto-provider.h | 5 +++++ ofproto/ofproto.c | 10 ++++++++++ ofproto/ofproto.h | 2 ++ vswitchd/bridge.c | 3 +++ vswitchd/vswitch.xml | 13 +++++++++++++ 6 files changed, 48 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); diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 307a51527..f5dc59ad0 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -832,6 +832,9 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) ofproto_set_min_revalidate_pps( smap_get_uint(&ovs_cfg->other_config, "min-revalidate-pps", OFPROTO_MIN_REVALIDATE_PPS_DEFAULT)); + ofproto_set_offloaded_stats_delay( + smap_get_uint(&ovs_cfg->other_config, "offloaded-stats-delay", + OFPROTO_OFFLOADED_STATS_DELAY)); ofproto_set_vlan_limit(smap_get_int(&ovs_cfg->other_config, "vlan-limit", LEGACY_MAX_VLAN_HEADERS)); ofproto_set_bundle_idle_timeout(smap_get_uint(&ovs_cfg->other_config, diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 12708a313..3e94b969c 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -216,6 +216,19 @@ </p> </column> + <column name="other_config" key="offloaded-stats-delay" + type='{"type": "integer", "minInteger": 0}'> + <p> + 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 + <ref column="other_config" key="min-revalidate-pps"/>. + </p> + <p> + The default is 2000. + </p> + </column> + <column name="other_config" key="hw-offload" type='{"type": "boolean"}'> <p> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
