login
Search
Traffic stats counters (rx_bytes and tx_bytes) in net_device are
"unsigned long". On 32-bit systems, they wrap around every few
minutes, giving out wrong answers to the amount of traffic. To get the
right message, another available approach is "ethtool -S". However,
r8169 didn't support those two counters so far.
Add traffic counters tx_bytes and rx_bytes with 64-bit width for
ethtool. On 32-bit systems, gcc treats each one as two 32-bit
variables, making the increment not "atomic". But there is no sync
issue since the updates to the counters are serialized by driver logic
in any case. Results provided by ethtool maybe slightly biased if the
read and update operations are interleaved. But the results are much
better than the original ones that always fall into the range from 0
to 4GiB.
Signed-off-by: Junchang Wang <junchangwang@gmail.com>
---
drivers/net/r8169.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 217e709..19a2748 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -510,6 +510,8 @@ struct rtl8169_private {
struct mii_if_info mii;
struct rtl8169_counters counters;
+ u64 tx_bytes;
+ u64 rx_bytes;
u32 saved_wolopts;
};
@@ -1162,6 +1164,8 @@ static void rtl8169_set_msglevel(struct
net_device *dev, u32 value)
static const char rtl8169_gstrings[][ETH_GSTRING_LEN] = {
"tx_packets",
"rx_packets",
+ "tx_bytes",
+ "rx_bytes",
"tx_errors",
"rx_errors",
"rx_missed",
@@ -1236,17 +1240,19 @@ static void rtl8169_get_ethtool_stats(struct
net_device *dev,
data[0] = le64_to_cpu(tp->counters.tx_packets);
data[1] = le64_to_cpu(tp->counters.rx_packets);
- data[2] = le64_to_cpu(tp->counters.tx_errors);
- data[3] = le32_to_cpu(tp->counters.rx_errors);
- data[4] = le16_to_cpu(tp->counters.rx_missed);
- data[5] = le16_to_cpu(tp->counters.align_errors);
- data[6] = le32_to_cpu(tp->counters.tx_one_collision);
- data[7] = ...ethtool is - imvho - biased toward hardware, not toward software emulation. If the packets are short enough, replace "_bytes" by "_packets", "_minutes" by "_hours" or "_every_day" and the same kind of problem appear. You can fix the application at zero cost in the kernel: poll < 34 s and update the application counters with the kernel counters increment. -- Ueimor --
r8169 has provided 64-bit hardware counters for #packets, #error_packets, etc. They works fine even on 32-bit systems. What we Thanks for you advice. -- --Junchang --
From: Junchang Wang <junchangwang@gmail.com> I absolutely do not want to see drivers start doing this, so right off the bat I am not going to apply this patch. If the problem is that people want 64-bit counters available for core statistics on 32-bit systems, we do not fix that problem by hacking every single driver to provide them side-band via ethtool. First of all, we now have "struct rtnl_link_stats64" in linux/if_link.h, it's there to start combating this problem generically, for every device, rather than the way you are trying handle it only for one specific driver at a time. So that's the area where you should start looking to solve these kinds of problem. --
Hi David, Most NICs have provided those two 64-bit counters in hardware. They work fine even in 32-bit systems and don't need new 64-bit counters any more. Frankly, r8169 is the first Gbps NIC I have seen that does not support those two counters. So I thought changing upper layer is Thanks for your advice. I'll go deep into it and see how we can solve this problem. -- --Junchang --
On Tue, 2010-05-25 at 16:15 -0700, David Miller wrote:
My understand of the current situation is as follows; correct me if any
of this is wrong:
The standard counters in struct net_device_stats have type unsigned long
which is the native word size and so can be read and updated
automatically. Net drivers can update counters from the data path
without any interlocking with their ndo_get_stats implementation or the
networking core code which reads them.
The values returned by ndo_get_stats (by reference) are exposed:
- Through procfs (/proc/net/dev) as columns of numbers
- Through sysfs (/sys/class/net/*/stats/*) as single numeric strings
- Through netlink (IFLA_STATS and IFLA_STATS64) as 32-bit or 64-bit
values in binary structures
Changing the counter types to u64 for 32-bit architectures would remove
atomicity and expose half-updated counters to userland. Changing the
driver interface significantly so that atomicity is not needed would
require changes to hundreds of drivers.
Assuming the above is all correct, I think we can only solve this with a
gradual change (as for net_device_ops). The following might work:
1. a. Define struct net_device_stats64 identically to rtnl_link_stats64.
b. Add net_device_ops::ndo_get_stats64, the implementation of which
will return a pointer to such a structure. The referenced
structure must only be updated atomically, except within the
call to ndo_get_stats64.
(For 64-bit architectures, these could be macro aliases to the
existing structure and operations.)
c. On 32-bit architectures, insert unsigned long padding after each
member of struct net_device_stats.
d. Add an anonymous union in net_device; move stats into the union
and add struct net_device_stats64 stats64.
e. Change dev_get_stats() to return a pointer to struct
net_device_stats64, and to call ndo_get_stats64 if available in
preference to ndo_get_stats. Adjust callers accordingly.
f. Either change ...On Wed, 02 Jun 2010 22:34:29 +0100 Another big issue is maintaining ABI compatibility for /proc and ioctl interfaces. So bigger values would only be available through netlink, and most applications using counters don't use netlink. -- --
Doesn't the /proc interface already allow 64 bit values in case of 64 bit hosts running the same 32 bit user space? For the ioctl interfaces, we can of course introduce additional ioctl commands that always deal with 64 bit values, many other subsystems have been extended in similar ways. We don't mandate that a program built against new kernel headers must work on old kernels, so it is possibly to actually change the definitions, as long as the kernel still supports the old interfaces for existing binaries. We should still have a good reason even for breaking the ABI in ways that we don't guarantee to be stable. Arnd --
Yes. And the most widely used consumer of /proc/net/dev, ifconfig, [...] Are there any ioctl interfaces to net_device_stats? I didn't spot any. Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. --
Use struct rtnl_link_stats64 as the statistics structure. On 32-bit architectures, insert 32 bits of padding after/before each field of struct net_device_stats to make its layout compatible with struct rtnl_link_stats64. Add an anonymous union in net_device; move stats into the union and add struct rtnl_link_stats64 stats64. Add net_device_ops::ndo_get_stats64, implementations of which will return a pointer to struct rtnl_link_stats64. Drivers that implement this operation must not update the structure asynchronously. Change dev_get_stats() to call this operation if available, and to return a pointer to struct rtnl_link_stats64. Define accessor macros for reading rtnl_link_stats64 in a way that allows for asynchronous atomic updates by drivers that do not implement ndo_get_stats64. Change callers of dev_get_stats() to use these macros when reading the returned structure. Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> --- This implements step 1 of the transition I previously proposed, with the following modifications: - Use struct rtnl_link_stats64 directly instead of defining an identical struct net_device_stats64. - Do not allow asynchronous writes to struct rtnl_link_stats64. This is not possible to do atomically on all architectures, so there is little value in allowing it. - Assume that the padding in struct net_device_stats is cleared by drivers if they do not use net_device::stats. Drivers must already clear the fields they don't use, such as tx_compressed. Ben. include/linux/if_link.h | 3 +- include/linux/netdevice.h | 112 +++++++++++++++++++++++++++------------- net/8021q/vlanproc.c | 18 ++++-- net/core/dev.c | 60 +++++++++++++--------- net/core/net-sysfs.c | 13 +++-- net/core/rtnetlink.c | 126 ++++++++++++++++++++++----------------------- 6 files changed, 194 insertions(+), 138 deletions(-) diff --git a/include/linux/if_link.h b/include/linux/if_link.h index 85c812d..7fcad2e ...
On Thu, 03 Jun 2010 18:39:04 +0100 Macros... with multiple casts. Gack -- --
RTNL_LINK_STATS64_READ_OFFSET() is only really needed in net-sysfs.c,
and that ugliness could maybe be left there. So maybe the accessors
could be defined as something like:
#if BITS_PER_LONG == 64
static inline u64 rtnl_link_stats64_read(const u64 *field)
{
return ACCESS_ONCE(*field);
}
static inline u32 rtnl_link_stats64_read32(const u64 *field)
{
return ACCESS_ONCE(*field);
}
#else
static inline u64 rtnl_link_stats64_read(const u64 *field)
{
#if defined(__LITTLE_ENDIAN)
const u32 *low = (const u32 *)field, *high = low + 1;
#else
const u32 *high = (const u32 *)field, *low = high + 1;
#endif
return ACCESS_ONCE(*low) | (u64)*high << 32;
}
static inline u32 rtnl_link_stats64_read32(const u64 *field)
{
#if defined(__LITTLE_ENDIAN)
const u32 *low = (const u32 *)field;
#else
const u32 *low = (const u32 *)field + 1;
#endif
return ACCESS_ONCE(*low);
}
#endif
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
On Thu, 03 Jun 2010 20:11:38 +0100 Do we really care if compiler reorders access. I think not. There was no order guarantee in the past. -- --
Since these reads are potentially racing with writes, we want to ensure that they are atomic. Without the volatile-qualification, the compiler can legitimately split or repeat the reads, though I don't see any particular reason why this is a likely optimisation. Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. --
On Fri, 04 Jun 2010 19:15:18 +0100 But this part of the code is only being run on on 64 bit machines. Updates of basic types for the CPU are atomic, lots of other code already assumes this. Take off your tin hat, this is excessive paranoia. -- --
I like my tin hat, I'm sure I saw more oopses before I put it on. ;-) Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. --
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
This begins step 2 of the transition. It's a trivial change since sfc
already maintains 64-bit statistics and does not update
net_device::stats asynchronously.
Ben.
drivers/net/sfc/efx.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 26b0cc2..8ad476a 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -1492,11 +1492,11 @@ static int efx_net_stop(struct net_device *net_dev)
}
/* Context: process, dev_base_lock or RTNL held, non-blocking. */
-static struct net_device_stats *efx_net_stats(struct net_device *net_dev)
+static struct rtnl_link_stats64 *efx_net_stats(struct net_device *net_dev)
{
struct efx_nic *efx = netdev_priv(net_dev);
struct efx_mac_stats *mac_stats = &efx->mac_stats;
- struct net_device_stats *stats = &net_dev->stats;
+ struct rtnl_link_stats64 *stats = &net_dev->stats64;
spin_lock_bh(&efx->stats_lock);
efx->type->update_stats(efx);
@@ -1630,7 +1630,7 @@ static void efx_set_multicast_list(struct net_device *net_dev)
static const struct net_device_ops efx_netdev_ops = {
.ndo_open = efx_net_open,
.ndo_stop = efx_net_stop,
- .ndo_get_stats = efx_net_stats,
+ .ndo_get_stats64 = efx_net_stats,
.ndo_tx_timeout = efx_watchdog,
.ndo_start_xmit = efx_hard_start_xmit,
.ndo_validate_addr = eth_validate_addr,
--
1.6.2.5
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
Hi Ben, I realized the network team doesn't care about 64-bit counters (especially rx_*) on 32-bit systems. A similar disscussion can be found here: http://www.gossamer-threads.com/lists/linux/kernel/282631?search_string=64%20stats;#28... And Eric just gave a explanation why they stand by that point. Updating rx_* counters in core network will dirty a cache line. --Junchang --
Well, the outlined discussion is 8 years old. Some things changed so we
probably have more possibilities today. per_cpu infrastructure is pretty
cool now for example.
If stats are updated only by the NIC, we can have 64 bit stats with
nearly 0 cost.
Real problem is some stats are updated by software.
Upgrading them to 64 bits would need atomic64 to coordinate writers and
readers, or making sure reader use appropriate locks to serialize with
Sorry, I realize I was a bit wrong in my last mail.
In txq_trans_update(txq) we only update the time (in jiffies) of last
transmission, using a txq field instead of dev->trans_start)
txq->tx_bytes, txq->tx_packets, txq->tx_dropped _might_ be used by
_drivers_ to update tx counters, instead of dev->stats{tx_bytes,
tx_packets, tx_dropped}, especially by multiqueue drivers.
txq->tx... updates are better because they are multiqueue compatable (no
need to use atomic ops), and they use a cache line already dirtied
because we hold txq lock at transmission time (unless for LLTX drivers)
(For an example, check drivers/net/macvlan.c, macvlan_start_xmit() and
commit 2c11455321f3)
--
