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

William Tu u9012063 at gmail.com
Wed Apr 29 18:07:52 UTC 2020


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 at gmail.com> 
> 发送时间: 2020年4月27日 12:20
> 收件人: 姜立东 <jianglidong3 at jd.com>
> 抄送: dev at 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 at gmail.com>
> > 发送时间: 2020年4月25日 22:31
> > 收件人: 姜立东 <jianglidong3 at jd.com>
> > 抄送: dev at 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 at 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 at 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 at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list