Hi Ilya and Eelco, I have seen udpif->dump_duration in revalidate_ukey() go as high as ~2 seconds . But the dump duration is very random and sporadic. Most likely a function of how busy the CPU is.
We have tried a max-revalidator time of 1 second and we saw connectivity losses presumably due to the flows being deleted in the "else" case for should_revalidate() call in the revalidate_ukey() function. Taking into consideration the Nvidia MLX driver for instance: https://ansymbol.com/linux/v5.14/source/drivers/net/ethernet/mellanox/mlx5/core/fs_counters.c#L40 . We can see that they poll for stats every 1 second. But definitely could be delayed. So the update period of hardware stats is at a minimum every 1 second for MLX NICs. Thanks, William Zhao On Tue, Jan 3, 2023 at 2:22 PM Ilya Maximets <[email protected]> wrote: > > On 1/3/23 13:58, Eelco Chaudron wrote: > > > > > > On 3 Jan 2023, at 13:36, Ilya Maximets wrote: > > > >> On 1/3/23 13:26, Eelco Chaudron wrote: > >>> > >>> > >>> 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 affects the judgement on the dump duration and the > >> time when revalidators will wake up next time. If your > >> dump duration is already high, your statistics gathering > >> is already delayed. And your hardware doesn't seem to > >> provide statistics in any timely manner as well. > > > > Well, this is true if the delay is constant, but this is from the last run, > > and not all runs take the same amount :( Also, there is a mix of hardware > > and kernel flows, where kernel flows do provide real-time statistics vs max > > of 2 seconds delayed by HW offload. > > > > Take the following example; we just added some flows, which will trigger > > multiple updates back to back. So let’s say it takes 251ms to do the update > > (way under the 1.x seconds the hardware counter is updated). > > > > The second iteration would purge all HW offloaded flows, as no counter was > > updated in the 251ms window :( > > > > To solve this we could set the max-revalidator time to 4 seconds, but this > > will negatively affect all statistics gathering for flows. And maybe this > > might also have some other side effects in existing configurations. > > If the dump takes 251ms, you only need to bump max-revalidator to 504ms, > not 4 seconds. We should not mix up the time it takes to update the > statistics and the dump duration. > > Flow being revalidated doesn't mean it will be deleted. The flow will > be deleted during revalidation only if it is actually incorrect. > > If it is idle for 10 seconds, it will be deleted without even considering > revalidation. This is controlled by the max-idle config. > > In your case flows didn't reach max-idle yet, but removed *without* > revalidation. This happens because OVS thinks that it's too expensive > to trigger revalidation based on the flow dump duration. > This is controlled by the max-revalidator config. > > If your actual dump takes 251ms, but max-revalidator is 504ms, the > flow should not be removed even if it has zero stats for a next few > seconds. As long as the flow itself is correct. > > That's why we need to know what is the actual dump duration here. > > > > >>> It also does not solve the problem, as any update in the flow > >>> table/config can > >>> trigger a revalidation. > >> > >> It will, but there is a check right above the one that > >> you're changing and it looks like this: > >> > >> if (udpif->dump_duration < ofproto_max_revalidator / 2) { > >> /* We are likely to handle full revalidation for the flows. */ > >> return true; > >> } > >> > >> So, by bumping the max-revalidator value we will avoid > >> blind deletion and will properly revalidate the flow. > >> The flow will not be deleted then, unless we hit the max-idle. > > > > See above. > > > >> Or am I missing something? > >> > >>> > >>> 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
