* [PATCH net-next v4 1/8] forcedeth: fix stats on hardware without extended stats support
2011-11-15 19:25 [PATCH net-next v4 0/8] forcedeth: stats & debug enhancements David Decotigny
@ 2011-11-15 19:25 ` David Decotigny
2011-11-15 19:25 ` [PATCH net-next v4 2/8] forcedeth: Add messages to indicate using MSI or MSI-X David Decotigny
` (6 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: David Decotigny @ 2011-11-15 19:25 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: David S. Miller, Ian Campbell, Eric Dumazet, Jeff Kirsher,
Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla, david decotigny
From: david decotigny <david.decotigny@google.com>
This change makes sure that tx_packets/rx_bytes ifconfig counters are
updated even on NICs that don't provide hardware support for these
stats: they are now updated in software. For the sake of consistency,
we also now have tx_bytes updated in software (hardware counters
include ethernet CRC, and software doesn't account for it).
This reverts parts of:
- "forcedeth: statistics optimization" (21828163b2)
- "forcedeth: Improve stats counters" (0bdfea8ba8)
- "forcedeth: remove unneeded stats updates" (4687f3f364)
Tested:
pktgen + loopback (http://patchwork.ozlabs.org/patch/124698/)
reports identical tx_packets/rx_packets and tx_bytes/rx_bytes.
Signed-off-by: David Decotigny <david.decotigny@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
(cherry picked from commit 898bdf2cb43eb0a962c397eb4dd1aec2c7211be2)
---
drivers/net/ethernet/nvidia/forcedeth.c | 36 +++++++++++++++++++++++-------
1 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index e8a5ae3..374625b 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -609,7 +609,7 @@ struct nv_ethtool_str {
};
static const struct nv_ethtool_str nv_estats_str[] = {
- { "tx_bytes" },
+ { "tx_bytes" }, /* includes Ethernet FCS CRC */
{ "tx_zero_rexmt" },
{ "tx_one_rexmt" },
{ "tx_many_rexmt" },
@@ -637,7 +637,7 @@ static const struct nv_ethtool_str nv_estats_str[] = {
/* version 2 stats */
{ "tx_deferral" },
{ "tx_packets" },
- { "rx_bytes" },
+ { "rx_bytes" }, /* includes Ethernet FCS CRC */
{ "tx_pause" },
{ "rx_pause" },
{ "rx_drop_frame" },
@@ -649,7 +649,7 @@ static const struct nv_ethtool_str nv_estats_str[] = {
};
struct nv_ethtool_stats {
- u64 tx_bytes;
+ u64 tx_bytes; /* should be ifconfig->tx_bytes + 4*tx_packets */
u64 tx_zero_rexmt;
u64 tx_one_rexmt;
u64 tx_many_rexmt;
@@ -670,14 +670,14 @@ struct nv_ethtool_stats {
u64 rx_unicast;
u64 rx_multicast;
u64 rx_broadcast;
- u64 rx_packets;
+ u64 rx_packets; /* should be ifconfig->rx_packets */
u64 rx_errors_total;
u64 tx_errors_total;
/* version 2 stats */
u64 tx_deferral;
- u64 tx_packets;
- u64 rx_bytes;
+ u64 tx_packets; /* should be ifconfig->tx_packets */
+ u64 rx_bytes; /* should be ifconfig->rx_bytes + 4*rx_packets */
u64 tx_pause;
u64 rx_pause;
u64 rx_drop_frame;
@@ -1706,10 +1706,17 @@ static struct net_device_stats *nv_get_stats(struct net_device *dev)
if (np->driver_data & (DEV_HAS_STATISTICS_V1|DEV_HAS_STATISTICS_V2|DEV_HAS_STATISTICS_V3)) {
nv_get_hw_stats(dev);
+ /*
+ * Note: because HW stats are not always available and
+ * for consistency reasons, the following ifconfig
+ * stats are managed by software: rx_bytes, tx_bytes,
+ * rx_packets and tx_packets. The related hardware
+ * stats reported by ethtool should be equivalent to
+ * these ifconfig stats, with 4 additional bytes per
+ * packet (Ethernet FCS CRC).
+ */
+
/* copy to net_device stats */
- 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;
@@ -2380,6 +2387,9 @@ static int nv_tx_done(struct net_device *dev, int limit)
if (flags & NV_TX_ERROR) {
if ((flags & NV_TX_RETRYERROR) && !(flags & NV_TX_RETRYCOUNT_MASK))
nv_legacybackoff_reseed(dev);
+ } else {
+ dev->stats.tx_packets++;
+ dev->stats.tx_bytes += np->get_tx_ctx->skb->len;
}
dev_kfree_skb_any(np->get_tx_ctx->skb);
np->get_tx_ctx->skb = NULL;
@@ -2390,6 +2400,9 @@ static int nv_tx_done(struct net_device *dev, int limit)
if (flags & NV_TX2_ERROR) {
if ((flags & NV_TX2_RETRYERROR) && !(flags & NV_TX2_RETRYCOUNT_MASK))
nv_legacybackoff_reseed(dev);
+ } else {
+ dev->stats.tx_packets++;
+ dev->stats.tx_bytes += np->get_tx_ctx->skb->len;
}
dev_kfree_skb_any(np->get_tx_ctx->skb);
np->get_tx_ctx->skb = NULL;
@@ -2429,6 +2442,9 @@ static int nv_tx_done_optimized(struct net_device *dev, int limit)
else
nv_legacybackoff_reseed(dev);
}
+ } else {
+ dev->stats.tx_packets++;
+ dev->stats.tx_bytes += np->get_tx_ctx->skb->len;
}
dev_kfree_skb_any(np->get_tx_ctx->skb);
@@ -2678,6 +2694,7 @@ static int nv_rx_process(struct net_device *dev, int limit)
skb->protocol = eth_type_trans(skb, dev);
napi_gro_receive(&np->napi, skb);
dev->stats.rx_packets++;
+ dev->stats.rx_bytes += len;
next_pkt:
if (unlikely(np->get_rx.orig++ == np->last_rx.orig))
np->get_rx.orig = np->first_rx.orig;
@@ -2761,6 +2778,7 @@ static int nv_rx_process_optimized(struct net_device *dev, int limit)
}
napi_gro_receive(&np->napi, skb);
dev->stats.rx_packets++;
+ dev->stats.rx_bytes += len;
} else {
dev_kfree_skb(skb);
}
--
1.7.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH net-next v4 2/8] forcedeth: Add messages to indicate using MSI or MSI-X
2011-11-15 19:25 [PATCH net-next v4 0/8] forcedeth: stats & debug enhancements David Decotigny
2011-11-15 19:25 ` [PATCH net-next v4 1/8] forcedeth: fix stats on hardware without extended stats support David Decotigny
@ 2011-11-15 19:25 ` David Decotigny
2011-11-15 19:32 ` Joe Perches
[not found] ` <cover.1321386214.git.david.decotigny@google.com>
2011-11-15 19:25 ` [PATCH net-next v4 3/8] forcedeth: allow to silence "TX timeout" debug messages David Decotigny
` (5 subsequent siblings)
7 siblings, 2 replies; 17+ messages in thread
From: David Decotigny @ 2011-11-15 19:25 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: David S. Miller, Ian Campbell, Eric Dumazet, Jeff Kirsher,
Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla, Mike Ditto, David Decotigny
From: Mike Ditto <mditto@google.com>
This adds a few debug messages to indicate whether PCIe interrupts are
signaled with MSI or MSI-X.
Signed-off-by: David Decotigny <david.decotigny@google.com>
---
drivers/net/ethernet/nvidia/forcedeth.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 374625b..fe17e42 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -3810,6 +3810,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) {
@@ -3831,6 +3832,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) {
--
1.7.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v4 2/8] forcedeth: Add messages to indicate using MSI or MSI-X
2011-11-15 19:25 ` [PATCH net-next v4 2/8] forcedeth: Add messages to indicate using MSI or MSI-X David Decotigny
@ 2011-11-15 19:32 ` Joe Perches
[not found] ` <cover.1321386214.git.david.decotigny@google.com>
1 sibling, 0 replies; 17+ messages in thread
From: Joe Perches @ 2011-11-15 19:32 UTC (permalink / raw)
To: David Decotigny
Cc: netdev, linux-kernel, David S. Miller, Ian Campbell, Eric Dumazet,
Jeff Kirsher, Ben Hutchings, Jiri Pirko, Szymon Janc,
Richard Jones, Ayaz Abdulla, Mike Ditto
On Tue, 2011-11-15 at 11:25 -0800, David Decotigny wrote:
> From: Mike Ditto <mditto@google.com>
>
> This adds a few debug messages to indicate whether PCIe interrupts are
> signaled with MSI or MSI-X.
Perhaps the changelog would be better without "debug".
These added messages aren't really debug messages.
They are emitted as netdev_info vs netdev_dbg.
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
[]
> @@ -3810,6 +3810,7 @@ static int nv_request_irq(struct net_device *dev, int intr_test)
> + netdev_info(dev, "MSI-X enabled\n");
> @@ -3831,6 +3832,7 @@ static int nv_request_irq(struct net_device *dev, int intr_test)
> + netdev_info(dev, "MSI enabled\n");
^ permalink raw reply [flat|nested] 17+ messages in thread[parent not found: <cover.1321386214.git.david.decotigny@google.com>]
* [PATCH net-next v4 2/8] forcedeth: Add messages to indicate using MSI or MSI-X
[not found] ` <cover.1321386214.git.david.decotigny@google.com>
@ 2011-11-15 19:51 ` David Decotigny
0 siblings, 0 replies; 17+ messages in thread
From: David Decotigny @ 2011-11-15 19:51 UTC (permalink / raw)
To: Joe Perches, netdev, linux-kernel
Cc: David S. Miller, Mike Ditto, David Decotigny
From: Mike Ditto <mditto@google.com>
This adds a few kernel messages to indicate whether PCIe interrupts
are signaled with MSI or MSI-X.
Signed-off-by: David Decotigny <david.decotigny@google.com>
---
drivers/net/ethernet/nvidia/forcedeth.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 374625b..fe17e42 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -3810,6 +3810,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) {
@@ -3831,6 +3832,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) {
--
1.7.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH net-next v4 3/8] forcedeth: allow to silence "TX timeout" debug messages
2011-11-15 19:25 [PATCH net-next v4 0/8] forcedeth: stats & debug enhancements David Decotigny
2011-11-15 19:25 ` [PATCH net-next v4 1/8] forcedeth: fix stats on hardware without extended stats support David Decotigny
2011-11-15 19:25 ` [PATCH net-next v4 2/8] forcedeth: Add messages to indicate using MSI or MSI-X David Decotigny
@ 2011-11-15 19:25 ` David Decotigny
2011-11-15 22:27 ` Stephen Hemminger
2011-11-15 19:25 ` [PATCH net-next v4 4/8] forcedeth: expose module parameters in /sys/module David Decotigny
` (4 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: David Decotigny @ 2011-11-15 19:25 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: David S. Miller, Ian Campbell, Eric Dumazet, Jeff Kirsher,
Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla, Sameer Nanda, David Decotigny
From: Sameer Nanda <snanda@google.com>
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.
Signed-off-by: David Decotigny <david.decotigny@google.com>
---
drivers/net/ethernet/nvidia/forcedeth.c | 98 ++++++++++++++++++-------------
1 files changed, 57 insertions(+), 41 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index fe17e42..9b917ff 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -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.
*/
@@ -2477,56 +2482,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));
+ }
}
}
@@ -6156,6 +6169,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 <manfred@colorfullife.com>");
MODULE_DESCRIPTION("Reverse Engineered nForce ethernet driver");
--
1.7.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v4 3/8] forcedeth: allow to silence "TX timeout" debug messages
2011-11-15 19:25 ` [PATCH net-next v4 3/8] forcedeth: allow to silence "TX timeout" debug messages David Decotigny
@ 2011-11-15 22:27 ` Stephen Hemminger
0 siblings, 0 replies; 17+ messages in thread
From: Stephen Hemminger @ 2011-11-15 22:27 UTC (permalink / raw)
To: David Decotigny
Cc: netdev, linux-kernel, David S. Miller, Ian Campbell, Eric Dumazet,
Jeff Kirsher, Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla, Sameer Nanda
On Tue, 15 Nov 2011 11:25:36 -0800
David Decotigny <david.decotigny@google.com> wrote:
> From: Sameer Nanda <snanda@google.com>
>
> 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.
>
>
>
> Signed-off-by: David Decotigny <david.decotigny@google.com>
This (and the counter) should really be generic. I know it is more annoying
to have to solve a generic problem, but putting my distributor hat on,
any solution that is specific to only one driver is not a solution that
is useful.
The control of tx_timeout should be a property of the device, and the statistic
should be available for all devices. There is a problem though, the existing
network device statistics structure is part of ABI and can't grow. You can
add new statistics to netlink and sysfs as attributes, but not for the older
static API's.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v4 4/8] forcedeth: expose module parameters in /sys/module
2011-11-15 19:25 [PATCH net-next v4 0/8] forcedeth: stats & debug enhancements David Decotigny
` (2 preceding siblings ...)
2011-11-15 19:25 ` [PATCH net-next v4 3/8] forcedeth: allow to silence "TX timeout" debug messages David Decotigny
@ 2011-11-15 19:25 ` David Decotigny
2011-11-15 22:32 ` Stephen Hemminger
2011-11-15 22:33 ` Stephen Hemminger
2011-11-15 19:25 ` [PATCH net-next v4 5/8] forcedeth: implement ndo_get_stats64() API David Decotigny
` (3 subsequent siblings)
7 siblings, 2 replies; 17+ messages in thread
From: David Decotigny @ 2011-11-15 19:25 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: David S. Miller, Ian Campbell, Eric Dumazet, Jeff Kirsher,
Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla, David Decotigny
In particular, debug_tx_timeout can be updated at runtime.
Signed-off-by: David Decotigny <david.decotigny@google.com>
---
drivers/net/ethernet/nvidia/forcedeth.c | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 9b917ff..ee8cce5 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -6153,23 +6153,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");
--
1.7.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v4 4/8] forcedeth: expose module parameters in /sys/module
2011-11-15 19:25 ` [PATCH net-next v4 4/8] forcedeth: expose module parameters in /sys/module David Decotigny
@ 2011-11-15 22:32 ` Stephen Hemminger
2011-11-15 22:33 ` Stephen Hemminger
1 sibling, 0 replies; 17+ messages in thread
From: Stephen Hemminger @ 2011-11-15 22:32 UTC (permalink / raw)
To: David Decotigny
Cc: netdev, linux-kernel, David S. Miller, Ian Campbell, Eric Dumazet,
Jeff Kirsher, Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla
On Tue, 15 Nov 2011 11:25:37 -0800
David Decotigny <david.decotigny@google.com> wrote:
> +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.");
Probably the original developer (or marketing data sheet), thought this was some
unique feature of the hardware. But most devices have this already.
This driver should just implement proper control irq coalescing control via ethtool
and get rid of the silly module parameter.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v4 4/8] forcedeth: expose module parameters in /sys/module
2011-11-15 19:25 ` [PATCH net-next v4 4/8] forcedeth: expose module parameters in /sys/module David Decotigny
2011-11-15 22:32 ` Stephen Hemminger
@ 2011-11-15 22:33 ` Stephen Hemminger
1 sibling, 0 replies; 17+ messages in thread
From: Stephen Hemminger @ 2011-11-15 22:33 UTC (permalink / raw)
To: David Decotigny
Cc: netdev, linux-kernel, David S. Miller, Ian Campbell, Eric Dumazet,
Jeff Kirsher, Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla
On Tue, 15 Nov 2011 11:25:37 -0800
David Decotigny <david.decotigny@google.com> wrote:
> +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);
Once again these attributes are visible through other means (/proc/interrupts for MSI)
and the 64bit dma is NETIF_F_HIGHDMA. They shouldn't be module parameters.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v4 5/8] forcedeth: implement ndo_get_stats64() API
2011-11-15 19:25 [PATCH net-next v4 0/8] forcedeth: stats & debug enhancements David Decotigny
` (3 preceding siblings ...)
2011-11-15 19:25 ` [PATCH net-next v4 4/8] forcedeth: expose module parameters in /sys/module David Decotigny
@ 2011-11-15 19:25 ` David Decotigny
2011-11-15 22:01 ` David Decotigny
2011-11-15 19:25 ` [PATCH net-next v4 6/8] forcedeth: account for dropped RX frames David Decotigny
` (2 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: David Decotigny @ 2011-11-15 19:25 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: David S. Miller, Ian Campbell, Eric Dumazet, Jeff Kirsher,
Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla, David Decotigny
This commit implements the ndo_get_stats64() API for forcedeth. Since
hardware stats are being updated from different contexts (process and
timer), this commit adds protection (locking + atomic variables). For
software stats, it relies on the u64_stats_sync.h API.
Tested:
- 16-way SMP x86_64 ->
RX bytes:7244556582 (7.2 GB) TX bytes:181904254 (181.9 MB)
- pktgen + loopback: identical rx_bytes/tx_bytes and rx_packets/tx_packets
Signed-off-by: David Decotigny <david.decotigny@google.com>
---
drivers/net/ethernet/nvidia/forcedeth.c | 195 +++++++++++++++++++++++--------
1 files changed, 144 insertions(+), 51 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index ee8cce5..ff01d5e 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -65,7 +65,8 @@
#include <linux/slab.h>
#include <linux/uaccess.h>
#include <linux/prefetch.h>
-#include <linux/io.h>
+#include <linux/u64_stats_sync.h>
+#include <linux/io.h>
#include <asm/irq.h>
#include <asm/system.h>
@@ -736,6 +737,18 @@ 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.
+ *
+ * Hardware stats updates are protected by hwstats_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
+ *
+ * Software stats are accessed only through a 64b synchronization
+ * point and are not subject to other synchronization techniques (one
+ * unique updating thread for each stat [single queue RX/TX fast
+ * paths], or callers already synchronized [for tx_dropped, except from
+ * nv_open/nv_close]).
*/
/* in dev: base, irq */
@@ -745,9 +758,13 @@ struct fe_priv {
struct net_device *dev;
struct napi_struct napi;
- /* General data:
- * Locking: spin_lock(&np->lock); */
+ /* hardware stats are updated in syscall and timer */
+ spinlock_t hwstats_lock;
struct nv_ethtool_stats estats;
+
+ /* software stats are accessed through a 64b synchronization point */
+ struct u64_stats_sync swstats_syncp;
+
int in_shutdown;
u32 linkspeed;
int duplex;
@@ -798,6 +815,11 @@ struct fe_priv {
u32 nic_poll_irq;
int rx_ring_size;
+ /* RX software stats */
+ u64 stat_rx_packets;
+ u64 stat_rx_bytes; /* not always available in HW */
+ u64 stat_rx_missed_errors;
+
/* media detection workaround.
* Locking: Within irq hander or disable_irq+spin_lock(&np->lock);
*/
@@ -820,6 +842,11 @@ struct fe_priv {
struct nv_skb_map *tx_end_flip;
int tx_stop;
+ /* TX software stats */
+ u64 stat_tx_packets; /* not always available in HW */
+ u64 stat_tx_bytes;
+ u64 stat_tx_dropped;
+
/* msi/msi-x fields */
u32 msi_flags;
struct msix_entry msi_x_entry[NV_MSI_X_MAX_VECTORS];
@@ -1635,11 +1662,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)->hwstats_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 hwstats_lock with
+ * spin_lock_irqsave() in calling functions. */
+ WARN_ONCE(in_irq(), "forcedeth: estats spin_lock(_bh) from top-half");
+ assert_spin_locked(&np->hwstats_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);
@@ -1698,40 +1733,67 @@ static void nv_get_hw_stats(struct net_device *dev)
}
/*
- * 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)->hwstats_lock)
+ __releases(&netdev_priv(dev)->hwstats_lock)
{
struct fe_priv *np = netdev_priv(dev);
+ unsigned int syncp_start;
+
+ /*
+ * Note: because HW stats are not always available and for
+ * consistency reasons, the following ifconfig stats are
+ * managed by software: rx_bytes, tx_bytes, rx_packets and
+ * tx_packets. The related hardware stats reported by ethtool
+ * should be equivalent to these ifconfig stats, with 4
+ * additional bytes per packet (Ethernet FCS CRC).
+ */
+
+ /* software stats */
+ do {
+ syncp_start = u64_stats_fetch_begin(&np->swstats_syncp);
+ storage->rx_packets = np->stat_rx_packets;
+ storage->tx_packets = np->stat_tx_packets;
+ storage->rx_bytes = np->stat_rx_bytes;
+ storage->tx_bytes = np->stat_tx_bytes;
+ storage->tx_dropped = np->stat_tx_dropped;
+ storage->rx_missed_errors = np->stat_rx_missed_errors;
+ } while (u64_stats_fetch_retry(&np->swstats_syncp, syncp_start));
/* 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);
+ if (np->driver_data & DEV_HAS_STATISTICS_V123) {
+ spin_lock_bh(&np->hwstats_lock);
- /*
- * Note: because HW stats are not always available and
- * for consistency reasons, the following ifconfig
- * stats are managed by software: rx_bytes, tx_bytes,
- * rx_packets and tx_packets. The related hardware
- * stats reported by ethtool should be equivalent to
- * these ifconfig stats, with 4 additional bytes per
- * packet (Ethernet FCS CRC).
- */
+ nv_update_stats(dev);
+
+ /* generic stats */
+ storage->rx_errors = np->estats.rx_errors_total;
+ storage->tx_errors = np->estats.tx_errors_total;
+
+ /* meaningful only when NIC supports stats v3 */
+ 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;
- /* copy to net_device stats */
- 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;
+ /* 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->hwstats_lock);
}
- return &dev->stats;
+ return storage;
}
/*
@@ -1932,8 +1994,11 @@ static void nv_drain_tx(struct net_device *dev)
np->tx_ring.ex[i].bufhigh = 0;
np->tx_ring.ex[i].buflow = 0;
}
- if (nv_release_txskb(np, &np->tx_skb[i]))
- dev->stats.tx_dropped++;
+ if (nv_release_txskb(np, &np->tx_skb[i])) {
+ u64_stats_update_begin(&np->swstats_syncp);
+ np->stat_tx_dropped++;
+ u64_stats_update_end(&np->swstats_syncp);
+ }
np->tx_skb[i].dma = 0;
np->tx_skb[i].dma_len = 0;
np->tx_skb[i].dma_single = 0;
@@ -2390,11 +2455,14 @@ static int nv_tx_done(struct net_device *dev, int limit)
if (np->desc_ver == DESC_VER_1) {
if (flags & NV_TX_LASTPACKET) {
if (flags & NV_TX_ERROR) {
- if ((flags & NV_TX_RETRYERROR) && !(flags & NV_TX_RETRYCOUNT_MASK))
+ if ((flags & NV_TX_RETRYERROR)
+ && !(flags & NV_TX_RETRYCOUNT_MASK))
nv_legacybackoff_reseed(dev);
} else {
- dev->stats.tx_packets++;
- dev->stats.tx_bytes += np->get_tx_ctx->skb->len;
+ u64_stats_update_begin(&np->swstats_syncp);
+ np->stat_tx_packets++;
+ np->stat_tx_bytes += np->get_tx_ctx->skb->len;
+ u64_stats_update_end(&np->swstats_syncp);
}
dev_kfree_skb_any(np->get_tx_ctx->skb);
np->get_tx_ctx->skb = NULL;
@@ -2403,11 +2471,14 @@ static int nv_tx_done(struct net_device *dev, int limit)
} else {
if (flags & NV_TX2_LASTPACKET) {
if (flags & NV_TX2_ERROR) {
- if ((flags & NV_TX2_RETRYERROR) && !(flags & NV_TX2_RETRYCOUNT_MASK))
+ if ((flags & NV_TX2_RETRYERROR)
+ && !(flags & NV_TX2_RETRYCOUNT_MASK))
nv_legacybackoff_reseed(dev);
} else {
- dev->stats.tx_packets++;
- dev->stats.tx_bytes += np->get_tx_ctx->skb->len;
+ u64_stats_update_begin(&np->swstats_syncp);
+ np->stat_tx_packets++;
+ np->stat_tx_bytes += np->get_tx_ctx->skb->len;
+ u64_stats_update_end(&np->swstats_syncp);
}
dev_kfree_skb_any(np->get_tx_ctx->skb);
np->get_tx_ctx->skb = NULL;
@@ -2441,15 +2512,18 @@ static int nv_tx_done_optimized(struct net_device *dev, int limit)
if (flags & NV_TX2_LASTPACKET) {
if (flags & NV_TX2_ERROR) {
- if ((flags & NV_TX2_RETRYERROR) && !(flags & NV_TX2_RETRYCOUNT_MASK)) {
+ if ((flags & NV_TX2_RETRYERROR)
+ && !(flags & NV_TX2_RETRYCOUNT_MASK)) {
if (np->driver_data & DEV_HAS_GEAR_MODE)
nv_gear_backoff_reseed(dev);
else
nv_legacybackoff_reseed(dev);
}
} else {
- dev->stats.tx_packets++;
- dev->stats.tx_bytes += np->get_tx_ctx->skb->len;
+ u64_stats_update_begin(&np->swstats_syncp);
+ np->stat_tx_packets++;
+ np->stat_tx_bytes += np->get_tx_ctx->skb->len;
+ u64_stats_update_end(&np->swstats_syncp);
}
dev_kfree_skb_any(np->get_tx_ctx->skb);
@@ -2662,8 +2736,11 @@ 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++;
+ if (flags & NV_RX_MISSEDFRAME) {
+ u64_stats_update_begin(&np->swstats_syncp);
+ np->stat_rx_missed_errors++;
+ u64_stats_update_end(&np->swstats_syncp);
+ }
dev_kfree_skb(skb);
goto next_pkt;
}
@@ -2706,8 +2783,10 @@ 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++;
- dev->stats.rx_bytes += len;
+ u64_stats_update_begin(&np->swstats_syncp);
+ np->stat_rx_packets++;
+ np->stat_rx_bytes += len;
+ u64_stats_update_end(&np->swstats_syncp);
next_pkt:
if (unlikely(np->get_rx.orig++ == np->last_rx.orig))
np->get_rx.orig = np->first_rx.orig;
@@ -2790,8 +2869,10 @@ 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++;
- dev->stats.rx_bytes += len;
+ u64_stats_update_begin(&np->swstats_syncp);
+ np->stat_rx_packets++;
+ np->stat_rx_bytes += len;
+ u64_stats_update_end(&np->swstats_syncp);
} else {
dev_kfree_skb(skb);
}
@@ -4000,11 +4081,18 @@ static void nv_poll_controller(struct net_device *dev)
#endif
static void nv_do_stats_poll(unsigned long data)
+ __acquires(&netdev_priv(dev)->hwstats_lock)
+ __releases(&netdev_priv(dev)->hwstats_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->hwstats_lock)) {
+ nv_update_stats(dev);
+ spin_unlock(&np->hwstats_lock);
+ }
if (!np->in_shutdown)
mod_timer(&np->stats_poll,
@@ -4711,14 +4799,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)->hwstats_lock)
+ __releases(&netdev_priv(dev)->hwstats_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->hwstats_lock);
+ nv_update_stats(dev);
+ memcpy(buffer, &np->estats,
+ nv_get_sset_count(dev, ETH_SS_STATS)*sizeof(u64));
+ spin_unlock_bh(&np->hwstats_lock);
}
static int nv_link_test(struct net_device *dev)
@@ -5362,7 +5454,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,
@@ -5379,7 +5471,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,
@@ -5418,6 +5510,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->hwstats_lock);
SET_NETDEV_DEV(dev, &pci_dev->dev);
init_timer(&np->oom_kick);
--
1.7.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v4 5/8] forcedeth: implement ndo_get_stats64() API
2011-11-15 19:25 ` [PATCH net-next v4 5/8] forcedeth: implement ndo_get_stats64() API David Decotigny
@ 2011-11-15 22:01 ` David Decotigny
0 siblings, 0 replies; 17+ messages in thread
From: David Decotigny @ 2011-11-15 22:01 UTC (permalink / raw)
To: netdev, linux-kernel, Stephen Hemminger
Cc: David S. Miller, Ian Campbell, Eric Dumazet, Jeff Kirsher,
Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla, David Decotigny
Hi all,
I'm afraid this version (http://patchwork.ozlabs.org/patch/125861/) is wrong.
Each software stat field is updated by one single writer. But these
different stats are guarded by a single seqcount, so effectively
different writers are fiddling with the same seqcount. Question is: is
it Ok for the seqcount to be updated concurrently without protection?
Is the seqcount guaranteed to be correctly updated from the readers'
perspective? Or should I serialize the sections that update the
seqcount?
If I should protect it, then I need to revisit that patch again: I'd
prefer not to lock in the fast paths just because of the stats. I
could for example revert to v3 (using atomic_t stats). Would you have
any recommendation/suggestion?
Thanks! Regards,
--
David Decotigny
On Tue, Nov 15, 2011 at 11:25 AM, David Decotigny
<david.decotigny@google.com> wrote:
> This commit implements the ndo_get_stats64() API for forcedeth. Since
> hardware stats are being updated from different contexts (process and
> timer), this commit adds protection (locking + atomic variables). For
> software stats, it relies on the u64_stats_sync.h API.
>
> Tested:
> - 16-way SMP x86_64 ->
> RX bytes:7244556582 (7.2 GB) TX bytes:181904254 (181.9 MB)
> - pktgen + loopback: identical rx_bytes/tx_bytes and rx_packets/tx_packets
>
>
>
> Signed-off-by: David Decotigny <david.decotigny@google.com>
> ---
> drivers/net/ethernet/nvidia/forcedeth.c | 195 +++++++++++++++++++++++--------
> 1 files changed, 144 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index ee8cce5..ff01d5e 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -65,7 +65,8 @@
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> #include <linux/prefetch.h>
> -#include <linux/io.h>
> +#include <linux/u64_stats_sync.h>
> +#include <linux/io.h>
>
> #include <asm/irq.h>
> #include <asm/system.h>
> @@ -736,6 +737,18 @@ 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.
> + *
> + * Hardware stats updates are protected by hwstats_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
> + *
> + * Software stats are accessed only through a 64b synchronization
> + * point and are not subject to other synchronization techniques (one
> + * unique updating thread for each stat [single queue RX/TX fast
> + * paths], or callers already synchronized [for tx_dropped, except from
> + * nv_open/nv_close]).
> */
>
> /* in dev: base, irq */
> @@ -745,9 +758,13 @@ struct fe_priv {
> struct net_device *dev;
> struct napi_struct napi;
>
> - /* General data:
> - * Locking: spin_lock(&np->lock); */
> + /* hardware stats are updated in syscall and timer */
> + spinlock_t hwstats_lock;
> struct nv_ethtool_stats estats;
> +
> + /* software stats are accessed through a 64b synchronization point */
> + struct u64_stats_sync swstats_syncp;
> +
> int in_shutdown;
> u32 linkspeed;
> int duplex;
> @@ -798,6 +815,11 @@ struct fe_priv {
> u32 nic_poll_irq;
> int rx_ring_size;
>
> + /* RX software stats */
> + u64 stat_rx_packets;
> + u64 stat_rx_bytes; /* not always available in HW */
> + u64 stat_rx_missed_errors;
> +
> /* media detection workaround.
> * Locking: Within irq hander or disable_irq+spin_lock(&np->lock);
> */
> @@ -820,6 +842,11 @@ struct fe_priv {
> struct nv_skb_map *tx_end_flip;
> int tx_stop;
>
> + /* TX software stats */
> + u64 stat_tx_packets; /* not always available in HW */
> + u64 stat_tx_bytes;
> + u64 stat_tx_dropped;
> +
> /* msi/msi-x fields */
> u32 msi_flags;
> struct msix_entry msi_x_entry[NV_MSI_X_MAX_VECTORS];
> @@ -1635,11 +1662,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)->hwstats_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 hwstats_lock with
> + * spin_lock_irqsave() in calling functions. */
> + WARN_ONCE(in_irq(), "forcedeth: estats spin_lock(_bh) from top-half");
> + assert_spin_locked(&np->hwstats_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);
> @@ -1698,40 +1733,67 @@ static void nv_get_hw_stats(struct net_device *dev)
> }
>
> /*
> - * 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)->hwstats_lock)
> + __releases(&netdev_priv(dev)->hwstats_lock)
> {
> struct fe_priv *np = netdev_priv(dev);
> + unsigned int syncp_start;
> +
> + /*
> + * Note: because HW stats are not always available and for
> + * consistency reasons, the following ifconfig stats are
> + * managed by software: rx_bytes, tx_bytes, rx_packets and
> + * tx_packets. The related hardware stats reported by ethtool
> + * should be equivalent to these ifconfig stats, with 4
> + * additional bytes per packet (Ethernet FCS CRC).
> + */
> +
> + /* software stats */
> + do {
> + syncp_start = u64_stats_fetch_begin(&np->swstats_syncp);
> + storage->rx_packets = np->stat_rx_packets;
> + storage->tx_packets = np->stat_tx_packets;
> + storage->rx_bytes = np->stat_rx_bytes;
> + storage->tx_bytes = np->stat_tx_bytes;
> + storage->tx_dropped = np->stat_tx_dropped;
> + storage->rx_missed_errors = np->stat_rx_missed_errors;
> + } while (u64_stats_fetch_retry(&np->swstats_syncp, syncp_start));
>
> /* 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);
> + if (np->driver_data & DEV_HAS_STATISTICS_V123) {
> + spin_lock_bh(&np->hwstats_lock);
>
> - /*
> - * Note: because HW stats are not always available and
> - * for consistency reasons, the following ifconfig
> - * stats are managed by software: rx_bytes, tx_bytes,
> - * rx_packets and tx_packets. The related hardware
> - * stats reported by ethtool should be equivalent to
> - * these ifconfig stats, with 4 additional bytes per
> - * packet (Ethernet FCS CRC).
> - */
> + nv_update_stats(dev);
> +
> + /* generic stats */
> + storage->rx_errors = np->estats.rx_errors_total;
> + storage->tx_errors = np->estats.tx_errors_total;
> +
> + /* meaningful only when NIC supports stats v3 */
> + 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;
>
> - /* copy to net_device stats */
> - 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;
> + /* 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->hwstats_lock);
> }
>
> - return &dev->stats;
> + return storage;
> }
>
> /*
> @@ -1932,8 +1994,11 @@ static void nv_drain_tx(struct net_device *dev)
> np->tx_ring.ex[i].bufhigh = 0;
> np->tx_ring.ex[i].buflow = 0;
> }
> - if (nv_release_txskb(np, &np->tx_skb[i]))
> - dev->stats.tx_dropped++;
> + if (nv_release_txskb(np, &np->tx_skb[i])) {
> + u64_stats_update_begin(&np->swstats_syncp);
> + np->stat_tx_dropped++;
> + u64_stats_update_end(&np->swstats_syncp);
> + }
> np->tx_skb[i].dma = 0;
> np->tx_skb[i].dma_len = 0;
> np->tx_skb[i].dma_single = 0;
> @@ -2390,11 +2455,14 @@ static int nv_tx_done(struct net_device *dev, int limit)
> if (np->desc_ver == DESC_VER_1) {
> if (flags & NV_TX_LASTPACKET) {
> if (flags & NV_TX_ERROR) {
> - if ((flags & NV_TX_RETRYERROR) && !(flags & NV_TX_RETRYCOUNT_MASK))
> + if ((flags & NV_TX_RETRYERROR)
> + && !(flags & NV_TX_RETRYCOUNT_MASK))
> nv_legacybackoff_reseed(dev);
> } else {
> - dev->stats.tx_packets++;
> - dev->stats.tx_bytes += np->get_tx_ctx->skb->len;
> + u64_stats_update_begin(&np->swstats_syncp);
> + np->stat_tx_packets++;
> + np->stat_tx_bytes += np->get_tx_ctx->skb->len;
> + u64_stats_update_end(&np->swstats_syncp);
> }
> dev_kfree_skb_any(np->get_tx_ctx->skb);
> np->get_tx_ctx->skb = NULL;
> @@ -2403,11 +2471,14 @@ static int nv_tx_done(struct net_device *dev, int limit)
> } else {
> if (flags & NV_TX2_LASTPACKET) {
> if (flags & NV_TX2_ERROR) {
> - if ((flags & NV_TX2_RETRYERROR) && !(flags & NV_TX2_RETRYCOUNT_MASK))
> + if ((flags & NV_TX2_RETRYERROR)
> + && !(flags & NV_TX2_RETRYCOUNT_MASK))
> nv_legacybackoff_reseed(dev);
> } else {
> - dev->stats.tx_packets++;
> - dev->stats.tx_bytes += np->get_tx_ctx->skb->len;
> + u64_stats_update_begin(&np->swstats_syncp);
> + np->stat_tx_packets++;
> + np->stat_tx_bytes += np->get_tx_ctx->skb->len;
> + u64_stats_update_end(&np->swstats_syncp);
> }
> dev_kfree_skb_any(np->get_tx_ctx->skb);
> np->get_tx_ctx->skb = NULL;
> @@ -2441,15 +2512,18 @@ static int nv_tx_done_optimized(struct net_device *dev, int limit)
>
> if (flags & NV_TX2_LASTPACKET) {
> if (flags & NV_TX2_ERROR) {
> - if ((flags & NV_TX2_RETRYERROR) && !(flags & NV_TX2_RETRYCOUNT_MASK)) {
> + if ((flags & NV_TX2_RETRYERROR)
> + && !(flags & NV_TX2_RETRYCOUNT_MASK)) {
> if (np->driver_data & DEV_HAS_GEAR_MODE)
> nv_gear_backoff_reseed(dev);
> else
> nv_legacybackoff_reseed(dev);
> }
> } else {
> - dev->stats.tx_packets++;
> - dev->stats.tx_bytes += np->get_tx_ctx->skb->len;
> + u64_stats_update_begin(&np->swstats_syncp);
> + np->stat_tx_packets++;
> + np->stat_tx_bytes += np->get_tx_ctx->skb->len;
> + u64_stats_update_end(&np->swstats_syncp);
> }
>
> dev_kfree_skb_any(np->get_tx_ctx->skb);
> @@ -2662,8 +2736,11 @@ 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++;
> + if (flags & NV_RX_MISSEDFRAME) {
> + u64_stats_update_begin(&np->swstats_syncp);
> + np->stat_rx_missed_errors++;
> + u64_stats_update_end(&np->swstats_syncp);
> + }
> dev_kfree_skb(skb);
> goto next_pkt;
> }
> @@ -2706,8 +2783,10 @@ 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++;
> - dev->stats.rx_bytes += len;
> + u64_stats_update_begin(&np->swstats_syncp);
> + np->stat_rx_packets++;
> + np->stat_rx_bytes += len;
> + u64_stats_update_end(&np->swstats_syncp);
> next_pkt:
> if (unlikely(np->get_rx.orig++ == np->last_rx.orig))
> np->get_rx.orig = np->first_rx.orig;
> @@ -2790,8 +2869,10 @@ 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++;
> - dev->stats.rx_bytes += len;
> + u64_stats_update_begin(&np->swstats_syncp);
> + np->stat_rx_packets++;
> + np->stat_rx_bytes += len;
> + u64_stats_update_end(&np->swstats_syncp);
> } else {
> dev_kfree_skb(skb);
> }
> @@ -4000,11 +4081,18 @@ static void nv_poll_controller(struct net_device *dev)
> #endif
>
> static void nv_do_stats_poll(unsigned long data)
> + __acquires(&netdev_priv(dev)->hwstats_lock)
> + __releases(&netdev_priv(dev)->hwstats_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->hwstats_lock)) {
> + nv_update_stats(dev);
> + spin_unlock(&np->hwstats_lock);
> + }
>
> if (!np->in_shutdown)
> mod_timer(&np->stats_poll,
> @@ -4711,14 +4799,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)->hwstats_lock)
> + __releases(&netdev_priv(dev)->hwstats_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->hwstats_lock);
> + nv_update_stats(dev);
> + memcpy(buffer, &np->estats,
> + nv_get_sset_count(dev, ETH_SS_STATS)*sizeof(u64));
> + spin_unlock_bh(&np->hwstats_lock);
> }
>
> static int nv_link_test(struct net_device *dev)
> @@ -5362,7 +5454,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,
> @@ -5379,7 +5471,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,
> @@ -5418,6 +5510,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->hwstats_lock);
> SET_NETDEV_DEV(dev, &pci_dev->dev);
>
> init_timer(&np->oom_kick);
> --
> 1.7.3.1
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v4 6/8] forcedeth: account for dropped RX frames
2011-11-15 19:25 [PATCH net-next v4 0/8] forcedeth: stats & debug enhancements David Decotigny
` (4 preceding siblings ...)
2011-11-15 19:25 ` [PATCH net-next v4 5/8] forcedeth: implement ndo_get_stats64() API David Decotigny
@ 2011-11-15 19:25 ` David Decotigny
2011-11-15 22:21 ` Stephen Hemminger
2011-11-15 19:25 ` [PATCH net-next v4 7/8] forcedeth: new ethtool stat counter for TX timeouts David Decotigny
2011-11-15 19:25 ` [PATCH net-next v4 8/8] forcedeth: stats updated with a deferrable timer David Decotigny
7 siblings, 1 reply; 17+ messages in thread
From: David Decotigny @ 2011-11-15 19:25 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: David S. Miller, Ian Campbell, Eric Dumazet, Jeff Kirsher,
Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla, David Decotigny
This adds the stats counter for dropped RX frames.
Signed-off-by: David Decotigny <david.decotigny@google.com>
---
drivers/net/ethernet/nvidia/forcedeth.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index ff01d5e..a50c839 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -819,6 +819,7 @@ struct fe_priv {
u64 stat_rx_packets;
u64 stat_rx_bytes; /* not always available in HW */
u64 stat_rx_missed_errors;
+ u64 stat_rx_dropped;
/* media detection workaround.
* Locking: Within irq hander or disable_irq+spin_lock(&np->lock);
@@ -1762,6 +1763,7 @@ nv_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *storage)
storage->tx_packets = np->stat_tx_packets;
storage->rx_bytes = np->stat_rx_bytes;
storage->tx_bytes = np->stat_tx_bytes;
+ storage->rx_dropped = np->stat_rx_dropped;
storage->tx_dropped = np->stat_tx_dropped;
storage->rx_missed_errors = np->stat_rx_missed_errors;
} while (u64_stats_fetch_retry(&np->swstats_syncp, syncp_start));
@@ -1826,8 +1828,12 @@ 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 {
+ u64_stats_update_begin(&np->swstats_syncp);
+ np->stat_rx_dropped++;
+ u64_stats_update_end(&np->swstats_syncp);
return 1;
+ }
}
return 0;
}
@@ -1858,8 +1864,12 @@ 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 {
+ u64_stats_update_begin(&np->swstats_syncp);
+ np->stat_rx_dropped++;
+ u64_stats_update_end(&np->swstats_syncp);
return 1;
+ }
}
return 0;
}
--
1.7.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH net-next v4 6/8] forcedeth: account for dropped RX frames
2011-11-15 19:25 ` [PATCH net-next v4 6/8] forcedeth: account for dropped RX frames David Decotigny
@ 2011-11-15 22:21 ` Stephen Hemminger
2011-11-15 22:35 ` David Decotigny
0 siblings, 1 reply; 17+ messages in thread
From: Stephen Hemminger @ 2011-11-15 22:21 UTC (permalink / raw)
To: David Decotigny
Cc: netdev, linux-kernel, David S. Miller, Ian Campbell, Eric Dumazet,
Jeff Kirsher, Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla
On Tue, 15 Nov 2011 11:25:39 -0800
David Decotigny <david.decotigny@google.com> wrote:
> This adds the stats counter for dropped RX frames.
>
There is already an rx_dropped statistic in netdevice structure,
why do you need your own in ethtool?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH net-next v4 6/8] forcedeth: account for dropped RX frames
2011-11-15 22:21 ` Stephen Hemminger
@ 2011-11-15 22:35 ` David Decotigny
0 siblings, 0 replies; 17+ messages in thread
From: David Decotigny @ 2011-11-15 22:35 UTC (permalink / raw)
To: Stephen Hemminger
Cc: netdev, linux-kernel, David S. Miller, Ian Campbell, Eric Dumazet,
Jeff Kirsher, Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla
Hello,
On Tue, Nov 15, 2011 at 2:21 PM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> There is already an rx_dropped statistic in netdevice structure,
> why do you need your own in ethtool?
Sorry, I think my commit message was (once again) inaccurate. This
commit doesn't really "add" the stat, instead it adds code to update
the standard stats (struct rtnl_link_stats64).
Regards,
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH net-next v4 7/8] forcedeth: new ethtool stat counter for TX timeouts
2011-11-15 19:25 [PATCH net-next v4 0/8] forcedeth: stats & debug enhancements David Decotigny
` (5 preceding siblings ...)
2011-11-15 19:25 ` [PATCH net-next v4 6/8] forcedeth: account for dropped RX frames David Decotigny
@ 2011-11-15 19:25 ` David Decotigny
2011-11-15 19:25 ` [PATCH net-next v4 8/8] forcedeth: stats updated with a deferrable timer David Decotigny
7 siblings, 0 replies; 17+ messages in thread
From: David Decotigny @ 2011-11-15 19:25 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: David S. Miller, Ian Campbell, Eric Dumazet, Jeff Kirsher,
Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla, David Decotigny
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 <david.decotigny@google.com>
---
drivers/net/ethernet/nvidia/forcedeth.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index a50c839..7284f40 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -634,6 +634,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" },
@@ -674,6 +675,7 @@ struct nv_ethtool_stats {
u64 rx_packets; /* should be ifconfig->rx_packets */
u64 rx_errors_total;
u64 tx_errors_total;
+ u64 tx_timeout;
/* version 2 stats */
u64 tx_deferral;
@@ -848,6 +850,9 @@ struct fe_priv {
u64 stat_tx_bytes;
u64 stat_tx_dropped;
+ /* TX software stats exported by ethtool */
+ atomic_t stat_tx_timeout; /* TX timeouts since last nv_update_stats */
+
/* msi/msi-x fields */
u32 msi_flags;
struct msix_entry msi_x_entry[NV_MSI_X_MAX_VECTORS];
@@ -1731,6 +1736,8 @@ static void nv_update_stats(struct net_device *dev)
np->estats.tx_multicast += readl(base + NvRegTxMulticast);
np->estats.tx_broadcast += readl(base + NvRegTxBroadcast);
}
+
+ np->estats.tx_timeout += atomic_xchg(&np->stat_tx_timeout, 0);
}
/*
@@ -2627,6 +2634,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 */
--
1.7.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH net-next v4 8/8] forcedeth: stats updated with a deferrable timer
2011-11-15 19:25 [PATCH net-next v4 0/8] forcedeth: stats & debug enhancements David Decotigny
` (6 preceding siblings ...)
2011-11-15 19:25 ` [PATCH net-next v4 7/8] forcedeth: new ethtool stat counter for TX timeouts David Decotigny
@ 2011-11-15 19:25 ` David Decotigny
7 siblings, 0 replies; 17+ messages in thread
From: David Decotigny @ 2011-11-15 19:25 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: David S. Miller, Ian Campbell, Eric Dumazet, Jeff Kirsher,
Ben Hutchings, Jiri Pirko, Joe Perches, Szymon Janc,
Richard Jones, Ayaz Abdulla, David Decotigny
Mark stats timer as deferrable: punctuality in waking the stats timer
callback doesn't matter much, as it is responsible only to avoid
integer wraparound.
We need at least 1 other timer to fire within 17s (fully loaded 1Gbps)
to avoid wrap-arounds. Desired period is still 10s.
Signed-off-by: David Decotigny <david.decotigny@google.com>
---
drivers/net/ethernet/nvidia/forcedeth.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 7284f40..b5c85de 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -5538,7 +5538,7 @@ static int __devinit nv_probe(struct pci_dev *pci_dev, const struct pci_device_i
init_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 */
--
1.7.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread