On 08/12/2022 1:25, Balazs Nemeth wrote:
> Besides the unrealistic case of an overflow, stats->n_packets can become
> less than ukey->stats.n_packets only when incorrect statistics were
> read. For that reason, the condition that the code employed is a
> workaround to avoid large jumps in statistics simply by assuming no
> packets were read. However, if this happens while the revalidator is
> under heavy load, the workaround has an unintended side effect where
> should_revalidate returns false because the metric it calculates is
> based on a bogus value.
>
> Therefore, this patch makes it explicit that the workaround has been
> triggered and consequently skipping should_revalidate assuming it should
> have returned true. Note also that the workaround is only triggered
> if stats->n_packets is _strictly_ less than ukey->stats.n_packets. Not
> receiving any packets is a valid scenario where should_revalidate should
> not be skipped.
>
> Signed-off-by: Balazs Nemeth <[email protected]>
> ---
> ofproto/ofproto-dpif-upcall.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index ad9635496..f0078e3c1 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2327,6 +2327,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
> OVS_REQUIRES(ukey->mutex)
> {
> bool need_revalidate = ukey->reval_seq != reval_seq;
> + bool stats_workaround;
> enum reval_result result = UKEY_DELETE;
> struct dpif_flow_stats push;
>
> @@ -2334,7 +2335,8 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
>
> push.used = stats->used;
> push.tcp_flags = stats->tcp_flags;
> - push.n_packets = (stats->n_packets > ukey->stats.n_packets
> + stats_workaround = stats->n_packets < ukey->stats.n_packets;
> + push.n_packets = (!stats_workaround
> ? stats->n_packets - ukey->stats.n_packets
> : 0);
> push.n_bytes = (stats->n_bytes > ukey->stats.n_bytes
> @@ -2342,7 +2344,8 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
> : 0);
>
> if (need_revalidate) {
> - if (should_revalidate(udpif, push.n_packets, ukey->stats.used)) {
> + if (stats_workaround ||
> + should_revalidate(udpif, push.n_packets, ukey->stats.used)) {
> if (!ukey->xcache) {
> ukey->xcache = xlate_cache_new();
> } else {
this actually looks good to me as an improvement to the last patch
to protect against such scenario as described, as we know it happens.
I'm in favor of this.
Acked-by: Roi Dayan <[email protected]>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
