| Message ID | 20101216123131.GA3070@redhat.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
Le jeudi 16 décembre 2010 à 10:31 -0200, Flavio Leitner a écrit : > -static struct net_device_stats *e1000_get_stats(struct net_device *netdev) > +struct rtnl_link_stats64 *e1000e_get_stats64(struct net_device *netdev, > + struct rtnl_link_stats64 *stats) > { > - /* only return the current stats */ > - return &netdev->stats; > + struct e1000_adapter *adapter = netdev_priv(netdev); > + > + memset(stats, 0, sizeof(struct rtnl_link_stats64)); You dont need this memset(), stats is cleared by caller (dev_get_stats() in net/core/dev.c), as this was always done ;) > + spin_lock(&adapter->stats64_lock); > + e1000e_update_stats(adapter); > + /* Fill out the OS statistics structure */ > + stats->rx_bytes = adapter->stats.gorc; > + stats->rx_packets = adapter->stats.gprc; > + stats->tx_bytes = adapter->stats.gotc; > + stats->tx_packets = adapter->stats.gptc; > + stats->multicast = adapter->stats.mprc; > + stats->collisions = adapter->stats.colc; > + -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 16, 2010 at 01:50:30PM +0100, Eric Dumazet wrote: > Le jeudi 16 décembre 2010 à 10:31 -0200, Flavio Leitner a écrit : > > > -static struct net_device_stats *e1000_get_stats(struct net_device *netdev) > > +struct rtnl_link_stats64 *e1000e_get_stats64(struct net_device *netdev, > > + struct rtnl_link_stats64 *stats) > > { > > - /* only return the current stats */ > > - return &netdev->stats; > > + struct e1000_adapter *adapter = netdev_priv(netdev); > > + > > + memset(stats, 0, sizeof(struct rtnl_link_stats64)); > > You dont need this memset(), stats is cleared by caller (dev_get_stats() > in net/core/dev.c), as this was always done ;) Yes, but e1000_get_ethtool_stats() also calls it and doesn't do that. I could move the memset to the caller, but I thought it would be cleaner to leave where it is now. > > + spin_lock(&adapter->stats64_lock); > > + e1000e_update_stats(adapter); > > + /* Fill out the OS statistics structure */ > > + stats->rx_bytes = adapter->stats.gorc; > > + stats->rx_packets = adapter->stats.gprc; > > + stats->tx_bytes = adapter->stats.gotc; > > + stats->tx_packets = adapter->stats.gptc; > > + stats->multicast = adapter->stats.mprc; > > + stats->collisions = adapter->stats.colc; > > + > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Le jeudi 16 décembre 2010 à 11:10 -0200, Flavio Leitner a écrit : > Yes, but e1000_get_ethtool_stats() also calls it and doesn't do that. > I could move the memset to the caller, but I thought it would be cleaner > to leave where it is now. Yes, no problem ! Acked-by: Eric Dumazet <eric.dumazet@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 16, 2010 at 04:31, Flavio Leitner <fleitner@redhat.com> wrote: > On Tue, Dec 14, 2010 at 10:29:33PM +0100, Eric Dumazet wrote: >> Le mardi 14 décembre 2010 à 18:32 -0200, Flavio Leitner a écrit : >> > Provides accurate stats at the time user reads them. >> > >> > Signed-off-by: Flavio Leitner <fleitner@redhat.com> >> > --- >> > drivers/net/e1000e/e1000.h | 5 ++- >> > drivers/net/e1000e/ethtool.c | 27 +++++++++------- >> > drivers/net/e1000e/netdev.c | 68 ++++++++++++++++++++++++----------------- >> > 3 files changed, 59 insertions(+), 41 deletions(-) >> > >> > diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h >> > index fdc67fe..5a5e944 100644 >> > --- a/drivers/net/e1000e/e1000.h >> > +++ b/drivers/net/e1000e/e1000.h >> > @@ -363,6 +363,8 @@ struct e1000_adapter { >> > /* structs defined in e1000_hw.h */ >> > struct e1000_hw hw; >> > >> > + spinlock_t stats64_lock; >> > + struct rtnl_link_stats64 stats64; >> >> I am not sure why you add this stats64 in e1000_adapter ? >> >> Why isnt it provided by callers (automatic variable, or provided to >> ndo_get_stats64()). I dont see accumulators, only a full rewrite of this >> structure in e1000e_update_stats() ? > > Good point. I have modified the patch to fix that. > thanks! > > From 3487bd7dacd0c23bba315270139dab6e00e5ff02 Mon Sep 17 00:00:00 2001 > From: Flavio Leitner <fleitner@redhat.com> > Date: Thu, 16 Dec 2010 10:26:03 -0200 > Subject: [PATCH] e1000e: convert to stats64 > > Provides accurate stats at the time user reads them. > > Signed-off-by: Flavio Leitner <fleitner@redhat.com> > --- > drivers/net/e1000e/e1000.h | 3 ++ > drivers/net/e1000e/ethtool.c | 25 ++++++++------- > drivers/net/e1000e/netdev.c | 68 +++++++++++++++++++++++++++++++++-------- > 3 files changed, 70 insertions(+), 26 deletions(-) > I have dropped you previous version of the patch and applied v2 to my tree for review and testing. Thanks Flavio!
On Thu, Dec 16, 2010 at 07:14:35PM -0800, Jeff Kirsher wrote: > On Thu, Dec 16, 2010 at 04:31, Flavio Leitner <fleitner@redhat.com> wrote: > > On Tue, Dec 14, 2010 at 10:29:33PM +0100, Eric Dumazet wrote: > >> Le mardi 14 décembre 2010 à 18:32 -0200, Flavio Leitner a écrit : > >> > Provides accurate stats at the time user reads them. > >> > > >> > Signed-off-by: Flavio Leitner <fleitner@redhat.com> > >> > --- > >> > drivers/net/e1000e/e1000.h | 5 ++- > >> > drivers/net/e1000e/ethtool.c | 27 +++++++++------- > >> > drivers/net/e1000e/netdev.c | 68 ++++++++++++++++++++++++----------------- > >> > 3 files changed, 59 insertions(+), 41 deletions(-) > >> > > >> > diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h > >> > index fdc67fe..5a5e944 100644 > >> > --- a/drivers/net/e1000e/e1000.h > >> > +++ b/drivers/net/e1000e/e1000.h > >> > @@ -363,6 +363,8 @@ struct e1000_adapter { > >> > /* structs defined in e1000_hw.h */ > >> > struct e1000_hw hw; > >> > > >> > + spinlock_t stats64_lock; > >> > + struct rtnl_link_stats64 stats64; > >> > >> I am not sure why you add this stats64 in e1000_adapter ? > >> > >> Why isnt it provided by callers (automatic variable, or provided to > >> ndo_get_stats64()). I dont see accumulators, only a full rewrite of this > >> structure in e1000e_update_stats() ? > > > > Good point. I have modified the patch to fix that. > > thanks! > > > > From 3487bd7dacd0c23bba315270139dab6e00e5ff02 Mon Sep 17 00:00:00 2001 > > From: Flavio Leitner <fleitner@redhat.com> > > Date: Thu, 16 Dec 2010 10:26:03 -0200 > > Subject: [PATCH] e1000e: convert to stats64 > > > > Provides accurate stats at the time user reads them. > > > > Signed-off-by: Flavio Leitner <fleitner@redhat.com> > > --- > > drivers/net/e1000e/e1000.h | 3 ++ > > drivers/net/e1000e/ethtool.c | 25 ++++++++------- > > drivers/net/e1000e/netdev.c | 68 +++++++++++++++++++++++++++++++++-------- > > 3 files changed, 70 insertions(+), 26 deletions(-) > > > > I have dropped you previous version of the patch and applied v2 to my > tree for review and testing. > Thanks Flavio! Hi Jeff, Do you have any feedback on this patch? thanks,
On Fri, 2011-01-21 at 09:03 -0800, Flavio Leitner wrote: > On Thu, Dec 16, 2010 at 07:14:35PM -0800, Jeff Kirsher wrote: > > On Thu, Dec 16, 2010 at 04:31, Flavio Leitner <fleitner@redhat.com> wrote: > > > On Tue, Dec 14, 2010 at 10:29:33PM +0100, Eric Dumazet wrote: > > >> Le mardi 14 décembre 2010 à 18:32 -0200, Flavio Leitner a écrit : > > >> > Provides accurate stats at the time user reads them. > > >> > > > >> > Signed-off-by: Flavio Leitner <fleitner@redhat.com> > > >> > --- > > >> > drivers/net/e1000e/e1000.h | 5 ++- > > >> > drivers/net/e1000e/ethtool.c | 27 +++++++++------- > > >> > drivers/net/e1000e/netdev.c | 68 ++++++++++++++++++++++++----------------- > > >> > 3 files changed, 59 insertions(+), 41 deletions(-) > > >> > > > >> > diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h > > >> > index fdc67fe..5a5e944 100644 > > >> > --- a/drivers/net/e1000e/e1000.h > > >> > +++ b/drivers/net/e1000e/e1000.h > > >> > @@ -363,6 +363,8 @@ struct e1000_adapter { > > >> > /* structs defined in e1000_hw.h */ > > >> > struct e1000_hw hw; > > >> > > > >> > + spinlock_t stats64_lock; > > >> > + struct rtnl_link_stats64 stats64; > > >> > > >> I am not sure why you add this stats64 in e1000_adapter ? > > >> > > >> Why isnt it provided by callers (automatic variable, or provided to > > >> ndo_get_stats64()). I dont see accumulators, only a full rewrite of this > > >> structure in e1000e_update_stats() ? > > > > > > Good point. I have modified the patch to fix that. > > > thanks! > > > > > > From 3487bd7dacd0c23bba315270139dab6e00e5ff02 Mon Sep 17 00:00:00 2001 > > > From: Flavio Leitner <fleitner@redhat.com> > > > Date: Thu, 16 Dec 2010 10:26:03 -0200 > > > Subject: [PATCH] e1000e: convert to stats64 > > > > > > Provides accurate stats at the time user reads them. > > > > > > Signed-off-by: Flavio Leitner <fleitner@redhat.com> > > > --- > > > drivers/net/e1000e/e1000.h | 3 ++ > > > drivers/net/e1000e/ethtool.c | 25 ++++++++------- > > > drivers/net/e1000e/netdev.c | 68 +++++++++++++++++++++++++++++++++-------- > > > 3 files changed, 70 insertions(+), 26 deletions(-) > > > > > > > I have dropped you previous version of the patch and applied v2 to my > > tree for review and testing. > > Thanks Flavio! > > Hi Jeff, > > Do you have any feedback on this patch? > thanks, I have the patch ready to push to Dave for net-next, along with a few other e1000e patches. I was just waiting for net-next to open (which it did a few days ago).
diff --git a/drivers/net/e1000e/e1000.h b/drivers/net/e1000e/e1000.h index fdc67fe..d8fc3bc 100644 --- a/drivers/net/e1000e/e1000.h +++ b/drivers/net/e1000e/e1000.h @@ -363,6 +363,7 @@ struct e1000_adapter { /* structs defined in e1000_hw.h */ struct e1000_hw hw; + spinlock_t stats64_lock; struct e1000_hw_stats stats; struct e1000_phy_info phy_info; struct e1000_phy_stats phy_stats; @@ -493,6 +494,8 @@ extern int e1000e_setup_tx_resources(struct e1000_adapter *adapter); extern void e1000e_free_rx_resources(struct e1000_adapter *adapter); extern void e1000e_free_tx_resources(struct e1000_adapter *adapter); extern void e1000e_update_stats(struct e1000_adapter *adapter); +extern struct rtnl_link_stats64 *e1000e_get_stats64(struct net_device *netdev, + struct rtnl_link_stats64 *stats); extern void e1000e_set_interrupt_capability(struct e1000_adapter *adapter); extern void e1000e_reset_interrupt_capability(struct e1000_adapter *adapter); extern void e1000e_disable_aspm(struct pci_dev *pdev, u16 state); diff --git a/drivers/net/e1000e/ethtool.c b/drivers/net/e1000e/ethtool.c index 8984d16..f43b412 100644 --- a/drivers/net/e1000e/ethtool.c +++ b/drivers/net/e1000e/ethtool.c @@ -49,8 +49,8 @@ struct e1000_stats { sizeof(((struct e1000_adapter *)0)->m), \ offsetof(struct e1000_adapter, m) #define E1000_NETDEV_STAT(m) NETDEV_STATS, \ - sizeof(((struct net_device *)0)->m), \ - offsetof(struct net_device, m) + sizeof(((struct rtnl_link_stats64 *)0)->m), \ + offsetof(struct rtnl_link_stats64, m) static const struct e1000_stats e1000_gstrings_stats[] = { { "rx_packets", E1000_STAT(stats.gprc) }, @@ -61,21 +61,21 @@ static const struct e1000_stats e1000_gstrings_stats[] = { { "tx_broadcast", E1000_STAT(stats.bptc) }, { "rx_multicast", E1000_STAT(stats.mprc) }, { "tx_multicast", E1000_STAT(stats.mptc) }, - { "rx_errors", E1000_NETDEV_STAT(stats.rx_errors) }, - { "tx_errors", E1000_NETDEV_STAT(stats.tx_errors) }, - { "tx_dropped", E1000_NETDEV_STAT(stats.tx_dropped) }, + { "rx_errors", E1000_NETDEV_STAT(rx_errors) }, + { "tx_errors", E1000_NETDEV_STAT(tx_errors) }, + { "tx_dropped", E1000_NETDEV_STAT(tx_dropped) }, { "multicast", E1000_STAT(stats.mprc) }, { "collisions", E1000_STAT(stats.colc) }, - { "rx_length_errors", E1000_NETDEV_STAT(stats.rx_length_errors) }, - { "rx_over_errors", E1000_NETDEV_STAT(stats.rx_over_errors) }, + { "rx_length_errors", E1000_NETDEV_STAT(rx_length_errors) }, + { "rx_over_errors", E1000_NETDEV_STAT(rx_over_errors) }, { "rx_crc_errors", E1000_STAT(stats.crcerrs) }, - { "rx_frame_errors", E1000_NETDEV_STAT(stats.rx_frame_errors) }, + { "rx_frame_errors", E1000_NETDEV_STAT(rx_frame_errors) }, { "rx_no_buffer_count", E1000_STAT(stats.rnbc) }, { "rx_missed_errors", E1000_STAT(stats.mpc) }, { "tx_aborted_errors", E1000_STAT(stats.ecol) }, { "tx_carrier_errors", E1000_STAT(stats.tncrs) }, - { "tx_fifo_errors", E1000_NETDEV_STAT(stats.tx_fifo_errors) }, - { "tx_heartbeat_errors", E1000_NETDEV_STAT(stats.tx_heartbeat_errors) }, + { "tx_fifo_errors", E1000_NETDEV_STAT(tx_fifo_errors) }, + { "tx_heartbeat_errors", E1000_NETDEV_STAT(tx_heartbeat_errors) }, { "tx_window_errors", E1000_STAT(stats.latecol) }, { "tx_abort_late_coll", E1000_STAT(stats.latecol) }, { "tx_deferred_ok", E1000_STAT(stats.dc) }, @@ -1972,14 +1972,15 @@ static void e1000_get_ethtool_stats(struct net_device *netdev, u64 *data) { struct e1000_adapter *adapter = netdev_priv(netdev); + struct rtnl_link_stats64 net_stats; int i; char *p = NULL; - e1000e_update_stats(adapter); + e1000e_get_stats64(netdev, &net_stats); for (i = 0; i < E1000_GLOBAL_STATS_LEN; i++) { switch (e1000_gstrings_stats[i].type) { case NETDEV_STATS: - p = (char *) netdev + + p = (char *) &net_stats + e1000_gstrings_stats[i].stat_offset; break; case E1000_STATS: diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c index c4ca162..b3c4b7d 100644 --- a/drivers/net/e1000e/netdev.c +++ b/drivers/net/e1000e/netdev.c @@ -894,8 +894,6 @@ next_desc: adapter->total_rx_bytes += total_rx_bytes; adapter->total_rx_packets += total_rx_packets; - netdev->stats.rx_bytes += total_rx_bytes; - netdev->stats.rx_packets += total_rx_packets; return cleaned; } @@ -1051,8 +1049,6 @@ static bool e1000_clean_tx_irq(struct e1000_adapter *adapter) } adapter->total_tx_bytes += total_tx_bytes; adapter->total_tx_packets += total_tx_packets; - netdev->stats.tx_bytes += total_tx_bytes; - netdev->stats.tx_packets += total_tx_packets; return count < tx_ring->count; } @@ -1240,8 +1236,6 @@ next_desc: adapter->total_rx_bytes += total_rx_bytes; adapter->total_rx_packets += total_rx_packets; - netdev->stats.rx_bytes += total_rx_bytes; - netdev->stats.rx_packets += total_rx_packets; return cleaned; } @@ -1421,8 +1415,6 @@ next_desc: adapter->total_rx_bytes += total_rx_bytes; adapter->total_rx_packets += total_rx_packets; - netdev->stats.rx_bytes += total_rx_bytes; - netdev->stats.rx_packets += total_rx_packets; return cleaned; } @@ -3367,6 +3359,11 @@ void e1000e_down(struct e1000_adapter *adapter) del_timer_sync(&adapter->phy_info_timer); netif_carrier_off(netdev); + + spin_lock(&adapter->stats64_lock); + e1000e_update_stats(adapter); + spin_unlock(&adapter->stats64_lock); + adapter->link_speed = 0; adapter->link_duplex = 0; @@ -3408,6 +3405,8 @@ static int __devinit e1000_sw_init(struct e1000_adapter *adapter) adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN; adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN; + spin_lock_init(&adapter->stats64_lock); + e1000e_set_interrupt_capability(adapter); if (e1000_alloc_queues(adapter)) @@ -4279,7 +4278,9 @@ static void e1000_watchdog_task(struct work_struct *work) } link_up: + spin_lock(&adapter->stats64_lock); e1000e_update_stats(adapter); + spin_unlock(&adapter->stats64_lock); mac->tx_packet_delta = adapter->stats.tpt - adapter->tpt_old; adapter->tpt_old = adapter->stats.tpt; @@ -4891,16 +4892,55 @@ static void e1000_reset_task(struct work_struct *work) } /** - * e1000_get_stats - Get System Network Statistics + * e1000_get_stats64 - Get System Network Statistics * @netdev: network interface device structure + * @stats: rtnl_link_stats64 pointer * * Returns the address of the device statistics structure. - * The statistics are actually updated from the timer callback. **/ -static struct net_device_stats *e1000_get_stats(struct net_device *netdev) +struct rtnl_link_stats64 *e1000e_get_stats64(struct net_device *netdev, + struct rtnl_link_stats64 *stats) { - /* only return the current stats */ - return &netdev->stats; + struct e1000_adapter *adapter = netdev_priv(netdev); + + memset(stats, 0, sizeof(struct rtnl_link_stats64)); + spin_lock(&adapter->stats64_lock); + e1000e_update_stats(adapter); + /* Fill out the OS statistics structure */ + stats->rx_bytes = adapter->stats.gorc; + stats->rx_packets = adapter->stats.gprc; + stats->tx_bytes = adapter->stats.gotc; + stats->tx_packets = adapter->stats.gptc; + stats->multicast = adapter->stats.mprc; + stats->collisions = adapter->stats.colc; + + /* Rx Errors */ + + /* + * RLEC on some newer hardware can be incorrect so build + * our own version based on RUC and ROC + */ + stats->rx_errors = adapter->stats.rxerrc + + adapter->stats.crcerrs + adapter->stats.algnerrc + + adapter->stats.ruc + adapter->stats.roc + + adapter->stats.cexterr; + stats->rx_length_errors = adapter->stats.ruc + + adapter->stats.roc; + stats->rx_crc_errors = adapter->stats.crcerrs; + stats->rx_frame_errors = adapter->stats.algnerrc; + stats->rx_missed_errors = adapter->stats.mpc; + + /* Tx Errors */ + stats->tx_errors = adapter->stats.ecol + + adapter->stats.latecol; + stats->tx_aborted_errors = adapter->stats.ecol; + stats->tx_window_errors = adapter->stats.latecol; + stats->tx_carrier_errors = adapter->stats.tncrs; + + /* Tx Dropped needs to be maintained elsewhere */ + + spin_unlock(&adapter->stats64_lock); + return stats; } /** @@ -5624,7 +5664,7 @@ static const struct net_device_ops e1000e_netdev_ops = { .ndo_open = e1000_open, .ndo_stop = e1000_close, .ndo_start_xmit = e1000_xmit_frame, - .ndo_get_stats = e1000_get_stats, + .ndo_get_stats64 = e1000e_get_stats64, .ndo_set_multicast_list = e1000_set_multi, .ndo_set_mac_address = e1000_set_mac, .ndo_change_mtu = e1000_change_mtu,