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.
> 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.
>
>>>>>> 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.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
