[PATCH] r8169: Add counters tx_bytes and rx_bytes for ethtool

[PATCH] r8169: Add counters tx_bytes and rx_bytes for ethtool

[Junchang Wang]

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] = le32_to_cpu(tp->counters.tx_multi_collision);
-	data[8] = le64_to_cpu(tp->counters.rx_unicast);
-	data[9] = le64_to_cpu(tp->counters.rx_broadcast);
-	data[10] = le32_to_cpu(tp->counters.rx_multicast);
-	data[11] = le16_to_cpu(tp->counters.tx_aborted);
-	data[12] = le16_to_cpu(tp->counters.tx_underun);
+	data[2] = tp->tx_bytes;
+	data[3] = tp->rx_bytes;
+	data[4] = le64_to_cpu(tp->counters.tx_errors);
+	data[5] = le32_to_cpu(tp->counters.rx_errors);
+	data[6] = le16_to_cpu(tp->counters.rx_missed);
+	data[7] = le16_to_cpu(tp->counters.align_errors);
+	data[8] = le32_to_cpu(tp->counters.tx_one_collision);
+	data[9] = le32_to_cpu(tp->counters.tx_multi_collision);
+	data[10] = le64_to_cpu(tp->counters.rx_unicast);
+	data[11] = le64_to_cpu(tp->counters.rx_broadcast);
+	data[12] = le32_to_cpu(tp->counters.rx_multicast);
+	data[13] = le16_to_cpu(tp->counters.tx_aborted);
+	data[14] = le16_to_cpu(tp->counters.tx_underun);
 }

 static void rtl8169_get_strings(struct net_device *dev, u32
stringset, u8 *data)
@@ -4412,6 +4418,7 @@ static void rtl8169_tx_interrupt(struct net_device *dev,

 		dev->stats.tx_bytes += len;
 		dev->stats.tx_packets++;
+		tp->tx_bytes += len;

 		rtl8169_unmap_tx_skb(tp->pci_dev, tx_skb, tp->TxDescArray + entry);

@@ -4567,6 +4574,7 @@ static int rtl8169_rx_interrupt(struct net_device *dev,

 			dev->stats.rx_bytes += pkt_size;
 			dev->stats.rx_packets++;
+			tp->rx_bytes += pkt_size;
 		}

 		/* Work around for AMD plateform. */

--

Re: [PATCH] r8169: Add counters tx_bytes and rx_bytes for ethtool

[Francois Romieu]

Junchang Wang <junchangwang@gmail.com> :
> 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.

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

Re: [PATCH] r8169: Add counters tx_bytes and rx_bytes for ethtool

[Junchang Wang]

Hi Francois,

>
> If the packets are short enough, replace "_bytes" by "_packets", "_minutes"
> by "_hours" or "_every_day" and the same kind of problem appear.

r8169 has provided 64-bit hardware counters for #packets,
#error_packets, etc. They works fine even on 32-bit systems. What we
really need is just counter rx_bytes and tx_bytes.

>
> You can fix the application at zero cost in the kernel: poll < 34 s and
> update the application counters with the kernel counters increment.

Thanks for you advice.


-- 
--Junchang

Re: [PATCH] r8169: Add counters tx_bytes and rx_bytes for ethtool

[David Miller]

From: Junchang Wang <junchangwang@gmail.com>
Date: Tue, 25 May 2010 22:19:46 +0800

> 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>

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.

Re: [PATCH] r8169: Add counters tx_bytes and rx_bytes for ethtool

[Junchang Wang]

Hi David,
Thanks for your advice.

>
> 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.

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
immoderate and tried to provide a cheap but valuable way.

>
> 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.

Thanks for your advice. I'll go deep into it and see how we can solve
this problem.


-- 
--Junchang

64-bit net_device_stats

[Ben Hutchings]

On Tue, 2010-05-25 at 16:15 -0700, David Miller wrote:
[...]
> 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.

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 dev_get_stats() to clear the padding members, or
      change drivers to ensure that the padding is cleared.

2. Change drivers to define ndo_get_stats64, following the rule
   stated in 1b.

3. Much later, remove net_device_stats.

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.

Re: 64-bit net_device_stats

[Stephen Hemminger]

On Wed, 02 Jun 2010 22:34:29 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> 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.

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. 

-- 

Re: 64-bit net_device_stats

[Arnd Bergmann]

On Wednesday 02 June 2010 23:59:12 Stephen Hemminger wrote:
> On Wed, 02 Jun 2010 22:34:29 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > 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.
> 
> 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

Re: 64-bit net_device_stats

[Ben Hutchings]

On Thu, 2010-06-03 at 01:38 +0200, Arnd Bergmann wrote:
> On Wednesday 02 June 2010 23:59:12 Stephen Hemminger wrote:
> > On Wed, 02 Jun 2010 22:34:29 +0100
> > Ben Hutchings <bhutchings@solarflare.com> wrote:
> > 
> > > 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.
> > 
> > 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?

Yes.  And the most widely used consumer of /proc/net/dev, ifconfig,
already parses them as 64-bit values.

> 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.
[...]

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.

[PATCH 1/2] net: Enable 64-bit net device statistics on 32-bit architectures

[Ben Hutchings]

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 100644
--- a/include/linux/if_link.h
+++ b/include/linux/if_link.h
@@ -4,7 +4,7 @@
 #include <linux/types.h>
 #include <linux/netlink.h>
 
-/* The struct should be in sync with struct net_device_stats */
+/* This struct should be in sync with struct rtnl_link_stats64 */
 struct rtnl_link_stats {
 	__u32	rx_packets;		/* total packets received	*/
 	__u32	tx_packets;		/* total packets transmitted	*/
@@ -37,6 +37,7 @@ struct rtnl_link_stats {
 	__u32	tx_compressed;
 };
 
+/* The main device statistics structure */
 struct rtnl_link_stats64 {
 	__u64	rx_packets;		/* total packets received	*/
 	__u64	tx_packets;		/* total packets transmitted	*/
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a249161..3f2b49d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -159,45 +159,70 @@ static inline bool dev_xmit_complete(int rc)
 #define MAX_HEADER (LL_MAX_HEADER + 48)
 #endif
 
-#endif  /*  __KERNEL__  */
-
 /*
- *	Network device statistics. Akin to the 2.0 ether stats but
- *	with byte counters.
+ *	Old network device statistics. Fields are native words
+ *	(unsigned long) so they can be read and written atomically.
+ *	Each field is padded to 64 bits for compatibility with
+ *	rtnl_link_stats64.
  */
 
+#if BITS_PER_LONG == 64
+#define NET_DEVICE_STATS_DEFINE(name)	u64 name
+#define RTNL_LINK_STATS64_READ(stats, name)				\
+	ACCESS_ONCE((stats)->name)
+#define RTNL_LINK_STATS64_READ_OFFSET(stats, offset)			\
+	ACCESS_ONCE((const u64 *)((const u8 *)(stats) + (offset)))
+#define RTNL_LINK_STATS64_READ32(stats, name)				\
+	((u32)ACCESS_ONCE((stats)->name))
+#else
+#if defined(__LITTLE_ENDIAN)
+#define NET_DEVICE_STATS_DEFINE(name)	u32 name, pad_ ## name
+#define RTNL_LINK_STATS64_READ_OFFSET(stats, offset)			\
+	(ACCESS_ONCE(*(const u32 *)((const u8 *)(stats) + (offset))) |	\
+	 (u64)(*(const u32 *)((const u8 *)(stats) + (offset) + 4)) << 32)
+#define RTNL_LINK_STATS64_READ32(stats, name)				\
+	(((const volatile u32 *)&(stats)->name)[0])
+#else
+#define NET_DEVICE_STATS_DEFINE(name)	u32 pad_ ## name, name
+#define RTNL_LINK_STATS64_READ_OFFSET(stats, offset)			\
+	((u64)(*(const u32 *)((const u8 *)(stats) + (offset))) << 32 |	\
+	 ACCESS_ONCE(*(const u32 *)((const u8 *)(stats) + (offset) + 4)))
+#define RTNL_LINK_STATS64_READ32(stats, name)				\
+	(((const volatile u32 *)&(stats)->name)[1])
+#endif
+#define RTNL_LINK_STATS64_READ(stats, name)				\
+	RTNL_LINK_STATS64_READ_OFFSET(					\
+		stats, offsetof(struct rtnl_link_stats64, name))
+#endif
+
 struct net_device_stats {
-	unsigned long	rx_packets;		/* total packets received	*/
-	unsigned long	tx_packets;		/* total packets transmitted	*/
-	unsigned long	rx_bytes;		/* total bytes received 	*/
-	unsigned long	tx_bytes;		/* total bytes transmitted	*/
-	unsigned long	rx_errors;		/* bad packets received		*/
-	unsigned long	tx_errors;		/* packet transmit problems	*/
-	unsigned long	rx_dropped;		/* no space in linux buffers	*/
-	unsigned long	tx_dropped;		/* no space available in linux	*/
-	unsigned long	multicast;		/* multicast packets received	*/
-	unsigned long	collisions;
-
-	/* detailed rx_errors: */
-	unsigned long	rx_length_errors;
-	unsigned long	rx_over_errors;		/* receiver ring buff overflow	*/
-	unsigned long	rx_crc_errors;		/* recved pkt with crc error	*/
-	unsigned long	rx_frame_errors;	/* recv'd frame alignment error */
-	unsigned long	rx_fifo_errors;		/* recv'r fifo overrun		*/
-	unsigned long	rx_missed_errors;	/* receiver missed packet	*/
-
-	/* detailed tx_errors */
-	unsigned long	tx_aborted_errors;
-	unsigned long	tx_carrier_errors;
-	unsigned long	tx_fifo_errors;
-	unsigned long	tx_heartbeat_errors;
-	unsigned long	tx_window_errors;
-	
-	/* for cslip etc */
-	unsigned long	rx_compressed;
-	unsigned long	tx_compressed;
+	NET_DEVICE_STATS_DEFINE(rx_packets);
+	NET_DEVICE_STATS_DEFINE(tx_packets);
+	NET_DEVICE_STATS_DEFINE(rx_bytes);
+	NET_DEVICE_STATS_DEFINE(tx_bytes);
+	NET_DEVICE_STATS_DEFINE(rx_errors);
+	NET_DEVICE_STATS_DEFINE(tx_errors);
+	NET_DEVICE_STATS_DEFINE(rx_dropped);
+	NET_DEVICE_STATS_DEFINE(tx_dropped);
+	NET_DEVICE_STATS_DEFINE(multicast);
+	NET_DEVICE_STATS_DEFINE(collisions);
+	NET_DEVICE_STATS_DEFINE(rx_length_errors);
+	NET_DEVICE_STATS_DEFINE(rx_over_errors);
+	NET_DEVICE_STATS_DEFINE(rx_crc_errors);
+	NET_DEVICE_STATS_DEFINE(rx_frame_errors);
+	NET_DEVICE_STATS_DEFINE(rx_fifo_errors);
+	NET_DEVICE_STATS_DEFINE(rx_missed_errors);
+	NET_DEVICE_STATS_DEFINE(tx_aborted_errors);
+	NET_DEVICE_STATS_DEFINE(tx_carrier_errors);
+	NET_DEVICE_STATS_DEFINE(tx_fifo_errors);
+	NET_DEVICE_STATS_DEFINE(tx_heartbeat_errors);
+	NET_DEVICE_STATS_DEFINE(tx_window_errors);
+	NET_DEVICE_STATS_DEFINE(rx_compressed);
+	NET_DEVICE_STATS_DEFINE(tx_compressed);
 };
 
+#endif  /*  __KERNEL__  */
+
 
 /* Media selection options. */
 enum {
@@ -660,10 +685,19 @@ struct netdev_rx_queue {
  *	Callback uses when the transmitter has not made any progress
  *	for dev->watchdog ticks.
  *
+ * struct rtnl_link_stats64* (*ndo_get_stats64)(struct net_device *dev);
  * struct net_device_stats* (*ndo_get_stats)(struct net_device *dev);
  *	Called when a user wants to get the network device usage
- *	statistics. If not defined, the counters in dev->stats will
- *	be used.
+ *	statistics. Drivers must do one of the following:
+ *	1. Define @ndo_get_stats64 to update a rtnl_link_stats64 structure
+ *	   (which should normally be dev->stats64) and return a ponter to
+ *	   it. The structure must not be changed asynchronously.
+ *	2. Define @ndo_get_stats to update a net_device_stats64 structure
+ *	   (which should normally be dev->stats) and return a pointer to
+ *	   it. The structure may be changed asynchronously only if each
+ *	   field is written atomically.
+ *	3. Update dev->stats asynchronously and atomically, and define
+ *	   neither operation.
  *
  * void (*ndo_vlan_rx_register)(struct net_device *dev, struct vlan_group *grp);
  *	If device support VLAN receive accleration
@@ -718,6 +752,7 @@ struct net_device_ops {
 						   struct neigh_parms *);
 	void			(*ndo_tx_timeout) (struct net_device *dev);
 
+	struct rtnl_link_stats64* (*ndo_get_stats64)(struct net_device *dev);
 	struct net_device_stats* (*ndo_get_stats)(struct net_device *dev);
 
 	void			(*ndo_vlan_rx_register)(struct net_device *dev,
@@ -867,7 +902,10 @@ struct net_device {
 	int			ifindex;
 	int			iflink;
 
-	struct net_device_stats	stats;
+	union {
+		struct rtnl_link_stats64 stats64;
+		struct net_device_stats stats;
+	};
 
 #ifdef CONFIG_WIRELESS_EXT
 	/* List of functions to handle Wireless Extensions (instead of ioctl).
@@ -2118,7 +2156,7 @@ extern void		netdev_features_change(struct net_device *dev);
 /* Load a device via the kmod */
 extern void		dev_load(struct net *net, const char *name);
 extern void		dev_mcast_init(void);
-extern const struct net_device_stats *dev_get_stats(struct net_device *dev);
+extern const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev);
 extern void		dev_txq_stats_fold(const struct net_device *dev, struct net_device_stats *stats);
 
 extern int		netdev_max_backlog;
diff --git a/net/8021q/vlanproc.c b/net/8021q/vlanproc.c
index afead35..62706f2 100644
--- a/net/8021q/vlanproc.c
+++ b/net/8021q/vlanproc.c
@@ -278,8 +278,9 @@ static int vlandev_seq_show(struct seq_file *seq, void *offset)
 {
 	struct net_device *vlandev = (struct net_device *) seq->private;
 	const struct vlan_dev_info *dev_info = vlan_dev_info(vlandev);
-	const struct net_device_stats *stats;
+	const struct rtnl_link_stats64 *stats;
 	static const char fmt[] = "%30s %12lu\n";
+	static const char fmt64[] = "%30s %12llu\n";
 	int i;
 
 	if (!is_vlan_dev(vlandev))
@@ -291,12 +292,17 @@ static int vlandev_seq_show(struct seq_file *seq, void *offset)
 		   vlandev->name, dev_info->vlan_id,
 		   (int)(dev_info->flags & 1), vlandev->priv_flags);
 
-	seq_printf(seq, fmt, "total frames received", stats->rx_packets);
-	seq_printf(seq, fmt, "total bytes received", stats->rx_bytes);
-	seq_printf(seq, fmt, "Broadcast/Multicast Rcvd", stats->multicast);
+	seq_printf(seq, fmt64, "total frames received",
+		   RTNL_LINK_STATS64_READ(stats, rx_packets));
+	seq_printf(seq, fmt64, "total bytes received",
+		   RTNL_LINK_STATS64_READ(stats, rx_bytes));
+	seq_printf(seq, fmt64, "Broadcast/Multicast Rcvd",
+		   RTNL_LINK_STATS64_READ(stats, multicast));
 	seq_puts(seq, "\n");
-	seq_printf(seq, fmt, "total frames transmitted", stats->tx_packets);
-	seq_printf(seq, fmt, "total bytes transmitted", stats->tx_bytes);
+	seq_printf(seq, fmt64, "total frames transmitted",
+		   RTNL_LINK_STATS64_READ(stats, tx_packets));
+	seq_printf(seq, fmt64, "total bytes transmitted",
+		   RTNL_LINK_STATS64_READ(stats, tx_bytes));
 	seq_printf(seq, fmt, "total headroom inc",
 		   dev_info->cnt_inc_headroom_on_tx);
 	seq_printf(seq, fmt, "total encap on xmit",
diff --git a/net/core/dev.c b/net/core/dev.c
index 983a3c1..aa48257 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3686,25 +3686,34 @@ void dev_seq_stop(struct seq_file *seq, void *v)
 
 static void dev_seq_printf_stats(struct seq_file *seq, struct net_device *dev)
 {
-	const struct net_device_stats *stats = dev_get_stats(dev);
-
-	seq_printf(seq, "%6s: %7lu %7lu %4lu %4lu %4lu %5lu %10lu %9lu "
-		   "%8lu %7lu %4lu %4lu %4lu %5lu %7lu %10lu\n",
-		   dev->name, stats->rx_bytes, stats->rx_packets,
-		   stats->rx_errors,
-		   stats->rx_dropped + stats->rx_missed_errors,
-		   stats->rx_fifo_errors,
-		   stats->rx_length_errors + stats->rx_over_errors +
-		    stats->rx_crc_errors + stats->rx_frame_errors,
-		   stats->rx_compressed, stats->multicast,
-		   stats->tx_bytes, stats->tx_packets,
-		   stats->tx_errors, stats->tx_dropped,
-		   stats->tx_fifo_errors, stats->collisions,
-		   stats->tx_carrier_errors +
-		    stats->tx_aborted_errors +
-		    stats->tx_window_errors +
-		    stats->tx_heartbeat_errors,
-		   stats->tx_compressed);
+	const struct rtnl_link_stats64 *stats = dev_get_stats(dev);
+
+	seq_printf(seq, "%6s: %7llu %7llu %4llu %4llu %4llu %5llu %10llu %9llu "
+		   "%8llu %7llu %4llu %4llu %4llu %5llu %7llu %10llu\n",
+		   dev->name,
+		   RTNL_LINK_STATS64_READ(stats, rx_bytes),
+		   RTNL_LINK_STATS64_READ(stats, rx_packets),
+		   RTNL_LINK_STATS64_READ(stats, rx_errors),
+		   RTNL_LINK_STATS64_READ(stats, rx_dropped) +
+		   RTNL_LINK_STATS64_READ(stats, rx_missed_errors),
+		   RTNL_LINK_STATS64_READ(stats, rx_fifo_errors),
+		   RTNL_LINK_STATS64_READ(stats, rx_length_errors) +
+		   RTNL_LINK_STATS64_READ(stats, rx_over_errors) +
+		   RTNL_LINK_STATS64_READ(stats, rx_crc_errors) +
+		   RTNL_LINK_STATS64_READ(stats, rx_frame_errors),
+		   RTNL_LINK_STATS64_READ(stats, rx_compressed),
+		   RTNL_LINK_STATS64_READ(stats, multicast),
+		   RTNL_LINK_STATS64_READ(stats, tx_bytes),
+		   RTNL_LINK_STATS64_READ(stats, tx_packets),
+		   RTNL_LINK_STATS64_READ(stats, tx_errors),
+		   RTNL_LINK_STATS64_READ(stats, tx_dropped),
+		   RTNL_LINK_STATS64_READ(stats, tx_fifo_errors),
+		   RTNL_LINK_STATS64_READ(stats, collisions),
+		   RTNL_LINK_STATS64_READ(stats, tx_carrier_errors) +
+		   RTNL_LINK_STATS64_READ(stats, tx_aborted_errors) +
+		   RTNL_LINK_STATS64_READ(stats, tx_window_errors) +
+		   RTNL_LINK_STATS64_READ(stats, tx_heartbeat_errors),
+		   RTNL_LINK_STATS64_READ(stats, tx_compressed));
 }
 
 /*
@@ -5266,18 +5275,21 @@ EXPORT_SYMBOL(dev_txq_stats_fold);
  *	@dev: device to get statistics from
  *
  *	Get network statistics from device. The device driver may provide
- *	its own method by setting dev->netdev_ops->get_stats; otherwise
- *	the internal statistics structure is used.
+ *	its own method by setting dev->netdev_ops->get_stats64 or
+ *	dev->netdev_ops->get_stats; otherwise the internal statistics
+ *	structure is used.
  */
-const struct net_device_stats *dev_get_stats(struct net_device *dev)
+const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 
+	if (ops->ndo_get_stats64)
+		return ops->ndo_get_stats64(dev);
 	if (ops->ndo_get_stats)
-		return ops->ndo_get_stats(dev);
+		return (struct rtnl_link_stats64 *)ops->ndo_get_stats(dev);
 
 	dev_txq_stats_fold(dev, &dev->stats);
-	return &dev->stats;
+	return &dev->stats64;
 }
 EXPORT_SYMBOL(dev_get_stats);
 
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 99e7052..a8dc20d 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -29,6 +29,7 @@ static const char fmt_hex[] = "%#x\n";
 static const char fmt_long_hex[] = "%#lx\n";
 static const char fmt_dec[] = "%d\n";
 static const char fmt_ulong[] = "%lu\n";
+static const char fmt_u64[] = "%llu\n";
 
 static inline int dev_isalive(const struct net_device *dev)
 {
@@ -324,14 +325,14 @@ static ssize_t netstat_show(const struct device *d,
 	struct net_device *dev = to_net_dev(d);
 	ssize_t ret = -EINVAL;
 
-	WARN_ON(offset > sizeof(struct net_device_stats) ||
-			offset % sizeof(unsigned long) != 0);
+	WARN_ON(offset > sizeof(struct rtnl_link_stats64) ||
+			offset % sizeof(u64) != 0);
 
 	read_lock(&dev_base_lock);
 	if (dev_isalive(dev)) {
-		const struct net_device_stats *stats = dev_get_stats(dev);
-		ret = sprintf(buf, fmt_ulong,
-			      *(unsigned long *)(((u8 *) stats) + offset));
+		const struct rtnl_link_stats64 *stats = dev_get_stats(dev);
+		ret = sprintf(buf, fmt_u64,
+			      RTNL_LINK_STATS64_READ_OFFSET(stats, offset));
 	}
 	read_unlock(&dev_base_lock);
 	return ret;
@@ -343,7 +344,7 @@ static ssize_t show_##name(struct device *d,				\
 			   struct device_attribute *attr, char *buf) 	\
 {									\
 	return netstat_show(d, attr, buf,				\
-			    offsetof(struct net_device_stats, name));	\
+			    offsetof(struct rtnl_link_stats64, name));	\
 }									\
 static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL)
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1a2af24..8b131ad 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -579,69 +579,67 @@ static unsigned int rtnl_dev_combine_flags(const struct net_device *dev,
 }
 
 static void copy_rtnl_link_stats(struct rtnl_link_stats *a,
-				 const struct net_device_stats *b)
+				 const struct rtnl_link_stats64 *b)
 {
-	a->rx_packets = b->rx_packets;
-	a->tx_packets = b->tx_packets;
-	a->rx_bytes = b->rx_bytes;
-	a->tx_bytes = b->tx_bytes;
-	a->rx_errors = b->rx_errors;
-	a->tx_errors = b->tx_errors;
-	a->rx_dropped = b->rx_dropped;
-	a->tx_dropped = b->tx_dropped;
-
-	a->multicast = b->multicast;
-	a->collisions = b->collisions;
-
-	a->rx_length_errors = b->rx_length_errors;
-	a->rx_over_errors = b->rx_over_errors;
-	a->rx_crc_errors = b->rx_crc_errors;
-	a->rx_frame_errors = b->rx_frame_errors;
-	a->rx_fifo_errors = b->rx_fifo_errors;
-	a->rx_missed_errors = b->rx_missed_errors;
-
-	a->tx_aborted_errors = b->tx_aborted_errors;
-	a->tx_carrier_errors = b->tx_carrier_errors;
-	a->tx_fifo_errors = b->tx_fifo_errors;
-	a->tx_heartbeat_errors = b->tx_heartbeat_errors;
-	a->tx_window_errors = b->tx_window_errors;
-
-	a->rx_compressed = b->rx_compressed;
-	a->tx_compressed = b->tx_compressed;
+	a->rx_packets = RTNL_LINK_STATS64_READ32(b, rx_packets);
+	a->tx_packets = RTNL_LINK_STATS64_READ32(b, tx_packets);
+	a->rx_bytes = RTNL_LINK_STATS64_READ32(b, rx_bytes);
+	a->tx_bytes = RTNL_LINK_STATS64_READ32(b, tx_bytes);
+	a->rx_errors = RTNL_LINK_STATS64_READ32(b, rx_errors);
+	a->tx_errors = RTNL_LINK_STATS64_READ32(b, tx_errors);
+	a->rx_dropped = RTNL_LINK_STATS64_READ32(b, rx_dropped);
+	a->tx_dropped = RTNL_LINK_STATS64_READ32(b, tx_dropped);
+
+	a->multicast = RTNL_LINK_STATS64_READ32(b, multicast);
+	a->collisions = RTNL_LINK_STATS64_READ32(b, collisions);
+
+	a->rx_length_errors = RTNL_LINK_STATS64_READ32(b, rx_length_errors);
+	a->rx_over_errors = RTNL_LINK_STATS64_READ32(b, rx_over_errors);
+	a->rx_crc_errors = RTNL_LINK_STATS64_READ32(b, rx_crc_errors);
+	a->rx_frame_errors = RTNL_LINK_STATS64_READ32(b, rx_frame_errors);
+	a->rx_fifo_errors = RTNL_LINK_STATS64_READ32(b, rx_fifo_errors);
+	a->rx_missed_errors = RTNL_LINK_STATS64_READ32(b, rx_missed_errors);
+
+	a->tx_aborted_errors = RTNL_LINK_STATS64_READ32(b, tx_aborted_errors);
+	a->tx_carrier_errors = RTNL_LINK_STATS64_READ32(b, tx_carrier_errors);
+	a->tx_fifo_errors = RTNL_LINK_STATS64_READ32(b, tx_fifo_errors);
+	a->tx_heartbeat_errors = RTNL_LINK_STATS64_READ32(b, tx_heartbeat_errors);
+	a->tx_window_errors = RTNL_LINK_STATS64_READ32(b, tx_window_errors);
+
+	a->rx_compressed = RTNL_LINK_STATS64_READ32(b, rx_compressed);
+	a->tx_compressed = RTNL_LINK_STATS64_READ32(b, tx_compressed);
 }
 
-static void copy_rtnl_link_stats64(void *v, const struct net_device_stats *b)
+static void copy_rtnl_link_stats64(struct rtnl_link_stats64 *a,
+				   const struct rtnl_link_stats64 *b)
 {
-	struct rtnl_link_stats64 a;
-
-	a.rx_packets = b->rx_packets;
-	a.tx_packets = b->tx_packets;
-	a.rx_bytes = b->rx_bytes;
-	a.tx_bytes = b->tx_bytes;
-	a.rx_errors = b->rx_errors;
-	a.tx_errors = b->tx_errors;
-	a.rx_dropped = b->rx_dropped;
-	a.tx_dropped = b->tx_dropped;
-
-	a.multicast = b->multicast;
-	a.collisions = b->collisions;
-
-	a.rx_length_errors = b->rx_length_errors;
-	a.rx_over_errors = b->rx_over_errors;
-	a.rx_crc_errors = b->rx_crc_errors;
-	a.rx_frame_errors = b->rx_frame_errors;
-	a.rx_fifo_errors = b->rx_fifo_errors;
-	a.rx_missed_errors = b->rx_missed_errors;
-
-	a.tx_aborted_errors = b->tx_aborted_errors;
-	a.tx_carrier_errors = b->tx_carrier_errors;
-	a.tx_fifo_errors = b->tx_fifo_errors;
-	a.tx_heartbeat_errors = b->tx_heartbeat_errors;
-	a.tx_window_errors = b->tx_window_errors;
-
-	a.rx_compressed = b->rx_compressed;
-	a.tx_compressed = b->tx_compressed;
-	memcpy(v, &a, sizeof(a));
+	a->rx_packets = RTNL_LINK_STATS64_READ(b, rx_packets);
+	a->tx_packets = RTNL_LINK_STATS64_READ(b, tx_packets);
+	a->rx_bytes = RTNL_LINK_STATS64_READ(b, rx_bytes);
+	a->tx_bytes = RTNL_LINK_STATS64_READ(b, tx_bytes);
+	a->rx_errors = RTNL_LINK_STATS64_READ(b, rx_errors);
+	a->tx_errors = RTNL_LINK_STATS64_READ(b, tx_errors);
+	a->rx_dropped = RTNL_LINK_STATS64_READ(b, rx_dropped);
+	a->tx_dropped = RTNL_LINK_STATS64_READ(b, tx_dropped);
+
+	a->multicast = RTNL_LINK_STATS64_READ(b, multicast);
+	a->collisions = RTNL_LINK_STATS64_READ(b, collisions);
+
+	a->rx_length_errors = RTNL_LINK_STATS64_READ(b, rx_length_errors);
+	a->rx_over_errors = RTNL_LINK_STATS64_READ(b, rx_over_errors);
+	a->rx_crc_errors = RTNL_LINK_STATS64_READ(b, rx_crc_errors);
+	a->rx_frame_errors = RTNL_LINK_STATS64_READ(b, rx_frame_errors);
+	a->rx_fifo_errors = RTNL_LINK_STATS64_READ(b, rx_fifo_errors);
+	a->rx_missed_errors = RTNL_LINK_STATS64_READ(b, rx_missed_errors);
+
+	a->tx_aborted_errors = RTNL_LINK_STATS64_READ(b, tx_aborted_errors);
+	a->tx_carrier_errors = RTNL_LINK_STATS64_READ(b, tx_carrier_errors);
+	a->tx_fifo_errors = RTNL_LINK_STATS64_READ(b, tx_fifo_errors);
+	a->tx_heartbeat_errors = RTNL_LINK_STATS64_READ(b, tx_heartbeat_errors);
+	a->tx_window_errors = RTNL_LINK_STATS64_READ(b, tx_window_errors);
+
+	a->rx_compressed = RTNL_LINK_STATS64_READ(b, rx_compressed);
+	a->tx_compressed = RTNL_LINK_STATS64_READ(b, tx_compressed);
 }
 
 /* All VF info */
@@ -791,7 +789,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 {
 	struct ifinfomsg *ifm;
 	struct nlmsghdr *nlh;
-	const struct net_device_stats *stats;
+	const struct rtnl_link_stats64 *stats;
 	struct nlattr *attr;
 
 	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*ifm), flags);
-- 
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.

Re: [PATCH 1/2] net: Enable 64-bit net device statistics on 32-bit architectures

[Stephen Hemminger]

On Thu, 03 Jun 2010 18:39:04 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> #if BITS_PER_LONG == 64
> +#define NET_DEVICE_STATS_DEFINE(name)	u64 name
> +#define RTNL_LINK_STATS64_READ(stats, name)				\
> +	ACCESS_ONCE((stats)->name)
> +#define RTNL_LINK_STATS64_READ_OFFSET(stats, offset)			\
> +	ACCESS_ONCE((const u64 *)((const u8 *)(stats) + (offset)))
> +#define RTNL_LINK_STATS64_READ32(stats, name)				\
> +	((u32)ACCESS_ONCE((stats)->name))
> +#else
> +#if defined(__LITTLE_ENDIAN)
> +#define NET_DEVICE_STATS_DEFINE(name)	u32 name, pad_ ## name
> +#define RTNL_LINK_STATS64_READ_OFFSET(stats, offset)			\
> +	(ACCESS_ONCE(*(const u32 *)((const u8 *)(stats) + (offset))) |	\
> +	 (u64)(*(const u32 *)((const u8 *)(stats) + (offset) + 4)) << 32)
> +#define RTNL_LINK_STATS64_READ32(stats, name)				\
> +	(((const volatile u32 *)&(stats)->name)[0])
> +#else
> +#define NET_DEVICE_STATS_DEFINE(name)	u32 pad_ ## name, name
> +#define RTNL_LINK_STATS64_READ_OFFSET(stats, offset)			\
> +	((u64)(*(const u32 *)((const u8 *)(stats) + (offset))) << 32 |	\
> +	 ACCESS_ONCE(*(const u32 *)((const u8 *)(stats) + (offset) + 4)))
> +#define RTNL_LINK_STATS64_READ32(stats, name)				\
> +	(((const volatile u32 *)&(stats)->name)[1])
> +#endif
> +#define RTNL_LINK_STATS64_READ(stats, name)				\
> +	RTNL_LINK_STATS64_READ_OFFSET(					\
> +		stats, offsetof(struct rtnl_link_stats64, name))
> +#endif

Macros... with multiple casts. Gack

-- 

Re: [PATCH 1/2] net: Enable 64-bit net device statistics on 32-bit architectures

[Ben Hutchings]

On Thu, 2010-06-03 at 11:47 -0700, Stephen Hemminger wrote:
> On Thu, 03 Jun 2010 18:39:04 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > #if BITS_PER_LONG == 64
> > +#define NET_DEVICE_STATS_DEFINE(name)	u64 name
> > +#define RTNL_LINK_STATS64_READ(stats, name)				\
> > +	ACCESS_ONCE((stats)->name)
> > +#define RTNL_LINK_STATS64_READ_OFFSET(stats, offset)			\
> > +	ACCESS_ONCE((const u64 *)((const u8 *)(stats) + (offset)))
> > +#define RTNL_LINK_STATS64_READ32(stats, name)				\
> > +	((u32)ACCESS_ONCE((stats)->name))
> > +#else
> > +#if defined(__LITTLE_ENDIAN)
> > +#define NET_DEVICE_STATS_DEFINE(name)	u32 name, pad_ ## name
> > +#define RTNL_LINK_STATS64_READ_OFFSET(stats, offset)			\
> > +	(ACCESS_ONCE(*(const u32 *)((const u8 *)(stats) + (offset))) |	\
> > +	 (u64)(*(const u32 *)((const u8 *)(stats) + (offset) + 4)) << 32)
> > +#define RTNL_LINK_STATS64_READ32(stats, name)				\
> > +	(((const volatile u32 *)&(stats)->name)[0])
> > +#else
> > +#define NET_DEVICE_STATS_DEFINE(name)	u32 pad_ ## name, name
> > +#define RTNL_LINK_STATS64_READ_OFFSET(stats, offset)			\
> > +	((u64)(*(const u32 *)((const u8 *)(stats) + (offset))) << 32 |	\
> > +	 ACCESS_ONCE(*(const u32 *)((const u8 *)(stats) + (offset) + 4)))
> > +#define RTNL_LINK_STATS64_READ32(stats, name)				\
> > +	(((const volatile u32 *)&(stats)->name)[1])
> > +#endif
> > +#define RTNL_LINK_STATS64_READ(stats, name)				\
> > +	RTNL_LINK_STATS64_READ_OFFSET(					\
> > +		stats, offsetof(struct rtnl_link_stats64, name))
> > +#endif
> 
> 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.

Re: [PATCH 1/2] net: Enable 64-bit net device statistics on 32-bit architectures

[Stephen Hemminger]

On Thu, 03 Jun 2010 20:11:38 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> 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);
> }

Do we really care if compiler reorders access. I think not.
There was no order guarantee in the past.

-- 

Re: [PATCH 1/2] net: Enable 64-bit net device statistics on 32-bit architectures

[Ben Hutchings]

On Fri, 2010-06-04 at 10:28 -0700, Stephen Hemminger wrote:
> On Thu, 03 Jun 2010 20:11:38 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > 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);
> > }
> 
> 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.

