forcedeth: stats & debug enhancements
Subsystems (3)
cover forcedeth: stats & debug enhancements
These changes implement the ndo_get_stats64 API and add a few more stats and debugging features for forcedeth. They also ensure that stats updates are correct in SMP systems, 32 or 64-bits.
Regarding the "implement ndo_get_stats64() API" patch, I'm not sure I'm using the right way to protect the 64b stats. Ideally, I would like them to be non-blocking (u64_stats_sync.h), but as there are several sources for updates, I don't think I can do without locking or per-CPU stats. Would per-CPU stats be better here (note: I expect the contention on netdev_priv(dev)->stats_lock to be _VERY_ low)?
Tested:
~150Mbps incoming TCP, ethtool -S in a loop, x86_64 16-way:
tx_bytes: 1413863329
rx_packets: 38918872
tx_packets: 19828148
rx_bytes: 57818685991############################################
# Patch Set Summary:
David Decotigny (6):
forcedeth: expose module parameters in /sys/module
forcedeth: stats for rx_packets based on hardware registers
forcedeth: implement ndo_get_stats64() API
forcedeth: account for dropped RX frames
forcedeth: stats updated with a deferrable timer
forcedeth: whitespace/indentation fixes
Mike Ditto (1):
forcedeth: Add messages to indicate using MSI or MSI-X
Sameer Nanda (2):
forcedeth: allow to silence "TX timeout" debug messages
forcedeth: new ethtool stat counter for TX timeouts
drivers/net/ethernet/nvidia/forcedeth.c | 271 +++++++++++++++++++++---------- 1 files changed, 184 insertions(+), 87 deletions(-)
Signature · 1 line
1.7.3.1
PATCH 1/9 forcedeth: Add messages to indicate using MSI or MSI-X
From: Mike Ditto <[email protected]>
This adds a few debug messages to indicate whether PCIe interrupts are signaled with MSI or MSI-X.
Signed-off-by David Decotigny <[email protected]>
--- drivers/net/ethernet/nvidia/forcedeth.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/ethernet/nvidia/forcedeth.c +2 -0
@@ -3711,6 +3711,7 @@ static int nv_request_irq(struct net_device *dev, int intr_test)writel (0 , base + NvRegMSIXMap0);writel (0 , base + NvRegMSIXMap1); } +netdev_info (dev,"MSI-X enabled\n" ); } }if (ret !=0 && np->msi_flags &NV_MSI_CAPABLE ) {
@@ -3732,6 +3733,7 @@ static int nv_request_irq(struct net_device *dev, int intr_test)writel (0 , base + NvRegMSIMap1);/* enable msi vector 0 */ writel (NVREG_MSI_VECTOR_0_ENABLED , base + NvRegMSIIrqMask); +netdev_info (dev,"MSI enabled\n" ); } }if (ret !=0 ) {
Signature · 1 line
1.7.3.1
PATCH 2/9 forcedeth: allow to silence "TX timeout" debug messages
In reply to David DecotignyFrom: Sameer Nanda <[email protected]>
This adds a new module parameter "debug_tx_timeout" to silence most debug messages in case of TX timeout. These messages don't provide a signal/noise ratio high enough for production systems and, with ~30kB logged each time, they tend to add to a cascade effect if the system is already under stress (memory pressure, disk, etc.).
By default, the parameter is clear, meaning that only a single warning will be reported (the other more detailed debug messages are not displayed).
Signed-off-by David Decotigny <[email protected]>
--- drivers/net/ethernet/nvidia/forcedeth.c | 98 ++++++++++++++++++------------- 1 files changed, 57 insertions(+), 41 deletions(-)
drivers/net/ethernet/nvidia/forcedeth.c +57 -41
@@ -892,6 +892,11 @@ enum {static int dma_64bit =NV_DMA_64BIT_ENABLED ;/* +* Debug output control for tx_timeout +*/ +static bool debug_tx_timeout = false; + +/* * Crossover Detection * Realtek 8201 phy + some OEM boards do not work properly. */
@@ -2461,56 +2466,64 @@ static void nv_tx_timeout(struct net_device *dev)u32 status;union ring_type put_tx;int saved_tx_limit; -int i;if (np->msi_flags &NV_MSI_X_ENABLED ) status =readl (base + NvRegMSIXIrqStatus) &NVREG_IRQSTAT_MASK ;else status =readl (base + NvRegIrqStatus) &NVREG_IRQSTAT_MASK ; -netdev_info (dev,"Got tx_timeout. irq: %08x\n" , status); +netdev_warn (dev,"Got tx_timeout. irq status: %08x\n" , status); -netdev_info (dev,"Ring at %lx\n" , (unsigned long )np->ring_addr); -netdev_info (dev,"Dumping tx registers\n" ); -for (i =0 ; i <= np->register_size; i +=32 ) { -netdev_info (dev, -"%3x: %08x %08x %08x %08x %08x %08x %08x %08x\n" , - i, -readl (base + i +0 ),readl (base + i +4 ), -readl (base + i +8 ),readl (base + i +12 ), -readl (base + i +16 ),readl (base + i +20 ), -readl (base + i +24 ),readl (base + i +28 )); - } -netdev_info (dev,"Dumping tx ring\n" ); -for (i =0 ; i < np->tx_ring_size; i +=4 ) { -if (!nv_optimized (np)) { -netdev_info (dev, -"%03x: %08x %08x // %08x %08x // %08x %08x // %08x %08x\n" , - i, -le32_to_cpu (np->tx_ring.orig[i].buf), -le32_to_cpu (np->tx_ring.orig[i].flaglen), -le32_to_cpu (np->tx_ring.orig[i+1 ].buf), -le32_to_cpu (np->tx_ring.orig[i+1 ].flaglen), -le32_to_cpu (np->tx_ring.orig[i+2 ].buf), -le32_to_cpu (np->tx_ring.orig[i+2 ].flaglen), -le32_to_cpu (np->tx_ring.orig[i+3 ].buf), -le32_to_cpu (np->tx_ring.orig[i+3 ].flaglen)); - }else { +if (unlikely (debug_tx_timeout)) { +int i; + +netdev_info (dev,"Ring at %lx\n" , (unsigned long )np->ring_addr); +netdev_info (dev,"Dumping tx registers\n" ); +for (i =0 ; i <= np->register_size; i +=32 ) {netdev_info (dev, -"%03x: %08x %08x %08x // %08x %08x %08x // %08x %08x %08x // %08x %08x %08x\n" , +"%3x: %08x %08x %08x %08x " +"%08x %08x %08x %08x\n" , i, -le32_to_cpu (np->tx_ring.ex[i].bufhigh), -le32_to_cpu (np->tx_ring.ex[i].buflow), -le32_to_cpu (np->tx_ring.ex[i].flaglen), -le32_to_cpu (np->tx_ring.ex[i+1 ].bufhigh), -le32_to_cpu (np->tx_ring.ex[i+1 ].buflow), -le32_to_cpu (np->tx_ring.ex[i+1 ].flaglen), -le32_to_cpu (np->tx_ring.ex[i+2 ].bufhigh), -le32_to_cpu (np->tx_ring.ex[i+2 ].buflow), -le32_to_cpu (np->tx_ring.ex[i+2 ].flaglen), -le32_to_cpu (np->tx_ring.ex[i+3 ].bufhigh), -le32_to_cpu (np->tx_ring.ex[i+3 ].buflow), -le32_to_cpu (np->tx_ring.ex[i+3 ].flaglen)); +readl (base + i +0 ),readl (base + i +4 ), +readl (base + i +8 ),readl (base + i +12 ), +readl (base + i +16 ),readl (base + i +20 ), +readl (base + i +24 ),readl (base + i +28 )); + } +netdev_info (dev,"Dumping tx ring\n" ); +for (i =0 ; i < np->tx_ring_size; i +=4 ) { +if (!nv_optimized (np)) { +netdev_info (dev, +"%03x: %08x %08x // %08x %08x " +"// %08x %08x // %08x %08x\n" , + i, +le32_to_cpu (np->tx_ring.orig[i].buf), +le32_to_cpu (np->tx_ring.orig[i].flaglen), +le32_to_cpu (np->tx_ring.orig[i+1 ].buf), +le32_to_cpu (np->tx_ring.orig[i+1 ].flaglen), +le32_to_cpu (np->tx_ring.orig[i+2 ].buf), +le32_to_cpu (np->tx_ring.orig[i+2 ].flaglen), +le32_to_cpu (np->tx_ring.orig[i+3 ].buf), +le32_to_cpu (np->tx_ring.orig[i+3 ].flaglen)); + }else { +netdev_info (dev, +"%03x: %08x %08x %08x " +"// %08x %08x %08x " +"// %08x %08x %08x " +"// %08x %08x %08x\n" , + i, +le32_to_cpu (np->tx_ring.ex[i].bufhigh), +le32_to_cpu (np->tx_ring.ex[i].buflow), +le32_to_cpu (np->tx_ring.ex[i].flaglen), +le32_to_cpu (np->tx_ring.ex[i+1 ].bufhigh), +le32_to_cpu (np->tx_ring.ex[i+1 ].buflow), +le32_to_cpu (np->tx_ring.ex[i+1 ].flaglen), +le32_to_cpu (np->tx_ring.ex[i+2 ].bufhigh), +le32_to_cpu (np->tx_ring.ex[i+2 ].buflow), +le32_to_cpu (np->tx_ring.ex[i+2 ].flaglen), +le32_to_cpu (np->tx_ring.ex[i+3 ].bufhigh), +le32_to_cpu (np->tx_ring.ex[i+3 ].buflow), +le32_to_cpu (np->tx_ring.ex[i+3 ].flaglen)); + } } }
@@ -5984,6 +5997,9 @@ module_param(phy_cross, int, 0);MODULE_PARM_DESC (phy_cross,"Phy crossover detection for Realtek 8201 phy is enabled by setting to 1 and disabled by setting to 0." );module_param (phy_power_down, int,0 );MODULE_PARM_DESC (phy_power_down,"Power down phy and disable link when interface is down (1), or leave phy powered up (0)." ); +module_param (debug_tx_timeout, bool,0 ); +MODULE_PARM_DESC (debug_tx_timeout, +"Dump tx related registers and ring when tx_timeout happens" );MODULE_AUTHOR ("Manfred Spraul <[email protected]>" );MODULE_DESCRIPTION ("Reverse Engineered nForce ethernet driver" );
Signature · 1 line
1.7.3.1
PATCH 3/9 forcedeth: expose module parameters in /sys/module
In reply to David DecotignyIn particular, debug_tx_timeout can be updated at runtime.
Signed-off-by David Decotigny <[email protected]>
--- drivers/net/ethernet/nvidia/forcedeth.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)
drivers/net/ethernet/nvidia/forcedeth.c +9 -9
@@ -5981,23 +5981,23 @@ static void __exit exit_nic(void)pci_unregister_driver (&driver); } -module_param (max_interrupt_work, int,0 ); +module_param (max_interrupt_work, int,S_IRUGO );MODULE_PARM_DESC (max_interrupt_work,"forcedeth maximum events handled per interrupt" ); -module_param (optimization_mode, int,0 ); +module_param (optimization_mode, int,S_IRUGO );MODULE_PARM_DESC (optimization_mode,"In throughput mode (0), every tx & rx packet will generate an interrupt. In CPU mode (1), interrupts are controlled by a timer. In dynamic mode (2), the mode toggles between throughput and CPU mode based on network load." ); -module_param (poll_interval, int,0 ); +module_param (poll_interval, int,S_IRUGO );MODULE_PARM_DESC (poll_interval,"Interval determines how frequent timer interrupt is generated by [(time_in_micro_secs * 100) / (2^10)]. Min is 0 and Max is 65535." ); -module_param (msi, int,0 ); +module_param (msi, int,S_IRUGO );MODULE_PARM_DESC (msi,"MSI interrupts are enabled by setting to 1 and disabled by setting to 0." ); -module_param (msix, int,0 ); +module_param (msix, int,S_IRUGO );MODULE_PARM_DESC (msix,"MSIX interrupts are enabled by setting to 1 and disabled by setting to 0." ); -module_param (dma_64bit, int,0 ); +module_param (dma_64bit, int,S_IRUGO );MODULE_PARM_DESC (dma_64bit,"High DMA is enabled by setting to 1 and disabled by setting to 0." ); -module_param (phy_cross, int,0 ); +module_param (phy_cross, int,S_IRUGO );MODULE_PARM_DESC (phy_cross,"Phy crossover detection for Realtek 8201 phy is enabled by setting to 1 and disabled by setting to 0." ); -module_param (phy_power_down, int,0 ); +module_param (phy_power_down, int,S_IRUGO );MODULE_PARM_DESC (phy_power_down,"Power down phy and disable link when interface is down (1), or leave phy powered up (0)." ); -module_param (debug_tx_timeout, bool,0 ); +module_param (debug_tx_timeout, bool,S_IRUGO |S_IWUSR );MODULE_PARM_DESC (debug_tx_timeout,"Dump tx related registers and ring when tx_timeout happens" );
Signature · 1 line
1.7.3.1
PATCH 4/9 forcedeth: stats for rx_packets based on hardware registers
In reply to David DecotignyUse the hardware registers instead of a software implementation to account for the number of RX packets.
Signed-off-by David Decotigny <[email protected]>
--- drivers/net/ethernet/nvidia/forcedeth.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
drivers/net/ethernet/nvidia/forcedeth.c +1 -2
@@ -1712,6 +1712,7 @@ static struct net_device_stats *nv_get_stats(struct net_device *dev)nv_get_hw_stats (dev);/* copy to net_device stats */ + dev->stats.rx_packets = np->estats.rx_packets; dev->stats.tx_packets = np->estats.tx_packets; dev->stats.rx_bytes = np->estats.rx_bytes; dev->stats.tx_bytes = np->estats.tx_bytes;
@@ -2690,7 +2691,6 @@ static int nv_rx_process(struct net_device *dev, int limit)skb_put (skb, len); skb->protocol =eth_type_trans (skb, dev);napi_gro_receive (&np->napi, skb); - dev->stats.rx_packets++; next_pkt:if (unlikely (np->get_rx.orig++ == np->last_rx.orig)) np->get_rx.orig = np->first_rx.orig;
@@ -2773,7 +2773,6 @@ static int nv_rx_process_optimized(struct net_device *dev, int limit)__vlan_hwaccel_put_tag (skb, vid); }napi_gro_receive (&np->napi, skb); - dev->stats.rx_packets++; }else {dev_kfree_skb (skb); }
Signature · 1 line
1.7.3.1
Re: [PATCH net-next v1 4/9] forcedeth: stats for rx_packets based on hardware registers
Hi all,
After discussion with original author of patch "forcedeth: statistics optimization" (21828163b2b31e0675cb3e66a2a4a231dec06236), I will revise this strategy, and step back to using software counters to make the accounting of packets received at software level more accurate. Will send an update to this series shortly.
Regards,
--
David Decotigny
Quoted 42 lines · David Decotigny
Use the hardware registers instead of a software implementation to account for the number of RX packets.
Signed-off-by David Decotigny <[email protected]>---
drivers/net/ethernet/nvidia/forcedeth.c | 3 +--1 files changed, 1 insertions(+), 2 deletions(-)drivers/net/ethernet/nvidia/forcedeth.c +1
-1@@ -1712,6 +1712,7 @@ static struct net_device_stats *nv_get_stats(struct net_device *dev) nv_get_hw_stats(dev); /* copy to net_device stats */ + dev->stats.rx_packets = np->estats.rx_packets; dev->stats.tx_packets = np->estats.tx_packets; dev->stats.rx_bytes = np->estats.rx_bytes; dev->stats.tx_bytes = np->estats.tx_bytes;@@ -2690,7 +2691,6 @@ static int nv_rx_process(struct net_device *dev, int limit) skb_put(skb, len); skb->protocol = eth_type_trans(skb, dev); napi_gro_receive(&np->napi, skb); - dev->stats.rx_packets++;next_pkt:
if (unlikely(np->get_rx.orig++ == np->last_rx.orig))np->get_rx.orig=np->first_rx.orig;drivers/net/ethernet/nvidia/forcedeth.c +0
-2@@ -2773,7 +2773,6 @@ static int nv_rx_process_optimized(struct net_device *dev, int limit) __vlan_hwaccel_put_tag(skb, vid); } napi_gro_receive(&np->napi, skb); - dev->stats.rx_packets++; } else { dev_kfree_skb(skb); } --1.7.3.1
PATCH 5/9 forcedeth: implement ndo_get_stats64() API
In reply to David DecotignyThis commit implements the ndo_get_stats64() API for forcedeth. Since these stats are being updated from different contexts (process and timer), this commit adds protection (locking + atomic variables).
Tested:
16-way SMP x86_64 ->
RX bytes:7244556582 (7.2 GB) TX bytes:181904254 (181.9 MB)
Signed-off-by David Decotigny <[email protected]>
--- drivers/net/ethernet/nvidia/forcedeth.c | 132 +++++++++++++++++++++++-------- 1 files changed, 99 insertions(+), 33 deletions(-)
drivers/net/ethernet/nvidia/forcedeth.c +99 -33
@@ -692,6 +692,19 @@ struct nv_ethtool_stats {#define NV_DEV_STATISTICS_V2_COUNT (NV_DEV_STATISTICS_V3_COUNT - 3)#define NV_DEV_STATISTICS_V1_COUNT (NV_DEV_STATISTICS_V2_COUNT - 6) +/* driver statistics */ +struct nv_driver_stat { +atomic_t delta;/* increase since last nv_update_stats() */ +u64 total;/* cumulative, requires netdev_priv(dev)->stats_lock */ +}; + +#define NV_DRIVER_STAT_ATOMIC_INC (ptr_stat)/* atomic */ \ + ({ atomic_inc(&(ptr_stat)->delta); }) +#define NV_DRIVER_STAT_UPDATE_TOTAL (ptr_stat)/* requires stats_lock */ \ + ({ (ptr_stat)->total += atomic_xchg(&(ptr_stat)->delta, 0); }) +#define NV_DRIVER_STAT_GET_TOTAL (ptr_stat)/* requires stats_lock */ \ + ((ptr_stat)->total) +/* diagnostics */ #define NV_TEST_COUNT_BASE 3#define NV_TEST_COUNT_EXTENDED 4
@@ -736,6 +749,12 @@ struct nv_skb_map {* - tx setup is lockless: it relies on netif_tx_lock. Actual submission * needs netdev_priv(dev)->lock :-( * - set_multicast_list: preparation lockless, relies on netif_tx_lock. +* +* Stats are protected with stats_lock: +* - updated by nv_do_stats_poll (timer). This is meant to avoid +* integer wraparound in the NIC stats registers, at low frequency +* (0.1 Hz) +* - updated by nv_get_ethtool_stats + nv_get_stats64 */ /* in dev: base, irq */
@@ -745,9 +764,10 @@ struct fe_priv {struct net_device *dev;struct napi_struct napi; -/* General data: -* Locking: spin_lock(&np->lock); */ +/* stats are updated in syscall and timer */ +spinlock_t stats_lock;struct nv_ethtool_stats estats; +int in_shutdown;u32 linkspeed;int duplex;
@@ -797,6 +817,7 @@ struct fe_priv {struct timer_list stats_poll;u32 nic_poll_irq;int rx_ring_size; +struct nv_driver_stat stat_rx_missed_errors;/* media detection workaround. * Locking: Within irq hander or disable_irq+spin_lock(&np->lock);
@@ -819,6 +840,7 @@ struct fe_priv {struct nv_skb_map *tx_change_owner;struct nv_skb_map *tx_end_flip;int tx_stop; +struct nv_driver_stat stat_tx_dropped;/* msi/msi-x fields */ u32 msi_flags;
@@ -1635,11 +1657,19 @@ static void nv_mac_reset(struct net_device *dev)pci_push (base); } -static void nv_get_hw_stats (struct net_device *dev) +/* Caller must appropriately lock netdev_priv(dev)->stats_lock */ +static void nv_update_stats (struct net_device *dev) {struct fe_priv *np =netdev_priv (dev);u8 __iomem *base =get_hwbase (dev); +/* If it happens that this is run in top-half context, then +* replace the spin_lock of stats_lock with +* spin_lock_irqsave() in calling functions. */ +WARN_ONCE (in_irq (),"forcedeth: estats spin_lock(_bh) from top-half" ); +assert_spin_locked (&np->stats_lock); + +/* query hardware */ np->estats.tx_bytes +=readl (base + NvRegTxCnt); np->estats.tx_zero_rexmt +=readl (base + NvRegTxZeroReXmt); np->estats.tx_one_rexmt +=readl (base + NvRegTxOneReXmt);
@@ -1695,37 +1725,61 @@ static void nv_get_hw_stats(struct net_device *dev) np->estats.tx_multicast +=readl (base + NvRegTxMulticast); np->estats.tx_broadcast +=readl (base + NvRegTxBroadcast); } + +/* update software stats */ +NV_DRIVER_STAT_UPDATE_TOTAL (&np->stat_tx_dropped); +NV_DRIVER_STAT_UPDATE_TOTAL (&np->stat_rx_missed_errors); }/* -* nv_get_stats: dev->get_stats function +* nv_get_stats64: dev->ndo_get_stats64 function * Get latest stats value from the nic. * Called with read_lock(&dev_base_lock) held for read - * only synchronized against unregister_netdevice. */ -static struct net_device_stats *nv_get_stats (struct net_device *dev) +static struct rtnl_link_stats64 * +nv_get_stats64 (struct net_device *dev,struct rtnl_link_stats64 *storage) + __acquires(&netdev_priv(dev)->stats_lock) + __releases(&netdev_priv(dev)->stats_lock) {struct fe_priv *np =netdev_priv (dev);/* If the nic supports hw counters then retrieve latest values */ -if (np->driver_data & (DEV_HAS_STATISTICS_V1 |DEV_HAS_STATISTICS_V2 |DEV_HAS_STATISTICS_V3 )) { -nv_get_hw_stats (dev); - -/* copy to net_device stats */ - dev->stats.rx_packets = np->estats.rx_packets; - dev->stats.tx_packets = np->estats.tx_packets; - dev->stats.rx_bytes = np->estats.rx_bytes; - dev->stats.tx_bytes = np->estats.tx_bytes; - dev->stats.tx_fifo_errors = np->estats.tx_fifo_errors; - dev->stats.tx_carrier_errors = np->estats.tx_carrier_errors; - dev->stats.rx_crc_errors = np->estats.rx_crc_errors; - dev->stats.rx_over_errors = np->estats.rx_over_errors; - dev->stats.rx_fifo_errors = np->estats.rx_drop_frame; - dev->stats.rx_errors = np->estats.rx_errors_total; - dev->stats.tx_errors = np->estats.tx_errors_total; - } - -return &dev->stats; +if (np->driver_data & (DEV_HAS_STATISTICS_V1 + |DEV_HAS_STATISTICS_V2 + |DEV_HAS_STATISTICS_V3 )) { +spin_lock_bh (&np->stats_lock); + +nv_update_stats (dev); + +/* generic stats */ + storage->rx_packets = np->estats.rx_packets; + storage->tx_packets = np->estats.tx_packets; + storage->rx_bytes = np->estats.rx_bytes; + storage->tx_bytes = np->estats.tx_bytes; + storage->rx_errors = np->estats.rx_errors_total; + storage->tx_errors = np->estats.tx_errors_total; + storage->tx_dropped =NV_DRIVER_STAT_GET_TOTAL ( + &np->stat_tx_dropped); + storage->multicast = np->estats.rx_multicast; + +/* detailed rx_errors */ + storage->rx_length_errors = np->estats.rx_length_error; + storage->rx_over_errors = np->estats.rx_over_errors; + storage->rx_crc_errors = np->estats.rx_crc_errors; + storage->rx_frame_errors = np->estats.rx_frame_align_error; + storage->rx_fifo_errors = np->estats.rx_drop_frame; + storage->rx_missed_errors =NV_DRIVER_STAT_GET_TOTAL ( + &np->stat_rx_missed_errors); + +/* detailed tx_errors */ + storage->tx_carrier_errors = np->estats.tx_carrier_errors; + storage->tx_fifo_errors = np->estats.tx_fifo_errors; + +spin_unlock_bh (&np->stats_lock); + } + +return storage; }/*
@@ -1927,7 +1981,7 @@ static void nv_drain_tx(struct net_device *dev) np->tx_ring.ex[i].buflow =0 ; }if (nv_release_txskb (np, &np->tx_skb[i])) - dev->stats.tx_dropped++; +NV_DRIVER_STAT_ATOMIC_INC (&np->stat_tx_dropped); np->tx_skb[i].dma =0 ; np->tx_skb[i].dma_len =0 ; np->tx_skb[i].dma_single =0 ;
@@ -2648,7 +2702,7 @@ static int nv_rx_process(struct net_device *dev, int limit)/* the rest are hard errors */ else {if (flags &NV_RX_MISSEDFRAME ) - dev->stats.rx_missed_errors++; +NV_DRIVER_STAT_ATOMIC_INC (&np->stat_rx_missed_errors);dev_kfree_skb (skb); goto next_pkt; }
@@ -3900,11 +3954,18 @@ static void nv_poll_controller(struct net_device *dev)#endif static void nv_do_stats_poll (unsigned long data) + __acquires(&netdev_priv(dev)->stats_lock) + __releases(&netdev_priv(dev)->stats_lock) {struct net_device *dev = (struct net_device *) data;struct fe_priv *np =netdev_priv (dev); -nv_get_hw_stats (dev); +/* If lock is currently taken, the stats are being refreshed +* and hence fresh enough */ +if (spin_trylock (&np->stats_lock)) { +nv_update_stats (dev); +spin_unlock (&np->stats_lock); + }if (!np->in_shutdown) mod_timer(&np->stats_poll,
@@ -4549,14 +4610,18 @@ static int nv_get_sset_count(struct net_device *dev, int sset) } } -static void nv_get_ethtool_stats (struct net_device *dev,struct ethtool_stats *estats,u64 *buffer) +static void nv_get_ethtool_stats (struct net_device *dev, +struct ethtool_stats *estats,u64 *buffer) + __acquires(&netdev_priv(dev)->stats_lock) + __releases(&netdev_priv(dev)->stats_lock) {struct fe_priv *np =netdev_priv (dev); -/* update stats */ -nv_get_hw_stats (dev); - -memcpy (buffer, &np->estats,nv_get_sset_count (dev,ETH_SS_STATS )*sizeof (u64)); +spin_lock_bh (&np->stats_lock); +nv_update_stats (dev); +memcpy (buffer, &np->estats, +nv_get_sset_count (dev,ETH_SS_STATS )*sizeof (u64)); +spin_unlock_bh (&np->stats_lock); }static int nv_link_test (struct net_device *dev)
@@ -5194,7 +5259,7 @@ static int nv_close(struct net_device *dev)static const struct net_device_ops nv_netdev_ops = { .ndo_open = nv_open, .ndo_stop = nv_close, - .ndo_get_stats = nv_get_stats, + .ndo_get_stats64 = nv_get_stats64, .ndo_start_xmit = nv_start_xmit, .ndo_tx_timeout = nv_tx_timeout, .ndo_change_mtu = nv_change_mtu,
@@ -5211,7 +5276,7 @@ static const struct net_device_ops nv_netdev_ops = {static const struct net_device_ops nv_netdev_ops_optimized = { .ndo_open = nv_open, .ndo_stop = nv_close, - .ndo_get_stats = nv_get_stats, + .ndo_get_stats64 = nv_get_stats64, .ndo_start_xmit = nv_start_xmit_optimized, .ndo_tx_timeout = nv_tx_timeout, .ndo_change_mtu = nv_change_mtu,
@@ -5250,6 +5315,7 @@ static int __devinit nv_probe(struct pci_dev *pci_dev, const struct pci_device_i np->dev = dev; np->pci_dev = pci_dev;spin_lock_init (&np->lock); +spin_lock_init (&np->stats_lock);SET_NETDEV_DEV (dev, &pci_dev->dev);init_timer (&np->oom_kick);
Signature · 1 line
1.7.3.1
Re: [PATCH net-next v1 5/9] forcedeth: implement ndo_get_stats64() API
Quoted 3 lines · David Decotigny
This commit implements the
ndo_get_stats64()API for forcedeth. Since these stats are being updated from different contexts (process and timer), this commit adds protection (locking + atomic variables).
[...]
I don't claim to have reviewed this properly, but it looks reasonable.
Ben.
Signature · 3 lines
Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.
PATCH 6/9 forcedeth: account for dropped RX frames
In reply to David DecotignyThis adds the stats counter for dropped RX frames.
Signed-off-by David Decotigny <[email protected]>
--- drivers/net/ethernet/nvidia/forcedeth.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-)
drivers/net/ethernet/nvidia/forcedeth.c +10 -2
@@ -818,6 +818,7 @@ struct fe_priv {u32 nic_poll_irq;int rx_ring_size;struct nv_driver_stat stat_rx_missed_errors; +struct nv_driver_stat stat_rx_dropped;/* media detection workaround. * Locking: Within irq hander or disable_irq+spin_lock(&np->lock);
@@ -1727,6 +1728,7 @@ static void nv_update_stats(struct net_device *dev) }/* update software stats */ +NV_DRIVER_STAT_UPDATE_TOTAL (&np->stat_rx_dropped);NV_DRIVER_STAT_UPDATE_TOTAL (&np->stat_tx_dropped);NV_DRIVER_STAT_UPDATE_TOTAL (&np->stat_rx_missed_errors); }
@@ -1759,6 +1761,8 @@ nv_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *storage) storage->tx_bytes = np->estats.tx_bytes; storage->rx_errors = np->estats.rx_errors_total; storage->tx_errors = np->estats.tx_errors_total; + storage->rx_dropped =NV_DRIVER_STAT_GET_TOTAL ( + &np->stat_rx_dropped); storage->tx_dropped =NV_DRIVER_STAT_GET_TOTAL ( &np->stat_tx_dropped); storage->multicast = np->estats.rx_multicast;
@@ -1812,8 +1816,10 @@ static int nv_alloc_rx(struct net_device *dev) np->put_rx.orig = np->first_rx.orig;if (unlikely (np->put_rx_ctx++ == np->last_rx_ctx)) np->put_rx_ctx = np->first_rx_ctx; - }else + }else { +NV_DRIVER_STAT_ATOMIC_INC (&np->stat_rx_dropped);return 1 ; + } }return 0 ; }
@@ -1844,8 +1850,10 @@ static int nv_alloc_rx_optimized(struct net_device *dev) np->put_rx.ex = np->first_rx.ex;if (unlikely (np->put_rx_ctx++ == np->last_rx_ctx)) np->put_rx_ctx = np->first_rx_ctx; - }else + }else { +NV_DRIVER_STAT_ATOMIC_INC (&np->stat_rx_dropped);return 1 ; + } }return 0 ; }
Signature · 1 line
1.7.3.1
PATCH 7/9 forcedeth: new ethtool stat counter for TX timeouts
In reply to David DecotignyFrom: Sameer Nanda <[email protected]>
This change publishes a new ethtool stats: tx_timeout that counts the number of times the tx_timeout callback was triggered.
Signed-off-by David Decotigny <[email protected]>
--- drivers/net/ethernet/nvidia/forcedeth.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
drivers/net/ethernet/nvidia/forcedeth.c +6 -0
@@ -633,6 +633,7 @@ static const struct nv_ethtool_str nv_estats_str[] = { { "rx_packets" }, { "rx_errors_total" }, { "tx_errors_total" }, + { "tx_timeout" },/* version 2 stats */ { "tx_deferral" },
@@ -673,6 +674,7 @@ struct nv_ethtool_stats {u64 rx_packets;u64 rx_errors_total;u64 tx_errors_total; +u64 tx_timeout;/* version 2 stats */ u64 tx_deferral;
@@ -842,6 +844,7 @@ struct fe_priv {struct nv_skb_map *tx_end_flip;int tx_stop;struct nv_driver_stat stat_tx_dropped; +atomic_t stat_tx_timeout;/* TX timeouts since last nv_update_stats */ /* msi/msi-x fields */ u32 msi_flags;
@@ -1728,6 +1731,7 @@ static void nv_update_stats(struct net_device *dev) }/* update software stats */ + np->estats.tx_timeout +=atomic_xchg (&np->stat_tx_timeout,0 );NV_DRIVER_STAT_UPDATE_TOTAL (&np->stat_rx_dropped);NV_DRIVER_STAT_UPDATE_TOTAL (&np->stat_tx_dropped);NV_DRIVER_STAT_UPDATE_TOTAL (&np->stat_rx_missed_errors);
@@ -2590,6 +2594,8 @@ static void nv_tx_timeout(struct net_device *dev) } } +atomic_inc (&np->stat_tx_timeout); +spin_lock_irq (&np->lock);/* 1) stop tx engine */
Signature · 1 line
1.7.3.1
PATCH 8/9 forcedeth: stats updated with a deferrable timer
In reply to David DecotignyStats don't need to be updated periodically unless there is at least an IRQ: mark stats timer as deferrable.
Signed-off-by David Decotigny <[email protected]>
--- drivers/net/ethernet/nvidia/forcedeth.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/ethernet/nvidia/forcedeth.c +1 -1
@@ -5338,7 +5338,7 @@ static int __devinit nv_probe(struct pci_dev *pci_dev, const struct pci_device_iinit_timer (&np->nic_poll); np->nic_poll.data = (unsigned long ) dev; np->nic_poll.function = nv_do_nic_poll;/* timer handler */ -init_timer (&np->stats_poll); +init_timer_deferrable (&np->stats_poll); np->stats_poll.data = (unsigned long ) dev; np->stats_poll.function = nv_do_stats_poll;/* timer handler */
Signature · 1 line
1.7.3.1
PATCH 9/9 forcedeth: whitespace/indentation fixes
In reply to David DecotignySigned-off-by David Decotigny <[email protected]>
--- drivers/net/ethernet/nvidia/forcedeth.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/ethernet/nvidia/forcedeth.c +1 -1
@@ -65,7 +65,7 @@#include <linux/slab.h> #include <linux/uaccess.h> #include <linux/prefetch.h> -#include <linux/io.h> +#include <linux/io.h> #include <asm/irq.h> #include <asm/system.h>
Signature · 1 line
1.7.3.1
Quoted 9 lines · David Decotigny
These changes implement the
ndo_get_stats64API and add a few more stats and debugging features for forcedeth. They also ensure that stats updates are correct in SMP systems, 32 or 64-bits.Regarding the "implement
ndo_get_stats64()API" patch, I'm not sure I'm using the right way to protect the 64b stats. Ideally, I would like them to be non-blocking (u64_stats_sync.h), but as there are several sources for updates, I don't think I can do without locking or per-CPU stats.
The important thing is that the fast path remains fast, and I think you've achieved that (due to patch 4/9). The data path doesn't have to acquire the stats lock and it only has to use atomic operations in some error cases.
Quoted 2 lines
Would per-CPU stats be better here (note: I expect the contention on netdev_priv(dev)->stats_lock to be _VERY_ low)?
[...]
Only queue-less software devices should maintain per-CPU stats. For any device with queues, servicing of each queue must already be serialised and software stats can be maintained per-queue. In the single-queue case this means per-device.
Ben.
Signature · 3 lines
Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.