[ovs-dev] [PATCH] ofproto: Fix statistics of datapath operations.

Ilya Maximets i.maximets at ovn.org
Wed Oct 14 21:57:34 UTC 2020


On 10/14/20 3:19 PM, Stokes, Ian wrote:
>> If 'stats->n_packets' is less than 'op->ukey->stats.n_packets',
>> the calculation result will be wrong.
>> 'stats->n_bytes' is the same.
>>
> 
> Thanks for the patch, seems like a reasonable change, also I think this could be backported as far as OVS 2.6.
> One thing is could do with a fixes tag to reference commit that introduced the issue.

I'm trying to understand how that issue happens in the first place.
For this to happen statistics stored in ukey should be newer than
the statistics retrieved from the datapath, but this should not
happen under normal conditions.

Another possibility is that datapath statistics were flushed somehow
between two calls to retrieve it.  I'm aware of one of the possible
root causes for this scenario and the patch for it is under review
right now:
  https://patchwork.ozlabs.org/project/openvswitch/patch/20201012142735.5304-1-elibr@nvidia.com/
This case is HW offloading with netdev-offload-dpdk.  Was it the case
where this issue was originally observed?

In general, I do not feel comfortable merging this change until
I do not understand the root cause because we're likely hiding
the issue of underlying datapath with it.

Best regards, Ilya Maximets.

> 
> Acked-by: Ian Stokes <ian.stokes at intel.com>
>  
>> Signed-off-by: zhaozhanxu <zhaozhanxu at 163.com>
>> ---
>>  ofproto/ofproto-dpif-upcall.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 5e08ef10d..80678f843 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -2385,8 +2385,12 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
>> *ops, size_t n_ops)
>>              transition_ukey(op->ukey, UKEY_EVICTED);
>>              push->used = MAX(stats->used, op->ukey->stats.used);
>>              push->tcp_flags = stats->tcp_flags | op->ukey->stats.tcp_flags;
>> -            push->n_packets = stats->n_packets - op->ukey->stats.n_packets;
>> -            push->n_bytes = stats->n_bytes - op->ukey->stats.n_bytes;
>> +            push->n_packets = (stats->n_packets > op->ukey->stats.n_packets
>> +                               ? stats->n_packets - op->ukey->stats.n_packets
>> +                               : 0);
>> +            push->n_bytes = (stats->n_bytes > op->ukey->stats.n_bytes
>> +                             ? stats->n_bytes - op->ukey->stats.n_bytes
>> +                             : 0);
>>              ovs_mutex_unlock(&op->ukey->mutex);
>>          } else {
>>              push = stats;
>> --
>> 2.20.1



More information about the dev mailing list