On 3 Jan 2023, at 13:20, Ilya Maximets wrote:
> On 1/3/23 12:33, Eelco Chaudron wrote: >> Depending on the driver implementation it can take up to 2 seconds before >> offloaded flow statistics are updated. 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 before assuming no packets >> where received during this period. >> >> Signed-off-by: Eelco Chaudron <[email protected]> >> --- >> >> Changes: >> - v2: Use existing ukey->offloaded so the revalidate_missed_dp_flow case is >> also covered. >> >> ofproto/ofproto-dpif-upcall.c | 25 +++++++++++++++---------- >> 1 file changed, 15 insertions(+), 10 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index ad9635496..c395adbc6 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -2094,10 +2094,11 @@ 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, struct udpif_key *ukey, >> + uint64_t packets) >> { >> long long int metric, now, duration; >> + long long int used = ukey->stats.used; >> >> if (!used) { >> /* Always revalidate the first time a flow is dumped. */ >> @@ -2124,8 +2125,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 < 2000)) { >> + /* The flow is receiving more than min-revalidate-pps, so keep it. >> + * Or it's a hardware offloaded flow that might take up to 2 seconds >> + * to update its statistics. Until we are sure the statistics had a >> + * chance to be updated, also keep it. */ > > Hmm. If you know that a flow dump is taking a long time on this setup, > shouldn't we just bump the max-revalidator value in the database instead > of hardcoding yet another magic constant? Don’t think increasing the max-revalidator value is a good solution, as it affects the statistics gathering in general. It also does not solve the problem, as any update in the flow table/config can trigger a revalidation. If we do not like the magic static values we can add a configuration option, so it can be adjusted based on the driver implementation. > How long the dump actually takes? And how many flows in it? William do you have these values, as I did not capture any of this when debugging your setup. //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
