On 2017/12/25 3:16, Stephen Hemminger wrote:> On Wed, 20 Dec 2017 13:40:37 +0900 > Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp> wrote: > >> + >> +static const struct virtnet_gstats virtnet_gstrings_stats[] = { >> + { "rx_packets", VIRTNET_NETDEV_STAT(rx_packets) }, >> + { "tx_packets", VIRTNET_NETDEV_STAT(tx_packets) }, >> + { "rx_bytes", VIRTNET_NETDEV_STAT(rx_bytes) }, >> + { "tx_bytes", VIRTNET_NETDEV_STAT(tx_bytes) }, >> + { "rx_dropped", VIRTNET_NETDEV_STAT(rx_dropped) }, >> + { "rx_length_errors", VIRTNET_NETDEV_STAT(rx_length_errors) }, >> + { "rx_frame_errors", VIRTNET_NETDEV_STAT(rx_frame_errors) }, >> + { "tx_dropped", VIRTNET_NETDEV_STAT(tx_dropped) }, >> + { "tx_fifo_errors", VIRTNET_NETDEV_STAT(tx_fifo_errors) }, >> +}; >> + > > Please do not merge pre-existing global stats into ethtool. > It just duplicates existing functionality.Thanks for the feedback. I thought it's handy to be able to check stats like this in ethtool as well. Some drivers like ixgbe and mlx5 do similar thing. I can remove these duplicate stats (unless anyone has objection). -- Toshiaki Makita
On December 25, 2017 3:45:36 AM PST, Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp> wrote:>On 2017/12/25 3:16, Stephen Hemminger wrote: >> On Wed, 20 Dec 2017 13:40:37 +0900 >> Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp> wrote: >> >>> + >>> +static const struct virtnet_gstats virtnet_gstrings_stats[] = { >>> + { "rx_packets", VIRTNET_NETDEV_STAT(rx_packets) }, >>> + { "tx_packets", VIRTNET_NETDEV_STAT(tx_packets) }, >>> + { "rx_bytes", VIRTNET_NETDEV_STAT(rx_bytes) }, >>> + { "tx_bytes", VIRTNET_NETDEV_STAT(tx_bytes) }, >>> + { "rx_dropped", VIRTNET_NETDEV_STAT(rx_dropped) }, >>> + { "rx_length_errors", VIRTNET_NETDEV_STAT(rx_length_errors) }, >>> + { "rx_frame_errors", VIRTNET_NETDEV_STAT(rx_frame_errors) }, >>> + { "tx_dropped", VIRTNET_NETDEV_STAT(tx_dropped) }, >>> + { "tx_fifo_errors", VIRTNET_NETDEV_STAT(tx_fifo_errors) }, >>> +}; >>> + >> >> Please do not merge pre-existing global stats into ethtool. >> It just duplicates existing functionality. > >Thanks for the feedback. >I thought it's handy to be able to check stats like this in ethtool as >well. Some drivers like ixgbe and mlx5 do similar thing. >I can remove these duplicate stats (unless anyone has objection).For existing drivers, ethtool stats are kind of user ABI, so once these stats get in, we should not consider removing them. For new drivers or existing drivers without ethtool stats, we can do things a bit better as Stephen suggested. -- Florian
On 2017/12/26 1:47, Florian Fainelli wrote:> On December 25, 2017 3:45:36 AM PST, Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp> wrote: >> On 2017/12/25 3:16, Stephen Hemminger wrote: >>> On Wed, 20 Dec 2017 13:40:37 +0900 >>> Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp> wrote: >>> >>>> + >>>> +static const struct virtnet_gstats virtnet_gstrings_stats[] = { >>>> + { "rx_packets", VIRTNET_NETDEV_STAT(rx_packets) }, >>>> + { "tx_packets", VIRTNET_NETDEV_STAT(tx_packets) }, >>>> + { "rx_bytes", VIRTNET_NETDEV_STAT(rx_bytes) }, >>>> + { "tx_bytes", VIRTNET_NETDEV_STAT(tx_bytes) }, >>>> + { "rx_dropped", VIRTNET_NETDEV_STAT(rx_dropped) }, >>>> + { "rx_length_errors", VIRTNET_NETDEV_STAT(rx_length_errors) }, >>>> + { "rx_frame_errors", VIRTNET_NETDEV_STAT(rx_frame_errors) }, >>>> + { "tx_dropped", VIRTNET_NETDEV_STAT(tx_dropped) }, >>>> + { "tx_fifo_errors", VIRTNET_NETDEV_STAT(tx_fifo_errors) }, >>>> +}; >>>> + >>> >>> Please do not merge pre-existing global stats into ethtool. >>> It just duplicates existing functionality. >> >> Thanks for the feedback. >> I thought it's handy to be able to check stats like this in ethtool as >> well. Some drivers like ixgbe and mlx5 do similar thing. >> I can remove these duplicate stats (unless anyone has objection). > > For existing drivers, ethtool stats are kind of user ABI, so once these stats get in, we should not consider removing them. For new drivers or existing drivers without ethtool stats, we can do things a bit better as Stephen suggested.OK. will remove them in v2. Thanks, Toshiaki Makita