diff mbox series

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

Message ID 20200602075036.78112-1-zhaozhanxu@163.com
State New
Headers show
Series [ovs-dev] ofproto: Fix statistics of datapath operations. | expand

Commit Message

zhaozhanxu June 2, 2020, 7:50 a.m. UTC
If 'stats->n_packets' is less than 'op->ukey->stats.n_packets',
the calculation result will be wrong.
'stats->n_bytes' is the same.

Signed-off-by: zhaozhanxu <zhaozhanxu@163.com>
---
 ofproto/ofproto-dpif-upcall.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Ian Stokes Oct. 14, 2020, 1:19 p.m. UTC | #1
> 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.

Acked-by: Ian Stokes <ian.stokes@intel.com>
 
> Signed-off-by: zhaozhanxu <zhaozhanxu@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
> 
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Fabrizio D'Angelo Oct. 14, 2020, 7:58 p.m. UTC | #2
I believe this is the commit that introduced the issue:
13bb6ed00c10ae46b3f9b92d05edb41398714166


On Wed, Oct 14, 2020 at 9:19 AM Stokes, Ian <ian.stokes@intel.com> 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.
>
> Acked-by: Ian Stokes <ian.stokes@intel.com>
>
> > Signed-off-by: zhaozhanxu <zhaozhanxu@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
> >
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ilya Maximets Oct. 14, 2020, 9:57 p.m. UTC | #3
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@intel.com>
>  
>> Signed-off-by: zhaozhanxu <zhaozhanxu@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
diff mbox series

Patch

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;