On 17 Feb 2022, at 14:10, Ilya Maximets wrote:
> On 1/31/22 11:54, Eelco Chaudron wrote: >> Make sure to only update packet and byte counters when valid, >> or else this could lead to "temporarily/occasionally" >> out-of-sync flow counters. > > There was already the same patch submitted here: > https://patchwork.ozlabs.org/project/openvswitch/patch/20200602075036.78112-1-zhaozha...@163.com/ > > And I'm still not comfortable with the change, because it seems > like it only hides the underlying datapath problem. Do you know > why exactly datapath stats become lower than previously reported? > > If it's some kind of a statistics flush, that will mean that flow > statistics will not be updated until new stats will catch up to the > old value leading to the flow revalidation and incorrect flow stats > anyway. This was very hard to reproduce, only once out of 20-30 runs if I remember correctly. Without the patch, it would sometimes show very high numbers and then got updated after a while with the correct numbers. At least this is what I remember, as I did not take any notes, and my brain wanted to forget this patchset :) Guess the fix is needed anyway as this behavior was there since day one in revalidate_ukey(), e79a6c833. I also see that you got no reply to your comments, so I’ll take another stab to make sure this patch is really fixing the problem, or fixing a problem, and hiding another TC problem. > We fixed incorrect stats on flow modification for dpdk offload provider > previously here: > https://patchwork.ozlabs.org/project/openvswitch/patch/20201012142735.5304-1-el...@nvidia.com/ > > Do we need something similar for tc? I’ll take a close look at the DPDK patch, once I get my reproducer going. >> push_dp_ops() will now handle updating the stats similar to >> the way it's handled in revalidate_ukey(). >> >> Signed-off-by: Eelco Chaudron <echau...@redhat.com> >> --- >> ofproto/ofproto-dpif-upcall.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index 1c9c720f0..ca43ac083 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -2432,8 +2432,12 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, >> size_t n_ops) >> transition_ukey(op->ukey, UKEY_EVICTED); >> push->used = MAX(stats->used, op->ukey->stats.used); >> push->tcp_flags = stats->tcp_flags | op->ukey->stats.tcp_flags; >> - push->n_packets = stats->n_packets - op->ukey->stats.n_packets; >> - push->n_bytes = stats->n_bytes - op->ukey->stats.n_bytes; >> + push->n_packets = (stats->n_packets > op->ukey->stats.n_packets >> + ? stats->n_packets - >> op->ukey->stats.n_packets >> + : 0); >> + push->n_bytes = (stats->n_bytes > op->ukey->stats.n_bytes >> + ? stats->n_bytes - op->ukey->stats.n_bytes >> + : 0); >> ovs_mutex_unlock(&op->ukey->mutex); >> } else { >> push = stats; >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
