| Message ID | 172e1abc63cc4481ab1ee0ae82051448@jd.com |
|---|---|
| State | Accepted |
| Commit | b9f825a5442a979b28a5ec18640129058d334e85 |
| Headers | show |
| Series | [ovs-dev] 答复: [PATCH] netdev-linux: remove sum of vport stats and kernel netdev stats | expand |
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
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
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
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.
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 --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;