On Thu, Mar 30, 2023 at 09:04:02PM +0200, Ilya Maximets wrote:
> On 3/30/23 11:45, Simon Horman wrote:
> > On Fri, Mar 17, 2023 at 09:47:36PM +0100, Eelco Chaudron wrote:
> >>
> >>
> >> Send from my phone
> >>
> >>> Op 17 mrt. 2023 om 21:11 heeft Marcelo Ricardo Leitner
> >>> <mleit...@redhat.com> het volgende geschreven:
> >>>
> >>> On Thu, Mar 16, 2023 at 09:51:34AM +0100, Eelco Chaudron wrote:
> >>>>
> >>>>
> >>>>> On 22 Dec 2022, at 13:32, Eelco Chaudron wrote:
> >>>>>
> >>>>>> On 22 Dec 2022, at 10:26, Balazs Nemeth wrote:
> >>>>>
> >>>>>> The only way that stats->{n_packets,n_bytes} would decrease is due to
> >>>>>> an
> >
> > nit: s/way/ways/
> >
> >>>>>> overflow, or if there are bugs in how statistics are handled. In the
> >>>>>> past, there were multiple bugs 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.
> >
> > 1. Perhaps it would be prudent to log/count if/when this occurs
>
> +1
> We do have a coverage counter that will indicate the case where stats
> jump back. However, if we're certain that this should never happen,
> we should, probably, emit a warning or even an error log as well, so
> users are aware that something went wrong.
I was thinking more of a counter, which seems to already be covered.
But I have no objection to your reasoning about having a warning (too).
>
> > 2. I take it that the overflow handling would be follow-up work,
> > is that correct?
>
> The unsigned arithmetic should take case of overflowed counters,
> because the result of subtraction will still give a correct difference
> between the old and a new value, even if it overflowed and the new
> value is smaller. Unless, of course, it overflowed more than once.
More than once between samples?
If so, I'm assuming that is not a case we can hit unless there is a bug.
> >>>>>> Signed-off-by: Balazs Nemeth <bnem...@redhat.com>
> >>>>>> ---
> >>>>>>
> >>>>>> Depends on:
> >>>>>> - netdev-offload-tc: Preserve tc statistics when flow gets modified.
> >>>>>> - tc: Add support for TCA_STATS_PKT64
> >>>>>> - ofproto-dpif-upcall: Wait for valid hw flow stats before applying
> >>>>>> min-revalidate-pps
> >>>>>> - ofproto-dpif-upcall: Use last known stats ukey stats on revalidate
> >>>>>> missed dp flows.
> >>>>>
> >>>>> Did a lot of tests with this patch applied, but as mentioned it does
> >>>>> need all four patches mentioned above applied first.
> >>>>>
> >>>>> Acked-by: Eelco Chaudron <echau...@redhat.com>
> >>>>
> >>>> Now that all the dependent patches have been applied and backported to
> >>>> 2.17, can we have another look at this patch?
> >>>
> >>> Not sure what you mean here, Eelco. You mean, just a patch review or
> >>> you mean something else, like evaluating if the patch is still needed
> >>> at all?
> >>
> >> Guess I was not clear enough :) what I meant was for the maintainers to
> >> have another look at this patch and if no further objections apply it.
> >
> > There seems to now be minor fuzz when applying this patch (locally).
> > Perhaps a rebase is in order?
>
> Yeah, the coverage counter got introduced in a close proximity.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
