* [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union
@ 2010-07-08 23:29 Ben Hutchings
2010-07-08 23:30 ` [PATCH net-next-2.6 2/2] net: Document that dev_get_stats() returns the given pointer Ben Hutchings
2010-07-09 4:25 ` [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union Eric Dumazet
0 siblings, 2 replies; 7+ messages in thread
From: Ben Hutchings @ 2010-07-08 23:29 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers, Eric Dumazet
In commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b "net: Enable 64-bit
net device statistics on 32-bit architectures" I redefined struct
net_device_stats so that it could be used in a union with struct
rtnl_link_stats64, avoiding the need for explicit copying or
conversion between the two. However, this is unsafe because there is
no locking required and no lock consistently held around calls to
dev_get_stats() and use of the statistics structure it returns.
In commit 28172739f0a276eb8d6ca917b3974c2edb036da3 "net: fix 64 bit
counters on 32 bit arches" Eric Dumazet dealt with that problem by
requiring callers of dev_get_stats() to provide storage for the
result. This means that the net_device::stats64 field and the padding
in struct net_device_stats are now redundant, so remove them.
Update the comment on net_device_ops::ndo_get_stats64 to reflect its
new usage.
Change dev_txq_stats_fold() to use struct rtnl_link_stats64, since
that is what all its callers are really using and it is no longer
going to be compatible with struct net_device_stats.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/macvlan.c | 2 +-
include/linux/netdevice.h | 70 ++++++++++++++++++---------------------------
net/8021q/vlan_dev.c | 2 +-
net/core/dev.c | 19 +++++++++---
4 files changed, 44 insertions(+), 49 deletions(-)
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 6112f14..1b28aae 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -436,7 +436,7 @@ static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev,
{
struct macvlan_dev *vlan = netdev_priv(dev);
- dev_txq_stats_fold(dev, (struct net_device_stats *)stats);
+ dev_txq_stats_fold(dev, stats);
if (vlan->rx_stats) {
struct macvlan_rx_stats *p, accum = {0};
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8018f6b..17e95e3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -162,42 +162,32 @@ static inline bool dev_xmit_complete(int rc)
/*
* 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) unsigned long name
-#elif defined(__LITTLE_ENDIAN)
-#define NET_DEVICE_STATS_DEFINE(name) unsigned long name, pad_ ## name
-#else
-#define NET_DEVICE_STATS_DEFINE(name) unsigned long pad_ ## name, name
-#endif
-
struct net_device_stats {
- 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);
+ unsigned long rx_packets;
+ unsigned long tx_packets;
+ unsigned long rx_bytes;
+ unsigned long tx_bytes;
+ unsigned long rx_errors;
+ unsigned long tx_errors;
+ unsigned long rx_dropped;
+ unsigned long tx_dropped;
+ unsigned long multicast;
+ unsigned long collisions;
+ unsigned long rx_length_errors;
+ unsigned long rx_over_errors;
+ unsigned long rx_crc_errors;
+ unsigned long rx_frame_errors;
+ unsigned long rx_fifo_errors;
+ unsigned long rx_missed_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;
+ unsigned long rx_compressed;
+ unsigned long tx_compressed;
};
#endif /* __KERNEL__ */
@@ -666,14 +656,13 @@ 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 rtnl_link_stats64* (*ndo_get_stats64)(struct net_device *dev,
* struct rtnl_link_stats64 *storage);
* struct net_device_stats* (*ndo_get_stats)(struct net_device *dev);
* Called when a user wants to get the network device usage
* 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.
+ * 1. Define @ndo_get_stats64 to fill in a zero-initialised
+ * rtnl_link_stats64 structure passed by the caller.
* 2. Define @ndo_get_stats to update a net_device_stats structure
* (which should normally be dev->stats) and return a pointer to
* it. The structure may be changed asynchronously only if each
@@ -888,10 +877,7 @@ struct net_device {
int ifindex;
int iflink;
- union {
- struct rtnl_link_stats64 stats64;
- struct net_device_stats stats;
- };
+ struct net_device_stats stats;
#ifdef CONFIG_WIRELESS_EXT
/* List of functions to handle Wireless Extensions (instead of ioctl).
@@ -2147,7 +2133,7 @@ extern void dev_mcast_init(void);
extern const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
struct rtnl_link_stats64 *storage);
extern void dev_txq_stats_fold(const struct net_device *dev,
- struct net_device_stats *stats);
+ struct rtnl_link_stats64 *stats);
extern int netdev_max_backlog;
extern int netdev_tstamp_prequeue;
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 7865a4c..e66c9bf 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -805,7 +805,7 @@ static u32 vlan_ethtool_get_flags(struct net_device *dev)
static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
{
- dev_txq_stats_fold(dev, (struct net_device_stats *)stats);
+ dev_txq_stats_fold(dev, stats);
if (vlan_dev_info(dev)->vlan_rx_stats) {
struct vlan_rx_stats *p, accum = {0};
diff --git a/net/core/dev.c b/net/core/dev.c
index eb4201c..5ec2eec 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5274,10 +5274,10 @@ void netdev_run_todo(void)
/**
* dev_txq_stats_fold - fold tx_queues stats
* @dev: device to get statistics from
- * @stats: struct net_device_stats to hold results
+ * @stats: struct rtnl_link_stats64 to hold results
*/
void dev_txq_stats_fold(const struct net_device *dev,
- struct net_device_stats *stats)
+ struct rtnl_link_stats64 *stats)
{
unsigned long tx_bytes = 0, tx_packets = 0, tx_dropped = 0;
unsigned int i;
@@ -5311,17 +5311,26 @@ const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
struct rtnl_link_stats64 *storage)
{
const struct net_device_ops *ops = dev->netdev_ops;
+ size_t i, n = sizeof(*storage) / sizeof(u64);
+ u64 *stats64 = (u64 *)storage;
+ const unsigned long *stats;
if (ops->ndo_get_stats64) {
memset(storage, 0, sizeof(*storage));
return ops->ndo_get_stats64(dev, storage);
}
+
if (ops->ndo_get_stats) {
- memcpy(storage, ops->ndo_get_stats(dev), sizeof(*storage));
+ stats = (const unsigned long *)ops->ndo_get_stats(dev);
+ for (i = 0; i < n; i++)
+ stats64[i] = stats[i];
return storage;
}
- memcpy(storage, &dev->stats, sizeof(*storage));
- dev_txq_stats_fold(dev, (struct net_device_stats *)storage);
+
+ stats = (const unsigned long *)&dev->stats;
+ for (i = 0; i < n; i++)
+ stats64[i] = stats[i];
+ dev_txq_stats_fold(dev, storage);
return storage;
}
EXPORT_SYMBOL(dev_get_stats);
--
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.
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next-2.6 2/2] net: Document that dev_get_stats() returns the given pointer
2010-07-08 23:29 [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union Ben Hutchings
@ 2010-07-08 23:30 ` Ben Hutchings
2010-07-09 4:25 ` [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union Eric Dumazet
1 sibling, 0 replies; 7+ messages in thread
From: Ben Hutchings @ 2010-07-08 23:30 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers, Eric Dumazet
Document that dev_get_stats() returns the same stats pointer it was
given. Remove const qualification from the returned pointer since the
caller may do what it likes with that structure.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
include/linux/netdevice.h | 4 ++--
net/core/dev.c | 12 ++++++------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 17e95e3..c4fedf0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2130,8 +2130,8 @@ 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 rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
- struct rtnl_link_stats64 *storage);
+extern struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
+ struct rtnl_link_stats64 *storage);
extern void dev_txq_stats_fold(const struct net_device *dev,
struct rtnl_link_stats64 *stats);
diff --git a/net/core/dev.c b/net/core/dev.c
index 5ec2eec..2605a68 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5302,13 +5302,13 @@ EXPORT_SYMBOL(dev_txq_stats_fold);
* @dev: device to get statistics from
* @storage: place to store stats
*
- * Get network statistics from device. The device driver may provide
- * its own method by setting dev->netdev_ops->get_stats64 or
- * dev->netdev_ops->get_stats; otherwise the internal statistics
- * structure is used.
+ * Get network statistics from device. Return @storage.
+ * The device driver may provide 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 rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
- struct rtnl_link_stats64 *storage)
+struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
+ struct rtnl_link_stats64 *storage)
{
const struct net_device_ops *ops = dev->netdev_ops;
size_t i, n = sizeof(*storage) / sizeof(u64);
--
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.
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union
2010-07-08 23:29 [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union Ben Hutchings
2010-07-08 23:30 ` [PATCH net-next-2.6 2/2] net: Document that dev_get_stats() returns the given pointer Ben Hutchings
@ 2010-07-09 4:25 ` Eric Dumazet
2010-07-09 6:46 ` David Miller
1 sibling, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2010-07-09 4:25 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, linux-net-drivers
Le vendredi 09 juillet 2010 à 00:29 +0100, Ben Hutchings a écrit :
> In commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b "net: Enable 64-bit
> net device statistics on 32-bit architectures" I redefined struct
> net_device_stats so that it could be used in a union with struct
> rtnl_link_stats64, avoiding the need for explicit copying or
> conversion between the two. However, this is unsafe because there is
> no locking required and no lock consistently held around calls to
> dev_get_stats() and use of the statistics structure it returns.
>
> In commit 28172739f0a276eb8d6ca917b3974c2edb036da3 "net: fix 64 bit
> counters on 32 bit arches" Eric Dumazet dealt with that problem by
> requiring callers of dev_get_stats() to provide storage for the
> result. This means that the net_device::stats64 field and the padding
> in struct net_device_stats are now redundant, so remove them.
>
> Update the comment on net_device_ops::ndo_get_stats64 to reflect its
> new usage.
>
> Change dev_txq_stats_fold() to use struct rtnl_link_stats64, since
> that is what all its callers are really using and it is no longer
> going to be compatible with struct net_device_stats.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> drivers/net/macvlan.c | 2 +-
> include/linux/netdevice.h | 70 ++++++++++++++++++---------------------------
> net/8021q/vlan_dev.c | 2 +-
> net/core/dev.c | 19 +++++++++---
> 4 files changed, 44 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 6112f14..1b28aae 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -436,7 +436,7 @@ static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev,
> {
> struct macvlan_dev *vlan = netdev_priv(dev);
>
> - dev_txq_stats_fold(dev, (struct net_device_stats *)stats);
> + dev_txq_stats_fold(dev, stats);
>
> if (vlan->rx_stats) {
> struct macvlan_rx_stats *p, accum = {0};
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 8018f6b..17e95e3 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -162,42 +162,32 @@ static inline bool dev_xmit_complete(int rc)
> /*
> * 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) unsigned long name
> -#elif defined(__LITTLE_ENDIAN)
> -#define NET_DEVICE_STATS_DEFINE(name) unsigned long name, pad_ ## name
> -#else
> -#define NET_DEVICE_STATS_DEFINE(name) unsigned long pad_ ## name, name
> -#endif
> -
> struct net_device_stats {
> - 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);
> + unsigned long rx_packets;
> + unsigned long tx_packets;
> + unsigned long rx_bytes;
> + unsigned long tx_bytes;
> + unsigned long rx_errors;
> + unsigned long tx_errors;
> + unsigned long rx_dropped;
> + unsigned long tx_dropped;
> + unsigned long multicast;
> + unsigned long collisions;
> + unsigned long rx_length_errors;
> + unsigned long rx_over_errors;
> + unsigned long rx_crc_errors;
> + unsigned long rx_frame_errors;
> + unsigned long rx_fifo_errors;
> + unsigned long rx_missed_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;
> + unsigned long rx_compressed;
> + unsigned long tx_compressed;
> };
>
> #endif /* __KERNEL__ */
> @@ -666,14 +656,13 @@ 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 rtnl_link_stats64* (*ndo_get_stats64)(struct net_device *dev,
> * struct rtnl_link_stats64 *storage);
> * struct net_device_stats* (*ndo_get_stats)(struct net_device *dev);
> * Called when a user wants to get the network device usage
> * 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.
> + * 1. Define @ndo_get_stats64 to fill in a zero-initialised
> + * rtnl_link_stats64 structure passed by the caller.
> * 2. Define @ndo_get_stats to update a net_device_stats structure
> * (which should normally be dev->stats) and return a pointer to
> * it. The structure may be changed asynchronously only if each
> @@ -888,10 +877,7 @@ struct net_device {
> int ifindex;
> int iflink;
>
> - union {
> - struct rtnl_link_stats64 stats64;
> - struct net_device_stats stats;
> - };
> + struct net_device_stats stats;
>
> #ifdef CONFIG_WIRELESS_EXT
> /* List of functions to handle Wireless Extensions (instead of ioctl).
> @@ -2147,7 +2133,7 @@ extern void dev_mcast_init(void);
> extern const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
> struct rtnl_link_stats64 *storage);
> extern void dev_txq_stats_fold(const struct net_device *dev,
> - struct net_device_stats *stats);
> + struct rtnl_link_stats64 *stats);
>
> extern int netdev_max_backlog;
> extern int netdev_tstamp_prequeue;
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 7865a4c..e66c9bf 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -805,7 +805,7 @@ static u32 vlan_ethtool_get_flags(struct net_device *dev)
>
> static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
> {
> - dev_txq_stats_fold(dev, (struct net_device_stats *)stats);
> + dev_txq_stats_fold(dev, stats);
>
> if (vlan_dev_info(dev)->vlan_rx_stats) {
> struct vlan_rx_stats *p, accum = {0};
> diff --git a/net/core/dev.c b/net/core/dev.c
> index eb4201c..5ec2eec 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5274,10 +5274,10 @@ void netdev_run_todo(void)
> /**
> * dev_txq_stats_fold - fold tx_queues stats
> * @dev: device to get statistics from
> - * @stats: struct net_device_stats to hold results
> + * @stats: struct rtnl_link_stats64 to hold results
> */
> void dev_txq_stats_fold(const struct net_device *dev,
> - struct net_device_stats *stats)
> + struct rtnl_link_stats64 *stats)
> {
> unsigned long tx_bytes = 0, tx_packets = 0, tx_dropped = 0;
> unsigned int i;
> @@ -5311,17 +5311,26 @@ const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
> struct rtnl_link_stats64 *storage)
> {
> const struct net_device_ops *ops = dev->netdev_ops;
> + size_t i, n = sizeof(*storage) / sizeof(u64);
> + u64 *stats64 = (u64 *)storage;
> + const unsigned long *stats;
>
> if (ops->ndo_get_stats64) {
> memset(storage, 0, sizeof(*storage));
> return ops->ndo_get_stats64(dev, storage);
> }
> +
> if (ops->ndo_get_stats) {
> - memcpy(storage, ops->ndo_get_stats(dev), sizeof(*storage));
> + stats = (const unsigned long *)ops->ndo_get_stats(dev);
> + for (i = 0; i < n; i++)
> + stats64[i] = stats[i];
This could be a small helper function, to hide the sizeof(*storage) /
sizeof(u64) magic..
static void stats_to_stats64(struct rtnl_link_stats64 *dst,
const net_device_stats *src)
{
#if BITS_PER_LONG == 64
BUILD_BUG_ON(sizeof(*dst) != sizeof(*src));
memcpy(dst, src, sizeof(*src));
#else
size_t i, n = sizeof(*dst) / sizeof(u64);
u64 *stats64 = (u64 *)dst;
const unsigned long *stats = (const unsigned long *)src;
BUILD_BUG_ON(sizeof(*src) != n * sizeof(unsigned long));
for (i = 0; i < n, i++)
stats64[i] = stats[i];
#endif
}
Thanks !
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union
2010-07-09 4:25 ` [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union Eric Dumazet
@ 2010-07-09 6:46 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2010-07-09 6:46 UTC (permalink / raw)
To: eric.dumazet; +Cc: bhutchings, netdev, linux-net-drivers
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 09 Jul 2010 06:25:43 +0200
> This could be a small helper function, to hide the sizeof(*storage) /
> sizeof(u64) magic..
>
> static void stats_to_stats64(struct rtnl_link_stats64 *dst,
> const net_device_stats *src)
> {
Ben, please add Eric' suggestion and respin your second patch as
well.
Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union
2010-07-09 21:08 ` Eric Dumazet
@ 2010-07-10 1:01 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2010-07-10 1:01 UTC (permalink / raw)
To: eric.dumazet; +Cc: bhutchings, netdev, linux-net-drivers
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 09 Jul 2010 23:08:54 +0200
> Le vendredi 09 juillet 2010 à 20:11 +0100, Ben Hutchings a écrit :
>> In commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b "net: Enable 64-bit
>> net device statistics on 32-bit architectures" I redefined struct
>> net_device_stats so that it could be used in a union with struct
>> rtnl_link_stats64, avoiding the need for explicit copying or
>> conversion between the two. However, this is unsafe because there is
>> no locking required and no lock consistently held around calls to
>> dev_get_stats() and use of the statistics structure it returns.
>>
>> In commit 28172739f0a276eb8d6ca917b3974c2edb036da3 "net: fix 64 bit
>> counters on 32 bit arches" Eric Dumazet dealt with that problem by
>> requiring callers of dev_get_stats() to provide storage for the
>> result. This means that the net_device::stats64 field and the padding
>> in struct net_device_stats are now redundant, so remove them.
>>
>> Update the comment on net_device_ops::ndo_get_stats64 to reflect its
>> new usage.
>>
>> Change dev_txq_stats_fold() to use struct rtnl_link_stats64, since
>> that is what all its callers are really using and it is no longer
>> going to be compatible with struct net_device_stats.
>>
>> Eric Dumazet suggested the separate function for the structure
>> conversion.
>>
>> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union
2010-07-09 19:11 Ben Hutchings
@ 2010-07-09 21:08 ` Eric Dumazet
2010-07-10 1:01 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2010-07-09 21:08 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, linux-net-drivers
Le vendredi 09 juillet 2010 à 20:11 +0100, Ben Hutchings a écrit :
> In commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b "net: Enable 64-bit
> net device statistics on 32-bit architectures" I redefined struct
> net_device_stats so that it could be used in a union with struct
> rtnl_link_stats64, avoiding the need for explicit copying or
> conversion between the two. However, this is unsafe because there is
> no locking required and no lock consistently held around calls to
> dev_get_stats() and use of the statistics structure it returns.
>
> In commit 28172739f0a276eb8d6ca917b3974c2edb036da3 "net: fix 64 bit
> counters on 32 bit arches" Eric Dumazet dealt with that problem by
> requiring callers of dev_get_stats() to provide storage for the
> result. This means that the net_device::stats64 field and the padding
> in struct net_device_stats are now redundant, so remove them.
>
> Update the comment on net_device_ops::ndo_get_stats64 to reflect its
> new usage.
>
> Change dev_txq_stats_fold() to use struct rtnl_link_stats64, since
> that is what all its callers are really using and it is no longer
> going to be compatible with struct net_device_stats.
>
> Eric Dumazet suggested the separate function for the structure
> conversion.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Thanks !
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union
@ 2010-07-09 19:11 Ben Hutchings
2010-07-09 21:08 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Ben Hutchings @ 2010-07-09 19:11 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers, Eric Dumazet
In commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b "net: Enable 64-bit
net device statistics on 32-bit architectures" I redefined struct
net_device_stats so that it could be used in a union with struct
rtnl_link_stats64, avoiding the need for explicit copying or
conversion between the two. However, this is unsafe because there is
no locking required and no lock consistently held around calls to
dev_get_stats() and use of the statistics structure it returns.
In commit 28172739f0a276eb8d6ca917b3974c2edb036da3 "net: fix 64 bit
counters on 32 bit arches" Eric Dumazet dealt with that problem by
requiring callers of dev_get_stats() to provide storage for the
result. This means that the net_device::stats64 field and the padding
in struct net_device_stats are now redundant, so remove them.
Update the comment on net_device_ops::ndo_get_stats64 to reflect its
new usage.
Change dev_txq_stats_fold() to use struct rtnl_link_stats64, since
that is what all its callers are really using and it is no longer
going to be compatible with struct net_device_stats.
Eric Dumazet suggested the separate function for the structure
conversion.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/macvlan.c | 2 +-
include/linux/netdevice.h | 70 ++++++++++++++++++---------------------------
net/8021q/vlan_dev.c | 2 +-
net/core/dev.c | 31 ++++++++++++++++---
4 files changed, 56 insertions(+), 49 deletions(-)
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 6112f14..1b28aae 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -436,7 +436,7 @@ static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev,
{
struct macvlan_dev *vlan = netdev_priv(dev);
- dev_txq_stats_fold(dev, (struct net_device_stats *)stats);
+ dev_txq_stats_fold(dev, stats);
if (vlan->rx_stats) {
struct macvlan_rx_stats *p, accum = {0};
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8018f6b..17e95e3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -162,42 +162,32 @@ static inline bool dev_xmit_complete(int rc)
/*
* 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) unsigned long name
-#elif defined(__LITTLE_ENDIAN)
-#define NET_DEVICE_STATS_DEFINE(name) unsigned long name, pad_ ## name
-#else
-#define NET_DEVICE_STATS_DEFINE(name) unsigned long pad_ ## name, name
-#endif
-
struct net_device_stats {
- 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);
+ unsigned long rx_packets;
+ unsigned long tx_packets;
+ unsigned long rx_bytes;
+ unsigned long tx_bytes;
+ unsigned long rx_errors;
+ unsigned long tx_errors;
+ unsigned long rx_dropped;
+ unsigned long tx_dropped;
+ unsigned long multicast;
+ unsigned long collisions;
+ unsigned long rx_length_errors;
+ unsigned long rx_over_errors;
+ unsigned long rx_crc_errors;
+ unsigned long rx_frame_errors;
+ unsigned long rx_fifo_errors;
+ unsigned long rx_missed_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;
+ unsigned long rx_compressed;
+ unsigned long tx_compressed;
};
#endif /* __KERNEL__ */
@@ -666,14 +656,13 @@ 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 rtnl_link_stats64* (*ndo_get_stats64)(struct net_device *dev,
* struct rtnl_link_stats64 *storage);
* struct net_device_stats* (*ndo_get_stats)(struct net_device *dev);
* Called when a user wants to get the network device usage
* 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.
+ * 1. Define @ndo_get_stats64 to fill in a zero-initialised
+ * rtnl_link_stats64 structure passed by the caller.
* 2. Define @ndo_get_stats to update a net_device_stats structure
* (which should normally be dev->stats) and return a pointer to
* it. The structure may be changed asynchronously only if each
@@ -888,10 +877,7 @@ struct net_device {
int ifindex;
int iflink;
- union {
- struct rtnl_link_stats64 stats64;
- struct net_device_stats stats;
- };
+ struct net_device_stats stats;
#ifdef CONFIG_WIRELESS_EXT
/* List of functions to handle Wireless Extensions (instead of ioctl).
@@ -2147,7 +2133,7 @@ extern void dev_mcast_init(void);
extern const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
struct rtnl_link_stats64 *storage);
extern void dev_txq_stats_fold(const struct net_device *dev,
- struct net_device_stats *stats);
+ struct rtnl_link_stats64 *stats);
extern int netdev_max_backlog;
extern int netdev_tstamp_prequeue;
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index a1b8171..7cb285f 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -805,7 +805,7 @@ static u32 vlan_ethtool_get_flags(struct net_device *dev)
static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
{
- dev_txq_stats_fold(dev, (struct net_device_stats *)stats);
+ dev_txq_stats_fold(dev, stats);
if (vlan_dev_info(dev)->vlan_rx_stats) {
struct vlan_rx_stats *p, accum = {0};
diff --git a/net/core/dev.c b/net/core/dev.c
index eb4201c..79ee26e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5274,10 +5274,10 @@ void netdev_run_todo(void)
/**
* dev_txq_stats_fold - fold tx_queues stats
* @dev: device to get statistics from
- * @stats: struct net_device_stats to hold results
+ * @stats: struct rtnl_link_stats64 to hold results
*/
void dev_txq_stats_fold(const struct net_device *dev,
- struct net_device_stats *stats)
+ struct rtnl_link_stats64 *stats)
{
unsigned long tx_bytes = 0, tx_packets = 0, tx_dropped = 0;
unsigned int i;
@@ -5297,6 +5297,27 @@ void dev_txq_stats_fold(const struct net_device *dev,
}
EXPORT_SYMBOL(dev_txq_stats_fold);
+/* Convert net_device_stats to rtnl_link_stats64. They have the same
+ * fields in the same order, with only the type differing.
+ */
+static void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
+ const struct net_device_stats *netdev_stats)
+{
+#if BITS_PER_LONG == 64
+ BUILD_BUG_ON(sizeof(*stats64) != sizeof(*netdev_stats));
+ memcpy(stats64, netdev_stats, sizeof(*stats64));
+#else
+ size_t i, n = sizeof(*stats64) / sizeof(u64);
+ const unsigned long *src = (const unsigned long *)netdev_stats;
+ u64 *dst = (u64 *)stats64;
+
+ BUILD_BUG_ON(sizeof(*netdev_stats) / sizeof(unsigned long) !=
+ sizeof(*stats64) / sizeof(u64));
+ for (i = 0; i < n; i++)
+ dst[i] = src[i];
+#endif
+}
+
/**
* dev_get_stats - get network device statistics
* @dev: device to get statistics from
@@ -5317,11 +5338,11 @@ const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
return ops->ndo_get_stats64(dev, storage);
}
if (ops->ndo_get_stats) {
- memcpy(storage, ops->ndo_get_stats(dev), sizeof(*storage));
+ netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev));
return storage;
}
- memcpy(storage, &dev->stats, sizeof(*storage));
- dev_txq_stats_fold(dev, (struct net_device_stats *)storage);
+ netdev_stats_to_stats64(storage, &dev->stats);
+ dev_txq_stats_fold(dev, storage);
return storage;
}
EXPORT_SYMBOL(dev_get_stats);
--
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.
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-07-10 1:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-08 23:29 [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union Ben Hutchings
2010-07-08 23:30 ` [PATCH net-next-2.6 2/2] net: Document that dev_get_stats() returns the given pointer Ben Hutchings
2010-07-09 4:25 ` [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union Eric Dumazet
2010-07-09 6:46 ` David Miller
2010-07-09 19:11 Ben Hutchings
2010-07-09 21:08 ` Eric Dumazet
2010-07-10 1:01 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.