On 5/23/23 15:11, Balazs Nemeth wrote:
> The only way that stats->{n_packets,n_bytes} would decrease is due to an
> overflow, or if there are bugs in how statistics are handled. In the
> past, there were multiple issues that caused a jump backward. A
> workaround was in place to set the statistics to 0 in that case. When
> this happened while the revalidator was under heavy load, the workaround
> had an unintended side effect where should_revalidate returned false
> causing the flow to be removed because the metric it calculated was
> based on a bogus value. Since many of those bugs have now been
> identified and resolved, there is no need to set the statistics to 0. In
> addition, the (unlikely) overflow still needs to be handled
> appropriately. If an unexpected jump does happen, just log it as a
> warning.
>
> Signed-off-by: Balazs Nemeth <[email protected]>
> ---
> ofproto/ofproto-dpif-upcall.c | 26 ++++++++++++++++++--------
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index cd57fdbd9..b06315d22 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2368,24 +2368,34 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
> enum reval_result result = UKEY_DELETE;
> struct dpif_flow_stats push;
>
> - ofpbuf_clear(odp_actions);
> -
> push.used = stats->used;
> push.tcp_flags = stats->tcp_flags;
> - push.n_packets = (stats->n_packets > ukey->stats.n_packets
> - ? stats->n_packets - ukey->stats.n_packets
> - : 0);
> - push.n_bytes = (stats->n_bytes > ukey->stats.n_bytes
> - ? stats->n_bytes - ukey->stats.n_bytes
> - : 0);
> + push.n_packets = stats->n_packets - ukey->stats.n_packets;
> + push.n_bytes = stats->n_bytes - ukey->stats.n_bytes;
>
> if (stats->n_packets < ukey->stats.n_packets &&
> ukey->stats.n_packets < UINT64_THREE_QUARTERS) {
> + static struct vlog_rate_limit rll = VLOG_RATE_LIMIT_INIT(1, 5);
> + struct ds ds = DS_EMPTY_INITIALIZER;
> +
> /* Report cases where the packet counter is lower than the previous
> * instance, but exclude the potential wrapping of an uint64_t. */
> COVERAGE_INC(ukey_invalid_stat_reset);
> +
> + odp_flow_key_format(ukey->key, ukey->key_len, &ds);
> + ds_put_cstr(&ds, ", actions:");
> + format_odp_actions(&ds, odp_actions->data, odp_actions->size, NULL);
The 'odp_actions' here is a scratch pad for the processing below,
it doesn't contain any actual actions for this ukey.
We should get real actions from the ukey->actions with rcu_get()
and print them out instead, e.g.:
actions = ovsrcu_get(struct ofpbuf *, &ukey->actions);
format_odp_actions(&ds, actions->data, actions->size, NULL);
> + odp_format_ufid(&ukey->ufid, &ds);
> + ds_put_cstr(&ds, " ");
Above two lines should be in the opposite order, otherwise ufid is
glued to the actions.
> +
> + VLOG_WARN_RL(&rll, "Unexpected jump in packet stats from %"PRIu64
> + " to %"PRIu64" when handling ukey %s",
> + stats->n_packets, ukey->stats.n_packets, ds_cstr(&ds));
> + ds_destroy(&ds);
> }
>
> + ofpbuf_clear(odp_actions);
No need to move this.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
