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/[email protected]/

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.

We fixed incorrect stats on flow modification for dpdk offload provider
previously here:
  
https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/

Do we need something similar for tc?

best regards, Ilya Maximets.

> 
> push_dp_ops() will now handle updating the stats similar to
> the way it's handled in revalidate_ukey().
> 
> Signed-off-by: Eelco Chaudron <[email protected]>
> ---
>  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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to