On 24 May 2023, at 21:05, Ilya Maximets wrote:
> 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 <bnem...@redhat.com>
>> ---
>> 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.
I guess this should be before the odp_flow_key_format() function, missed it
when reviewing :( The same as other logs do.
>> +
>> + 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
