| Message ID | 20200602075036.78112-1-zhaozhanxu@163.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ofproto: Fix statistics of datapath operations. | expand |
> 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
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 >
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 --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;
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(-)