Re: [PATCH 1/2] net: Enable 64-bit net device statistics on 32-bit architectures

[Stephen Hemminger]

On Fri, 04 Jun 2010 19:15:18 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Fri, 2010-06-04 at 10:28 -0700, Stephen Hemminger wrote:
> > On Thu, 03 Jun 2010 20:11:38 +0100
> > Ben Hutchings <bhutchings@solarflare.com> wrote:
> > 
> > > 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);
> > > }
> > 
> > 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.
> 

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.

-- 

Re: [PATCH 1/2] net: Enable 64-bit net device statistics on 32-bit architectures

[Ben Hutchings]

On Fri, 2010-06-04 at 13:39 -0700, Stephen Hemminger wrote:
> On Fri, 04 Jun 2010 19:15:18 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > On Fri, 2010-06-04 at 10:28 -0700, Stephen Hemminger wrote:
> > > On Thu, 03 Jun 2010 20:11:38 +0100
> > > Ben Hutchings <bhutchings@solarflare.com> wrote:
> > > 
> > > > 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);
> > > > }
> > > 
> > > 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.
> > 
> 
> 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.

[PATCH 2/2] sfc: Implement 64-bit net device statistics on all architectures

[Ben Hutchings]

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.

Re: 64-bit net_device_stats

[Junchang Wang]

Hi Ben,

On Wed, Jun 02, 2010 at 10:34:29PM +0100, Ben Hutchings wrote:
>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:
>
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;#282631

And Eric just gave a explanation why they stand by that point. Updating rx_* counters in core network will dirty a cache line.

--Junchang

Re: 64-bit net_device_stats

[Eric Dumazet]

Le vendredi 04 juin 2010 à 09:59 +0800, Junchang Wang a écrit :
> Hi Ben,
> 
> On Wed, Jun 02, 2010 at 10:34:29PM +0100, Ben Hutchings wrote:
> >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:
> >
> 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;#282631
> 

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
the writer.


> And Eric just gave a explanation why they stand by that point. Updating rx_* counters in core network will dirty a cache line.

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)