On 2/27/23 15:58, Eelco Chaudron wrote: > 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.hor...@corigine.com> > Signed-off-by: Eelco Chaudron <echau...@redhat.com> > --- > > 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 to latest upstream master. > Made the key parameter const in should_revalidate(). > > ofproto/ofproto-dpif-upcall.c | 26 ++++++++++++++++---------- > ofproto/ofproto-provider.h | 4 ++++ > ofproto/ofproto.c | 9 +++++++++ > ofproto/ofproto.h | 2 ++ > vswitchd/bridge.c | 3 +++ > vswitchd/vswitch.xml | 12 ++++++++++++ > 6 files changed, 46 insertions(+), 10 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index fc94078cb..60c273a1d 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -2100,10 +2100,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; > @@ -2134,8 +2136,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; > @@ -2339,7 +2345,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; > @@ -2358,7 +2364,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key > *ukey, > : 0); > > 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 { > @@ -2374,7 +2380,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; > } > @@ -2774,6 +2780,7 @@ revalidate(struct revalidator *revalidator) > continue; > } > > + ukey->offloaded = f->attrs.offloaded; > already_dumped = ukey->dump_seq == dump_seq; > if (already_dumped) { > /* The flow has already been handled during this flow dump > @@ -2805,8 +2812,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; > > @@ -2891,7 +2897,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..71341a01f 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -541,6 +541,10 @@ 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. This is used when calculating the min revalidate pps. */
This comment is confusing, because 'min revalidate pps' is a configuration option and we do not calculate it. We should say 'Offloaded flows younger than this delay will always be revalidated regardless of ofproto_min_revalidate_pps'. Or something along these lines. > +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..69fece4c5 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,14 @@ 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. This is used when calculating the min revalidate pps. > */ Same here. > +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 4e15167ab..fa7973ac7 100644 > --- a/ofproto/ofproto.h > +++ b/ofproto/ofproto.h > @@ -311,6 +311,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); > @@ -340,6 +341,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 abf2afe57..c35c16c8d 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 05ac1fbe5..fde19838d 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -216,6 +216,18 @@ > </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. This is used when calculating the > + min-revalidate-pps. Same here. Nit: Please use xml markup to refer options. i.e.: <ref column="other_config" key="min-revalidate-pps"/> Would still be great if we didn't need this patch at all, but I guess we stuck with it for now. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev