diff mbox series

[ovs-dev] 答复: [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats

Message ID 172e1abc63cc4481ab1ee0ae82051448@jd.com
State New
Headers show
Series [ovs-dev] 答复: [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats | expand

Commit Message

姜立东 April 27, 2020, 2 a.m. UTC
Hi William,

Thanks for your comments. 
Agree with you, those counters such as collisions and multicast should be kept as current implementation, 
since they are don't provided by vport stats. 

I also suggest to remove rx/tx bytes and packets, rx/tx error and drops as well, only count physical or hardware stats in,
because it is confusing when checking with netdev_stats_from_ovs_vport_stats, if netdev_stats_from_ovs_vport_stats 
has provided them, why they are copied again.

What do you think about change as below? 



> +
>      ovs_mutex_unlock(&netdev->mutex);
>  
>      return error;
> --
> 1.8.3.1
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Comments

William Tu April 27, 2020, 4:19 a.m. UTC | #1
On Mon, Apr 27, 2020 at 02:00:38AM +0000, 姜立东 wrote:
> Hi William,
> 
> Thanks for your comments. 
> Agree with you, those counters such as collisions and multicast should be kept as current implementation, 
> since they are don't provided by vport stats. 
> 
> I also suggest to remove rx/tx bytes and packets, rx/tx error and drops as well, only count physical or hardware stats in,
> because it is confusing when checking with netdev_stats_from_ovs_vport_stats, if netdev_stats_from_ovs_vport_stats 
> has provided them, why they are copied again.
> 
> What do you think about change as below? 
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index ff045cb..b851236 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2208,18 +2208,6 @@ netdev_linux_get_stats(const struct netdev *netdev_,
>          /* stats not available from OVS then use netdev stats. */
>          *stats = dev_stats;
>      } else {
> -        /* Use kernel netdev's packet and byte counts since vport's counters
> -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> -         * enabled. */
But the comments above says the reason using the counter below.
> -        stats->rx_packets = dev_stats.rx_packets;
> -        stats->rx_bytes = dev_stats.rx_bytes;
> -        stats->tx_packets = dev_stats.tx_packets;
> -        stats->tx_bytes = dev_stats.tx_bytes;
> -

How about we just remove the below 4 stats?
> -        stats->rx_errors           += dev_stats.rx_errors;
> -        stats->tx_errors           += dev_stats.tx_errors;
> -        stats->rx_dropped          += dev_stats.rx_dropped;
> -        stats->tx_dropped          += dev_stats.tx_dropped;
>          stats->multicast           += dev_stats.multicast;
>          stats->collisions          += dev_stats.collisions;
>          stats->rx_length_errors    += dev_stats.rx_length_errors;
> 
> BR,
> Lidong
> 
> -----邮件原件-----
> 发件人: William Tu <u9012063@gmail.com> 
> 发送时间: 2020年4月25日 22:31
> 收件人: 姜立东 <jianglidong3@jd.com>
> 抄送: dev@openvswitch.org
> 主题: Re: [ovs-dev] [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats
> 
> On Thu, Apr 23, 2020 at 05:35:14AM +0000, 姜立东 via dev wrote:
> > From df9ff3b67f11e467928ca0873031d81b87f3d0c5 Mon Sep 17 00:00:00 2001
> > From: Jiang Lidong <jianglidong3@jd.com>
> > Date: Thu, 23 Apr 2020 11:07:28 +0800
> > Subject: [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats
> >     When using kernel veth as OVS interface, doubled drop counter
> >     value is shown when veth drops packets due to traffic overrun.
> > 
> >     In netdev_linux_get_stats, it reads both vport stats and kernel
> >     netdev stats, in case vport stats retrieve failure. If both of
> >     them success, error counters are added to include errors from
> >     different layers. But implementation of ovs_vport_get_stats in
> >     kernel data path has included kernel netdev stats by calling
> >     dev_get_stats. When drop or other error counters is not zero,
> >     its value is doubled by netdev_linux_get_stats.
> > 
> >     In this change, adding kernel netdev stats into vport stats
> >     is removed, since vport stats includes all information of
> >     kernel netdev stats.
> > 
> > Signed-off-by: Jiang Lidong <jianglidong3@jd.com>
> > ---
> >  lib/netdev-linux.c | 27 +--------------------------
> >  1 file changed, 1 insertion(+), 26 deletions(-)
> > 
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 
> > ff045cb..6d139d0 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -2207,33 +2207,8 @@ netdev_linux_get_stats(const struct netdev *netdev_,
> >      } else if (netdev->vport_stats_error) {
> >          /* stats not available from OVS then use netdev stats. */
> >          *stats = dev_stats;
> > -    } else {
> > -        /* Use kernel netdev's packet and byte counts since vport's counters
> > -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> > -         * enabled. */
> > -        stats->rx_packets = dev_stats.rx_packets;
> > -        stats->rx_bytes = dev_stats.rx_bytes;
> > -        stats->tx_packets = dev_stats.tx_packets;
> > -        stats->tx_bytes = dev_stats.tx_bytes;
> > -
> > -        stats->rx_errors           += dev_stats.rx_errors;
> > -        stats->tx_errors           += dev_stats.tx_errors;
> > -        stats->rx_dropped          += dev_stats.rx_dropped;
> > -        stats->tx_dropped          += dev_stats.tx_dropped;
> 
> Thanks for reporting the issue, looking at
>   get_stats_via_vport
>     get_stats_via_vport__
>       netdev_stats_from_ovs_vport_stats
>          // only set the ovs_vport_stats, which has 8 fields above
> 
> But with your patch it will remove all the reset of stats below
> 
> > -        stats->multicast           += dev_stats.multicast;
> > -        stats->collisions          += dev_stats.collisions;
> > -        stats->rx_length_errors    += dev_stats.rx_length_errors;
> > -        stats->rx_over_errors      += dev_stats.rx_over_errors;
> > -        stats->rx_crc_errors       += dev_stats.rx_crc_errors;
> > -        stats->rx_frame_errors     += dev_stats.rx_frame_errors;
> > -        stats->rx_fifo_errors      += dev_stats.rx_fifo_errors;
> > -        stats->rx_missed_errors    += dev_stats.rx_missed_errors;
> > -        stats->tx_aborted_errors   += dev_stats.tx_aborted_errors;
> > -        stats->tx_carrier_errors   += dev_stats.tx_carrier_errors;
> > -        stats->tx_fifo_errors      += dev_stats.tx_fifo_errors;
> > -        stats->tx_heartbeat_errors += dev_stats.tx_heartbeat_errors;
> > -        stats->tx_window_errors    += dev_stats.tx_window_errors;
> >      }
> 
> 
> How about we just over write the error and drop counts?
>  
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index c6e46f1885aa..c169c52611c4 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2220,10 +2220,11 @@ netdev_linux_get_stats(const struct netdev *netdev_,
>          stats->tx_packets = dev_stats.tx_packets;
>          stats->tx_bytes = dev_stats.tx_bytes;
>  
> -        stats->rx_errors           += dev_stats.rx_errors;
> -        stats->tx_errors           += dev_stats.tx_errors;
> -        stats->rx_dropped          += dev_stats.rx_dropped;
> -        stats->tx_dropped          += dev_stats.tx_dropped;
> +        stats->rx_errors           = dev_stats.rx_errors;
> +        stats->tx_errors           = dev_stats.tx_errors;
> +        stats->rx_dropped          = dev_stats.rx_dropped;
> +        stats->tx_dropped          = dev_stats.tx_dropped;
> +
>          stats->multicast           += dev_stats.multicast;
>          stats->collisions          += dev_stats.collisions;
>          stats->rx_length_errors    += dev_stats.rx_length_errors;
> 
> 
> > +
> >      ovs_mutex_unlock(&netdev->mutex);
> >  
> >      return error;
> > --
> > 1.8.3.1
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
姜立东 April 28, 2020, 3:29 a.m. UTC | #2
Hi William, 

> -        /* Use kernel netdev's packet and byte counts since vport's counters
> -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> -         * enabled. */

Actually I think it should be moved to netdev_stats_from_ovs_vport_stats :), that explains what netdev_stats_from_ovs_vport_stats is doing.
In fact, I think better solution is copying all physical abnormal counters in netdev_stats_from_ovs_vport_stats , 
and remove this copy from netdev_linux_get_stats. 
but netdev_stats_from_ovs_vport_stats is in kernel module that may not be upgraded in some application scenarios. 
So I moved to remove unnecessary copies, such as RX/TX bytes, packets, drops.

Regards,
Lidong

-----邮件原件-----
发件人: William Tu <u9012063@gmail.com> 
发送时间: 2020年4月27日 12:20
收件人: 姜立东 <jianglidong3@jd.com>
抄送: dev@openvswitch.org
主题: Re: 答复: [ovs-dev] [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats

On Mon, Apr 27, 2020 at 02:00:38AM +0000, 姜立东 wrote:
> Hi William,
> 
> Thanks for your comments. 
> Agree with you, those counters such as collisions and multicast should 
> be kept as current implementation, since they are don't provided by vport stats.
> 
> I also suggest to remove rx/tx bytes and packets, rx/tx error and 
> drops as well, only count physical or hardware stats in, because it is 
> confusing when checking with netdev_stats_from_ovs_vport_stats, if netdev_stats_from_ovs_vport_stats has provided them, why they are copied again.
> 
> What do you think about change as below? 
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 
> ff045cb..b851236 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2208,18 +2208,6 @@ netdev_linux_get_stats(const struct netdev *netdev_,
>          /* stats not available from OVS then use netdev stats. */
>          *stats = dev_stats;
>      } else {
> -        /* Use kernel netdev's packet and byte counts since vport's counters
> -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> -         * enabled. */
But the comments above says the reason using the counter below.
> -        stats->rx_packets = dev_stats.rx_packets;
> -        stats->rx_bytes = dev_stats.rx_bytes;
> -        stats->tx_packets = dev_stats.tx_packets;
> -        stats->tx_bytes = dev_stats.tx_bytes;
> -

How about we just remove the below 4 stats?
> -        stats->rx_errors           += dev_stats.rx_errors;
> -        stats->tx_errors           += dev_stats.tx_errors;
> -        stats->rx_dropped          += dev_stats.rx_dropped;
> -        stats->tx_dropped          += dev_stats.tx_dropped;
>          stats->multicast           += dev_stats.multicast;
>          stats->collisions          += dev_stats.collisions;
>          stats->rx_length_errors    += dev_stats.rx_length_errors;
> 
> BR,
> Lidong
> 
> -----邮件原件-----
> 发件人: William Tu <u9012063@gmail.com>
> 发送时间: 2020年4月25日 22:31
> 收件人: 姜立东 <jianglidong3@jd.com>
> 抄送: dev@openvswitch.org
> 主题: Re: [ovs-dev] [PATCH] netdev-linux: remove sum of vport stats and 
> kernel netdev stats
> 
> On Thu, Apr 23, 2020 at 05:35:14AM +0000, 姜立东 via dev wrote:
> > From df9ff3b67f11e467928ca0873031d81b87f3d0c5 Mon Sep 17 00:00:00 
> > 2001
> > From: Jiang Lidong <jianglidong3@jd.com>
> > Date: Thu, 23 Apr 2020 11:07:28 +0800
> > Subject: [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats
> >     When using kernel veth as OVS interface, doubled drop counter
> >     value is shown when veth drops packets due to traffic overrun.
> > 
> >     In netdev_linux_get_stats, it reads both vport stats and kernel
> >     netdev stats, in case vport stats retrieve failure. If both of
> >     them success, error counters are added to include errors from
> >     different layers. But implementation of ovs_vport_get_stats in
> >     kernel data path has included kernel netdev stats by calling
> >     dev_get_stats. When drop or other error counters is not zero,
> >     its value is doubled by netdev_linux_get_stats.
> > 
> >     In this change, adding kernel netdev stats into vport stats
> >     is removed, since vport stats includes all information of
> >     kernel netdev stats.
> > 
> > Signed-off-by: Jiang Lidong <jianglidong3@jd.com>
> > ---
> >  lib/netdev-linux.c | 27 +--------------------------
> >  1 file changed, 1 insertion(+), 26 deletions(-)
> > 
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index
> > ff045cb..6d139d0 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -2207,33 +2207,8 @@ netdev_linux_get_stats(const struct netdev *netdev_,
> >      } else if (netdev->vport_stats_error) {
> >          /* stats not available from OVS then use netdev stats. */
> >          *stats = dev_stats;
> > -    } else {
> > -        /* Use kernel netdev's packet and byte counts since vport's counters
> > -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> > -         * enabled. */
> > -        stats->rx_packets = dev_stats.rx_packets;
> > -        stats->rx_bytes = dev_stats.rx_bytes;
> > -        stats->tx_packets = dev_stats.tx_packets;
> > -        stats->tx_bytes = dev_stats.tx_bytes;
> > -
> > -        stats->rx_errors           += dev_stats.rx_errors;
> > -        stats->tx_errors           += dev_stats.tx_errors;
> > -        stats->rx_dropped          += dev_stats.rx_dropped;
> > -        stats->tx_dropped          += dev_stats.tx_dropped;
> 
> Thanks for reporting the issue, looking at
>   get_stats_via_vport
>     get_stats_via_vport__
>       netdev_stats_from_ovs_vport_stats
>          // only set the ovs_vport_stats, which has 8 fields above
> 
> But with your patch it will remove all the reset of stats below
> 
> > -        stats->multicast           += dev_stats.multicast;
> > -        stats->collisions          += dev_stats.collisions;
> > -        stats->rx_length_errors    += dev_stats.rx_length_errors;
> > -        stats->rx_over_errors      += dev_stats.rx_over_errors;
> > -        stats->rx_crc_errors       += dev_stats.rx_crc_errors;
> > -        stats->rx_frame_errors     += dev_stats.rx_frame_errors;
> > -        stats->rx_fifo_errors      += dev_stats.rx_fifo_errors;
> > -        stats->rx_missed_errors    += dev_stats.rx_missed_errors;
> > -        stats->tx_aborted_errors   += dev_stats.tx_aborted_errors;
> > -        stats->tx_carrier_errors   += dev_stats.tx_carrier_errors;
> > -        stats->tx_fifo_errors      += dev_stats.tx_fifo_errors;
> > -        stats->tx_heartbeat_errors += dev_stats.tx_heartbeat_errors;
> > -        stats->tx_window_errors    += dev_stats.tx_window_errors;
> >      }
> 
> 
> How about we just over write the error and drop counts?
>  
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 
> c6e46f1885aa..c169c52611c4 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2220,10 +2220,11 @@ netdev_linux_get_stats(const struct netdev *netdev_,
>          stats->tx_packets = dev_stats.tx_packets;
>          stats->tx_bytes = dev_stats.tx_bytes;
>  
> -        stats->rx_errors           += dev_stats.rx_errors;
> -        stats->tx_errors           += dev_stats.tx_errors;
> -        stats->rx_dropped          += dev_stats.rx_dropped;
> -        stats->tx_dropped          += dev_stats.tx_dropped;
> +        stats->rx_errors           = dev_stats.rx_errors;
> +        stats->tx_errors           = dev_stats.tx_errors;
> +        stats->rx_dropped          = dev_stats.rx_dropped;
> +        stats->tx_dropped          = dev_stats.tx_dropped;
> +
>          stats->multicast           += dev_stats.multicast;
>          stats->collisions          += dev_stats.collisions;
>          stats->rx_length_errors    += dev_stats.rx_length_errors;
> 
> 
> > +
> >      ovs_mutex_unlock(&netdev->mutex);
> >  
> >      return error;
> > --
> > 1.8.3.1
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
William Tu April 29, 2020, 6:07 p.m. UTC | #3
On Tue, Apr 28, 2020 at 03:29:10AM +0000, 姜立东 wrote:
> Hi William, 
> 
> > -        /* Use kernel netdev's packet and byte counts since vport's counters
> > -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> > -         * enabled. */
> 
> Actually I think it should be moved to netdev_stats_from_ovs_vport_stats :), that explains what netdev_stats_from_ovs_vport_stats is doing.
> In fact, I think better solution is copying all physical abnormal counters in netdev_stats_from_ovs_vport_stats , 
> and remove this copy from netdev_linux_get_stats. 
> but netdev_stats_from_ovs_vport_stats is in kernel module that may not be upgraded in some application scenarios. 
> So I moved to remove unnecessary copies, such as RX/TX bytes, packets, drops.
> 
> Regards,
> Lidong

oh, I see your point. Then this looks good to me.
Add Pravin to see if he has some comments.

William
> 
> -----邮件原件-----
> 发件人: William Tu <u9012063@gmail.com> 
> 发送时间: 2020年4月27日 12:20
> 收件人: 姜立东 <jianglidong3@jd.com>
> 抄送: dev@openvswitch.org
> 主题: Re: 答复: [ovs-dev] [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats
> 
> On Mon, Apr 27, 2020 at 02:00:38AM +0000, 姜立东 wrote:
> > Hi William,
> > 
> > Thanks for your comments. 
> > Agree with you, those counters such as collisions and multicast should 
> > be kept as current implementation, since they are don't provided by vport stats.
> > 
> > I also suggest to remove rx/tx bytes and packets, rx/tx error and 
> > drops as well, only count physical or hardware stats in, because it is 
> > confusing when checking with netdev_stats_from_ovs_vport_stats, if netdev_stats_from_ovs_vport_stats has provided them, why they are copied again.
> > 
> > What do you think about change as below? 
> > 
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 
> > ff045cb..b851236 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -2208,18 +2208,6 @@ netdev_linux_get_stats(const struct netdev *netdev_,
> >          /* stats not available from OVS then use netdev stats. */
> >          *stats = dev_stats;
> >      } else {
> > -        /* Use kernel netdev's packet and byte counts since vport's counters
> > -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> > -         * enabled. */
> But the comments above says the reason using the counter below.
> > -        stats->rx_packets = dev_stats.rx_packets;
> > -        stats->rx_bytes = dev_stats.rx_bytes;
> > -        stats->tx_packets = dev_stats.tx_packets;
> > -        stats->tx_bytes = dev_stats.tx_bytes;
> > -
> 
> How about we just remove the below 4 stats?
> > -        stats->rx_errors           += dev_stats.rx_errors;
> > -        stats->tx_errors           += dev_stats.tx_errors;
> > -        stats->rx_dropped          += dev_stats.rx_dropped;
> > -        stats->tx_dropped          += dev_stats.tx_dropped;
> >          stats->multicast           += dev_stats.multicast;
> >          stats->collisions          += dev_stats.collisions;
> >          stats->rx_length_errors    += dev_stats.rx_length_errors;
> > 
> > BR,
> > Lidong
> > 
> > -----邮件原件-----
> > 发件人: William Tu <u9012063@gmail.com>
> > 发送时间: 2020年4月25日 22:31
> > 收件人: 姜立东 <jianglidong3@jd.com>
> > 抄送: dev@openvswitch.org
> > 主题: Re: [ovs-dev] [PATCH] netdev-linux: remove sum of vport stats and 
> > kernel netdev stats
> > 
> > On Thu, Apr 23, 2020 at 05:35:14AM +0000, 姜立东 via dev wrote:
> > > From df9ff3b67f11e467928ca0873031d81b87f3d0c5 Mon Sep 17 00:00:00 
> > > 2001
> > > From: Jiang Lidong <jianglidong3@jd.com>
> > > Date: Thu, 23 Apr 2020 11:07:28 +0800
> > > Subject: [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats
> > >     When using kernel veth as OVS interface, doubled drop counter
> > >     value is shown when veth drops packets due to traffic overrun.
> > > 
> > >     In netdev_linux_get_stats, it reads both vport stats and kernel
> > >     netdev stats, in case vport stats retrieve failure. If both of
> > >     them success, error counters are added to include errors from
> > >     different layers. But implementation of ovs_vport_get_stats in
> > >     kernel data path has included kernel netdev stats by calling
> > >     dev_get_stats. When drop or other error counters is not zero,
> > >     its value is doubled by netdev_linux_get_stats.
> > > 
> > >     In this change, adding kernel netdev stats into vport stats
> > >     is removed, since vport stats includes all information of
> > >     kernel netdev stats.
> > > 
> > > Signed-off-by: Jiang Lidong <jianglidong3@jd.com>
> > > ---
> > >  lib/netdev-linux.c | 27 +--------------------------
> > >  1 file changed, 1 insertion(+), 26 deletions(-)
> > > 
> > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index
> > > ff045cb..6d139d0 100644
> > > --- a/lib/netdev-linux.c
> > > +++ b/lib/netdev-linux.c
> > > @@ -2207,33 +2207,8 @@ netdev_linux_get_stats(const struct netdev *netdev_,
> > >      } else if (netdev->vport_stats_error) {
> > >          /* stats not available from OVS then use netdev stats. */
> > >          *stats = dev_stats;
> > > -    } else {
> > > -        /* Use kernel netdev's packet and byte counts since vport's counters
> > > -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> > > -         * enabled. */
> > > -        stats->rx_packets = dev_stats.rx_packets;
> > > -        stats->rx_bytes = dev_stats.rx_bytes;
> > > -        stats->tx_packets = dev_stats.tx_packets;
> > > -        stats->tx_bytes = dev_stats.tx_bytes;
> > > -
> > > -        stats->rx_errors           += dev_stats.rx_errors;
> > > -        stats->tx_errors           += dev_stats.tx_errors;
> > > -        stats->rx_dropped          += dev_stats.rx_dropped;
> > > -        stats->tx_dropped          += dev_stats.tx_dropped;
> > 
> > Thanks for reporting the issue, looking at
> >   get_stats_via_vport
> >     get_stats_via_vport__
> >       netdev_stats_from_ovs_vport_stats
> >          // only set the ovs_vport_stats, which has 8 fields above
> > 
> > But with your patch it will remove all the reset of stats below
> > 
> > > -        stats->multicast           += dev_stats.multicast;
> > > -        stats->collisions          += dev_stats.collisions;
> > > -        stats->rx_length_errors    += dev_stats.rx_length_errors;
> > > -        stats->rx_over_errors      += dev_stats.rx_over_errors;
> > > -        stats->rx_crc_errors       += dev_stats.rx_crc_errors;
> > > -        stats->rx_frame_errors     += dev_stats.rx_frame_errors;
> > > -        stats->rx_fifo_errors      += dev_stats.rx_fifo_errors;
> > > -        stats->rx_missed_errors    += dev_stats.rx_missed_errors;
> > > -        stats->tx_aborted_errors   += dev_stats.tx_aborted_errors;
> > > -        stats->tx_carrier_errors   += dev_stats.tx_carrier_errors;
> > > -        stats->tx_fifo_errors      += dev_stats.tx_fifo_errors;
> > > -        stats->tx_heartbeat_errors += dev_stats.tx_heartbeat_errors;
> > > -        stats->tx_window_errors    += dev_stats.tx_window_errors;
> > >      }
> > 
> > 
> > How about we just over write the error and drop counts?
> >  
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 
> > c6e46f1885aa..c169c52611c4 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -2220,10 +2220,11 @@ netdev_linux_get_stats(const struct netdev *netdev_,
> >          stats->tx_packets = dev_stats.tx_packets;
> >          stats->tx_bytes = dev_stats.tx_bytes;
> >  
> > -        stats->rx_errors           += dev_stats.rx_errors;
> > -        stats->tx_errors           += dev_stats.tx_errors;
> > -        stats->rx_dropped          += dev_stats.rx_dropped;
> > -        stats->tx_dropped          += dev_stats.tx_dropped;
> > +        stats->rx_errors           = dev_stats.rx_errors;
> > +        stats->tx_errors           = dev_stats.tx_errors;
> > +        stats->rx_dropped          = dev_stats.rx_dropped;
> > +        stats->tx_dropped          = dev_stats.tx_dropped;
> > +
> >          stats->multicast           += dev_stats.multicast;
> >          stats->collisions          += dev_stats.collisions;
> >          stats->rx_length_errors    += dev_stats.rx_length_errors;
> > 
> > 
> > > +
> > >      ovs_mutex_unlock(&netdev->mutex);
> > >  
> > >      return error;
> > > --
> > > 1.8.3.1
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Pravin Shelar April 30, 2020, 5:42 a.m. UTC | #4
On Wed, Apr 29, 2020 at 11:07 AM William Tu <u9012063@gmail.com> wrote:
>
> On Tue, Apr 28, 2020 at 03:29:10AM +0000, 姜立东 wrote:
> > Hi William,
> >
> > > -        /* Use kernel netdev's packet and byte counts since vport's counters
> > > -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> > > -         * enabled. */
> >
> > Actually I think it should be moved to netdev_stats_from_ovs_vport_stats :), that explains what netdev_stats_from_ovs_vport_stats is doing.
> > In fact, I think better solution is copying all physical abnormal counters in netdev_stats_from_ovs_vport_stats ,
> > and remove this copy from netdev_linux_get_stats.
> > but netdev_stats_from_ovs_vport_stats is in kernel module that may not be upgraded in some application scenarios.
> > So I moved to remove unnecessary copies, such as RX/TX bytes, packets, drops.
> >
> > Regards,
> > Lidong
>
> oh, I see your point. Then this looks good to me.
> Add Pravin to see if he has some comments.
>

The patch looks fine to me.
Thanks,
Pravin.
William Tu April 30, 2020, 2:37 p.m. UTC | #5
On Wed, Apr 29, 2020 at 10:42:41PM -0700, Pravin Shelar wrote:
> On Wed, Apr 29, 2020 at 11:07 AM William Tu <u9012063@gmail.com> wrote:
> >
> > On Tue, Apr 28, 2020 at 03:29:10AM +0000, 姜立东 wrote:
> > > Hi William,
> > >
> > > > -        /* Use kernel netdev's packet and byte counts since vport's counters
> > > > -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> > > > -         * enabled. */
> > >
> > > Actually I think it should be moved to netdev_stats_from_ovs_vport_stats :), that explains what netdev_stats_from_ovs_vport_stats is doing.
> > > In fact, I think better solution is copying all physical abnormal counters in netdev_stats_from_ovs_vport_stats ,
> > > and remove this copy from netdev_linux_get_stats.
> > > but netdev_stats_from_ovs_vport_stats is in kernel module that may not be upgraded in some application scenarios.
> > > So I moved to remove unnecessary copies, such as RX/TX bytes, packets, drops.
> > >
> > > Regards,
> > > Lidong
> >
> > oh, I see your point. Then this looks good to me.
> > Add Pravin to see if he has some comments.
> >
> 
> The patch looks fine to me.
> Thanks,
> Pravin.

Thanks.
I applied to master.
diff mbox series

Patch

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index ff045cb..b851236 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2208,18 +2208,6 @@  netdev_linux_get_stats(const struct netdev *netdev_,
         /* stats not available from OVS then use netdev stats. */
         *stats = dev_stats;
     } else {
-        /* Use kernel netdev's packet and byte counts since vport's counters
-         * do not reflect packet counts on the wire when GSO, TSO or GRO are
-         * enabled. */
-        stats->rx_packets = dev_stats.rx_packets;
-        stats->rx_bytes = dev_stats.rx_bytes;
-        stats->tx_packets = dev_stats.tx_packets;
-        stats->tx_bytes = dev_stats.tx_bytes;
-
-        stats->rx_errors           += dev_stats.rx_errors;
-        stats->tx_errors           += dev_stats.tx_errors;
-        stats->rx_dropped          += dev_stats.rx_dropped;
-        stats->tx_dropped          += dev_stats.tx_dropped;
         stats->multicast           += dev_stats.multicast;
         stats->collisions          += dev_stats.collisions;
         stats->rx_length_errors    += dev_stats.rx_length_errors;

BR,
Lidong

-----邮件原件-----
发件人: William Tu <u9012063@gmail.com> 
发送时间: 2020年4月25日 22:31
收件人: 姜立东 <jianglidong3@jd.com>
抄送: dev@openvswitch.org
主题: Re: [ovs-dev] [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats

On Thu, Apr 23, 2020 at 05:35:14AM +0000, 姜立东 via dev wrote:
> From df9ff3b67f11e467928ca0873031d81b87f3d0c5 Mon Sep 17 00:00:00 2001
> From: Jiang Lidong <jianglidong3@jd.com>
> Date: Thu, 23 Apr 2020 11:07:28 +0800
> Subject: [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats
>     When using kernel veth as OVS interface, doubled drop counter
>     value is shown when veth drops packets due to traffic overrun.
> 
>     In netdev_linux_get_stats, it reads both vport stats and kernel
>     netdev stats, in case vport stats retrieve failure. If both of
>     them success, error counters are added to include errors from
>     different layers. But implementation of ovs_vport_get_stats in
>     kernel data path has included kernel netdev stats by calling
>     dev_get_stats. When drop or other error counters is not zero,
>     its value is doubled by netdev_linux_get_stats.
> 
>     In this change, adding kernel netdev stats into vport stats
>     is removed, since vport stats includes all information of
>     kernel netdev stats.
> 
> Signed-off-by: Jiang Lidong <jianglidong3@jd.com>
> ---
>  lib/netdev-linux.c | 27 +--------------------------
>  1 file changed, 1 insertion(+), 26 deletions(-)
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 
> ff045cb..6d139d0 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2207,33 +2207,8 @@ netdev_linux_get_stats(const struct netdev *netdev_,
>      } else if (netdev->vport_stats_error) {
>          /* stats not available from OVS then use netdev stats. */
>          *stats = dev_stats;
> -    } else {
> -        /* Use kernel netdev's packet and byte counts since vport's counters
> -         * do not reflect packet counts on the wire when GSO, TSO or GRO are
> -         * enabled. */
> -        stats->rx_packets = dev_stats.rx_packets;
> -        stats->rx_bytes = dev_stats.rx_bytes;
> -        stats->tx_packets = dev_stats.tx_packets;
> -        stats->tx_bytes = dev_stats.tx_bytes;
> -
> -        stats->rx_errors           += dev_stats.rx_errors;
> -        stats->tx_errors           += dev_stats.tx_errors;
> -        stats->rx_dropped          += dev_stats.rx_dropped;
> -        stats->tx_dropped          += dev_stats.tx_dropped;

Thanks for reporting the issue, looking at
  get_stats_via_vport
    get_stats_via_vport__
      netdev_stats_from_ovs_vport_stats
         // only set the ovs_vport_stats, which has 8 fields above

But with your patch it will remove all the reset of stats below

> -        stats->multicast           += dev_stats.multicast;
> -        stats->collisions          += dev_stats.collisions;
> -        stats->rx_length_errors    += dev_stats.rx_length_errors;
> -        stats->rx_over_errors      += dev_stats.rx_over_errors;
> -        stats->rx_crc_errors       += dev_stats.rx_crc_errors;
> -        stats->rx_frame_errors     += dev_stats.rx_frame_errors;
> -        stats->rx_fifo_errors      += dev_stats.rx_fifo_errors;
> -        stats->rx_missed_errors    += dev_stats.rx_missed_errors;
> -        stats->tx_aborted_errors   += dev_stats.tx_aborted_errors;
> -        stats->tx_carrier_errors   += dev_stats.tx_carrier_errors;
> -        stats->tx_fifo_errors      += dev_stats.tx_fifo_errors;
> -        stats->tx_heartbeat_errors += dev_stats.tx_heartbeat_errors;
> -        stats->tx_window_errors    += dev_stats.tx_window_errors;
>      }


How about we just over write the error and drop counts?
 
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index c6e46f1885aa..c169c52611c4 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2220,10 +2220,11 @@  netdev_linux_get_stats(const struct netdev *netdev_,
         stats->tx_packets = dev_stats.tx_packets;
         stats->tx_bytes = dev_stats.tx_bytes;
 
-        stats->rx_errors           += dev_stats.rx_errors;
-        stats->tx_errors           += dev_stats.tx_errors;
-        stats->rx_dropped          += dev_stats.rx_dropped;
-        stats->tx_dropped          += dev_stats.tx_dropped;
+        stats->rx_errors           = dev_stats.rx_errors;
+        stats->tx_errors           = dev_stats.tx_errors;
+        stats->rx_dropped          = dev_stats.rx_dropped;
+        stats->tx_dropped          = dev_stats.tx_dropped;
+
         stats->multicast           += dev_stats.multicast;
         stats->collisions          += dev_stats.collisions;
         stats->rx_length_errors    += dev_stats.rx_length_errors;