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 <[email protected]> 发送时间: 2020年4月27日 12:20 收件人: 姜立东 <[email protected]> 抄送: [email protected] 主题: 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 <[email protected]> > 发送时间: 2020年4月25日 22:31 > 收件人: 姜立东 <[email protected]> > 抄送: [email protected] > 主题: 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 <[email protected]> > > 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 <[email protected]> > > --- > > 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
