* [PATCH net-next 0/9] net: introduce u64_stats_t
@ 2019-11-08 0:27 Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 1/9] net: provide dev_lstats_read() helper Eric Dumazet
` (9 more replies)
0 siblings, 10 replies; 11+ messages in thread
From: Eric Dumazet @ 2019-11-08 0:27 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
KCSAN found a data-race in per-cpu u64 stats accounting.
(The stack traces are included in the 8th patch :
tun: switch to u64_stats_t)
This patch series first consolidate code in five patches.
Then the last three patches address the data-race resolution.
Eric Dumazet (9):
net: provide dev_lstats_read() helper
net: provide dev_lstats_add() helper
net: nlmon: use standard dev_lstats_add() and dev_lstats_read()
veth: use standard dev_lstats_add() and dev_lstats_read()
vsockmon: use standard dev_lstats_add() and dev_lstats_read()
net: dummy: use standard dev_lstats_add() and dev_lstats_read()
u64_stats: provide u64_stats_t type
tun: switch to u64_stats_t
net: use u64_stats_t in struct pcpu_lstats
drivers/net/dummy.c | 36 ++++--------------------
drivers/net/loopback.c | 38 +++++++++++++------------
drivers/net/nlmon.c | 28 ++-----------------
drivers/net/tun.c | 32 ++++++++++-----------
drivers/net/veth.c | 43 ++++++++--------------------
drivers/net/vsockmon.c | 31 ++-------------------
include/linux/netdevice.h | 16 +++++++++--
include/linux/u64_stats_sync.h | 51 +++++++++++++++++++++++++++++++---
8 files changed, 118 insertions(+), 157 deletions(-)
--
2.24.0.432.g9d3f5f5b63-goog
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next 1/9] net: provide dev_lstats_read() helper
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
@ 2019-11-08 0:27 ` Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 2/9] net: provide dev_lstats_add() helper Eric Dumazet
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Dumazet @ 2019-11-08 0:27 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
Many network drivers use hand-coded implementation of the same thing,
let's factorize things so that u64_stats_t adoption is done once.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/loopback.c | 24 +++++++++++++++++-------
include/linux/netdevice.h | 2 ++
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 14545a8797a8a2c9b61abc96cbdb1a3542481745..92336ac4c5e68f63b814d6a70e7361b8954a91cf 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -99,13 +99,13 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
return NETDEV_TX_OK;
}
-static void loopback_get_stats64(struct net_device *dev,
- struct rtnl_link_stats64 *stats)
+void dev_lstats_read(struct net_device *dev, u64 *packets, u64 *bytes)
{
- u64 bytes = 0;
- u64 packets = 0;
int i;
+ *packets = 0;
+ *bytes = 0;
+
for_each_possible_cpu(i) {
const struct pcpu_lstats *lb_stats;
u64 tbytes, tpackets;
@@ -114,12 +114,22 @@ static void loopback_get_stats64(struct net_device *dev,
lb_stats = per_cpu_ptr(dev->lstats, i);
do {
start = u64_stats_fetch_begin_irq(&lb_stats->syncp);
- tbytes = lb_stats->bytes;
tpackets = lb_stats->packets;
+ tbytes = lb_stats->bytes;
} while (u64_stats_fetch_retry_irq(&lb_stats->syncp, start));
- bytes += tbytes;
- packets += tpackets;
+ *bytes += tbytes;
+ *packets += tpackets;
}
+}
+EXPORT_SYMBOL(dev_lstats_read);
+
+static void loopback_get_stats64(struct net_device *dev,
+ struct rtnl_link_stats64 *stats)
+{
+ u64 packets, bytes;
+
+ dev_lstats_read(dev, &packets, &bytes);
+
stats->rx_packets = packets;
stats->tx_packets = packets;
stats->rx_bytes = bytes;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 1f140a6b66dfbd1dd86d2d6b0e0adb812de4b243..75561992c31f7c32f5a50e3879bafb5a54bc5fa3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2401,6 +2401,8 @@ struct pcpu_lstats {
struct u64_stats_sync syncp;
} __aligned(2 * sizeof(u64));
+void dev_lstats_read(struct net_device *dev, u64 *packets, u64 *bytes);
+
#define __netdev_alloc_pcpu_stats(type, gfp) \
({ \
typeof(type) __percpu *pcpu_stats = alloc_percpu_gfp(type, gfp);\
--
2.24.0.432.g9d3f5f5b63-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 2/9] net: provide dev_lstats_add() helper
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 1/9] net: provide dev_lstats_read() helper Eric Dumazet
@ 2019-11-08 0:27 ` Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 3/9] net: nlmon: use standard dev_lstats_add() and dev_lstats_read() Eric Dumazet
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Dumazet @ 2019-11-08 0:27 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
Many network drivers need it and hand-coded the same function.
In order to ease u64_stats_t adoption, it is time to factorize.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/loopback.c | 12 ++----------
include/linux/netdevice.h | 10 ++++++++++
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 92336ac4c5e68f63b814d6a70e7361b8954a91cf..47ad2478b9f350f8bf3b103bd2a9a956379c75fa 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -68,7 +68,6 @@ EXPORT_SYMBOL(blackhole_netdev);
static netdev_tx_t loopback_xmit(struct sk_buff *skb,
struct net_device *dev)
{
- struct pcpu_lstats *lb_stats;
int len;
skb_tx_timestamp(skb);
@@ -85,16 +84,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
skb->protocol = eth_type_trans(skb, dev);
- /* it's OK to use per_cpu_ptr() because BHs are off */
- lb_stats = this_cpu_ptr(dev->lstats);
-
len = skb->len;
- if (likely(netif_rx(skb) == NET_RX_SUCCESS)) {
- u64_stats_update_begin(&lb_stats->syncp);
- lb_stats->bytes += len;
- lb_stats->packets++;
- u64_stats_update_end(&lb_stats->syncp);
- }
+ if (likely(netif_rx(skb) == NET_RX_SUCCESS))
+ dev_lstats_add(dev, len);
return NETDEV_TX_OK;
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 75561992c31f7c32f5a50e3879bafb5a54bc5fa3..461a36220cf46d62114efac0c4fb2b7b9a2ee386 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2403,6 +2403,16 @@ struct pcpu_lstats {
void dev_lstats_read(struct net_device *dev, u64 *packets, u64 *bytes);
+static inline void dev_lstats_add(struct net_device *dev, unsigned int len)
+{
+ struct pcpu_lstats *lstats = this_cpu_ptr(dev->lstats);
+
+ u64_stats_update_begin(&lstats->syncp);
+ lstats->bytes += len;
+ lstats->packets++;
+ u64_stats_update_end(&lstats->syncp);
+}
+
#define __netdev_alloc_pcpu_stats(type, gfp) \
({ \
typeof(type) __percpu *pcpu_stats = alloc_percpu_gfp(type, gfp);\
--
2.24.0.432.g9d3f5f5b63-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 3/9] net: nlmon: use standard dev_lstats_add() and dev_lstats_read()
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 1/9] net: provide dev_lstats_read() helper Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 2/9] net: provide dev_lstats_add() helper Eric Dumazet
@ 2019-11-08 0:27 ` Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 4/9] veth: " Eric Dumazet
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Dumazet @ 2019-11-08 0:27 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
No need to hand-code the exact same functions.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/nlmon.c | 28 +++-------------------------
1 file changed, 3 insertions(+), 25 deletions(-)
diff --git a/drivers/net/nlmon.c b/drivers/net/nlmon.c
index 68771b2f351a228860cdfbc7ab3f028665b2590e..afb119f383252cc4c78e5456d12b7066eb26953f 100644
--- a/drivers/net/nlmon.c
+++ b/drivers/net/nlmon.c
@@ -9,13 +9,7 @@
static netdev_tx_t nlmon_xmit(struct sk_buff *skb, struct net_device *dev)
{
- int len = skb->len;
- struct pcpu_lstats *stats = this_cpu_ptr(dev->lstats);
-
- u64_stats_update_begin(&stats->syncp);
- stats->bytes += len;
- stats->packets++;
- u64_stats_update_end(&stats->syncp);
+ dev_lstats_add(dev, skb->len);
dev_kfree_skb(skb);
@@ -56,25 +50,9 @@ static int nlmon_close(struct net_device *dev)
static void
nlmon_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
{
- int i;
- u64 bytes = 0, packets = 0;
-
- for_each_possible_cpu(i) {
- const struct pcpu_lstats *nl_stats;
- u64 tbytes, tpackets;
- unsigned int start;
-
- nl_stats = per_cpu_ptr(dev->lstats, i);
-
- do {
- start = u64_stats_fetch_begin_irq(&nl_stats->syncp);
- tbytes = nl_stats->bytes;
- tpackets = nl_stats->packets;
- } while (u64_stats_fetch_retry_irq(&nl_stats->syncp, start));
+ u64 packets, bytes;
- packets += tpackets;
- bytes += tbytes;
- }
+ dev_lstats_read(dev, &packets, &bytes);
stats->rx_packets = packets;
stats->tx_packets = 0;
--
2.24.0.432.g9d3f5f5b63-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 4/9] veth: use standard dev_lstats_add() and dev_lstats_read()
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
` (2 preceding siblings ...)
2019-11-08 0:27 ` [PATCH net-next 3/9] net: nlmon: use standard dev_lstats_add() and dev_lstats_read() Eric Dumazet
@ 2019-11-08 0:27 ` Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 5/9] vsockmon: " Eric Dumazet
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Dumazet @ 2019-11-08 0:27 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
This cleanup will ease u64_stats_t adoption in a single location.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/veth.c | 43 +++++++++++--------------------------------
1 file changed, 11 insertions(+), 32 deletions(-)
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 9f3c839f9e5f2cda2f103f943f94badc09691a2b..a552df37a347c72fed84909c45188e5dd1201df7 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -260,14 +260,8 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
skb_tx_timestamp(skb);
if (likely(veth_forward_skb(rcv, skb, rq, rcv_xdp) == NET_RX_SUCCESS)) {
- if (!rcv_xdp) {
- struct pcpu_lstats *stats = this_cpu_ptr(dev->lstats);
-
- u64_stats_update_begin(&stats->syncp);
- stats->bytes += length;
- stats->packets++;
- u64_stats_update_end(&stats->syncp);
- }
+ if (!rcv_xdp)
+ dev_lstats_add(dev, length);
} else {
drop:
atomic64_inc(&priv->dropped);
@@ -281,26 +275,11 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_OK;
}
-static u64 veth_stats_tx(struct pcpu_lstats *result, struct net_device *dev)
+static u64 veth_stats_tx(struct net_device *dev, u64 *packets, u64 *bytes)
{
struct veth_priv *priv = netdev_priv(dev);
- int cpu;
-
- result->packets = 0;
- result->bytes = 0;
- for_each_possible_cpu(cpu) {
- struct pcpu_lstats *stats = per_cpu_ptr(dev->lstats, cpu);
- u64 packets, bytes;
- unsigned int start;
- do {
- start = u64_stats_fetch_begin_irq(&stats->syncp);
- packets = stats->packets;
- bytes = stats->bytes;
- } while (u64_stats_fetch_retry_irq(&stats->syncp, start));
- result->packets += packets;
- result->bytes += bytes;
- }
+ dev_lstats_read(dev, packets, bytes);
return atomic64_read(&priv->dropped);
}
@@ -335,11 +314,11 @@ static void veth_get_stats64(struct net_device *dev,
struct veth_priv *priv = netdev_priv(dev);
struct net_device *peer;
struct veth_rq_stats rx;
- struct pcpu_lstats tx;
+ u64 packets, bytes;
- tot->tx_dropped = veth_stats_tx(&tx, dev);
- tot->tx_bytes = tx.bytes;
- tot->tx_packets = tx.packets;
+ tot->tx_dropped = veth_stats_tx(dev, &packets, &bytes);
+ tot->tx_bytes = bytes;
+ tot->tx_packets = packets;
veth_stats_rx(&rx, dev);
tot->rx_dropped = rx.xdp_drops;
@@ -349,9 +328,9 @@ static void veth_get_stats64(struct net_device *dev,
rcu_read_lock();
peer = rcu_dereference(priv->peer);
if (peer) {
- tot->rx_dropped += veth_stats_tx(&tx, peer);
- tot->rx_bytes += tx.bytes;
- tot->rx_packets += tx.packets;
+ tot->rx_dropped += veth_stats_tx(peer, &packets, &bytes);
+ tot->rx_bytes += bytes;
+ tot->rx_packets += packets;
veth_stats_rx(&rx, peer);
tot->tx_bytes += rx.xdp_bytes;
--
2.24.0.432.g9d3f5f5b63-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 5/9] vsockmon: use standard dev_lstats_add() and dev_lstats_read()
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
` (3 preceding siblings ...)
2019-11-08 0:27 ` [PATCH net-next 4/9] veth: " Eric Dumazet
@ 2019-11-08 0:27 ` Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 6/9] net: dummy: " Eric Dumazet
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Dumazet @ 2019-11-08 0:27 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
This cleanup will ease u64_stats_t adoption in a single location.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/vsockmon.c | 31 ++-----------------------------
1 file changed, 2 insertions(+), 29 deletions(-)
diff --git a/drivers/net/vsockmon.c b/drivers/net/vsockmon.c
index 14e324b846171437bca40ce197c8588e20fc036b..e8563acf98e8d8fef019f89d814e28e979b8ba2c 100644
--- a/drivers/net/vsockmon.c
+++ b/drivers/net/vsockmon.c
@@ -47,13 +47,7 @@ static int vsockmon_close(struct net_device *dev)
static netdev_tx_t vsockmon_xmit(struct sk_buff *skb, struct net_device *dev)
{
- int len = skb->len;
- struct pcpu_lstats *stats = this_cpu_ptr(dev->lstats);
-
- u64_stats_update_begin(&stats->syncp);
- stats->bytes += len;
- stats->packets++;
- u64_stats_update_end(&stats->syncp);
+ dev_lstats_add(dev, skb->len);
dev_kfree_skb(skb);
@@ -63,30 +57,9 @@ static netdev_tx_t vsockmon_xmit(struct sk_buff *skb, struct net_device *dev)
static void
vsockmon_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
{
- int i;
- u64 bytes = 0, packets = 0;
-
- for_each_possible_cpu(i) {
- const struct pcpu_lstats *vstats;
- u64 tbytes, tpackets;
- unsigned int start;
-
- vstats = per_cpu_ptr(dev->lstats, i);
+ dev_lstats_read(dev, &stats->rx_packets, &stats->rx_bytes);
- do {
- start = u64_stats_fetch_begin_irq(&vstats->syncp);
- tbytes = vstats->bytes;
- tpackets = vstats->packets;
- } while (u64_stats_fetch_retry_irq(&vstats->syncp, start));
-
- packets += tpackets;
- bytes += tbytes;
- }
-
- stats->rx_packets = packets;
stats->tx_packets = 0;
-
- stats->rx_bytes = bytes;
stats->tx_bytes = 0;
}
--
2.24.0.432.g9d3f5f5b63-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 6/9] net: dummy: use standard dev_lstats_add() and dev_lstats_read()
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
` (4 preceding siblings ...)
2019-11-08 0:27 ` [PATCH net-next 5/9] vsockmon: " Eric Dumazet
@ 2019-11-08 0:27 ` Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 7/9] u64_stats: provide u64_stats_t type Eric Dumazet
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Dumazet @ 2019-11-08 0:27 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
This driver can simply use the common infrastructure instead
of duplicating it.
This cleanup will ease u64_stats_t adoption in a single location.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/dummy.c | 36 +++++-------------------------------
1 file changed, 5 insertions(+), 31 deletions(-)
diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index 54e4d8b07f0e054b2fb83f4ea05063295a544f5b..3031a5fc54276009269992061f3bd11e02711927 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -51,41 +51,15 @@ static void set_multicast_list(struct net_device *dev)
{
}
-struct pcpu_dstats {
- u64 tx_packets;
- u64 tx_bytes;
- struct u64_stats_sync syncp;
-};
-
static void dummy_get_stats64(struct net_device *dev,
struct rtnl_link_stats64 *stats)
{
- int i;
-
- for_each_possible_cpu(i) {
- const struct pcpu_dstats *dstats;
- u64 tbytes, tpackets;
- unsigned int start;
-
- dstats = per_cpu_ptr(dev->dstats, i);
- do {
- start = u64_stats_fetch_begin_irq(&dstats->syncp);
- tbytes = dstats->tx_bytes;
- tpackets = dstats->tx_packets;
- } while (u64_stats_fetch_retry_irq(&dstats->syncp, start));
- stats->tx_bytes += tbytes;
- stats->tx_packets += tpackets;
- }
+ dev_lstats_read(dev, &stats->tx_packets, &stats->tx_bytes);
}
static netdev_tx_t dummy_xmit(struct sk_buff *skb, struct net_device *dev)
{
- struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
-
- u64_stats_update_begin(&dstats->syncp);
- dstats->tx_packets++;
- dstats->tx_bytes += skb->len;
- u64_stats_update_end(&dstats->syncp);
+ dev_lstats_add(dev, skb->len);
skb_tx_timestamp(skb);
dev_kfree_skb(skb);
@@ -94,8 +68,8 @@ static netdev_tx_t dummy_xmit(struct sk_buff *skb, struct net_device *dev)
static int dummy_dev_init(struct net_device *dev)
{
- dev->dstats = netdev_alloc_pcpu_stats(struct pcpu_dstats);
- if (!dev->dstats)
+ dev->lstats = netdev_alloc_pcpu_stats(struct pcpu_lstats);
+ if (!dev->lstats)
return -ENOMEM;
return 0;
@@ -103,7 +77,7 @@ static int dummy_dev_init(struct net_device *dev)
static void dummy_dev_uninit(struct net_device *dev)
{
- free_percpu(dev->dstats);
+ free_percpu(dev->lstats);
}
static int dummy_change_carrier(struct net_device *dev, bool new_carrier)
--
2.24.0.432.g9d3f5f5b63-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 7/9] u64_stats: provide u64_stats_t type
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
` (5 preceding siblings ...)
2019-11-08 0:27 ` [PATCH net-next 6/9] net: dummy: " Eric Dumazet
@ 2019-11-08 0:27 ` Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 8/9] tun: switch to u64_stats_t Eric Dumazet
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Eric Dumazet @ 2019-11-08 0:27 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
On 64bit arches, struct u64_stats_sync is empty and provides
no help against load/store tearing.
Using READ_ONCE()/WRITE_ONCE() would be needed.
But the update side would be slightly more expensive.
local64_t was defined so that we could use regular adds
in a manner which is atomic wrt IRQs.
However the u64_stats infra means we do not have to use
local64_t on 32bit arches since the syncp provides the needed
protection.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
include/linux/u64_stats_sync.h | 51 +++++++++++++++++++++++++++++++---
1 file changed, 47 insertions(+), 4 deletions(-)
diff --git a/include/linux/u64_stats_sync.h b/include/linux/u64_stats_sync.h
index a27604f99ed044d3492395c0328f41538234d7dc..9de5c10293f593fac6f64458d78f413c3c9fe26c 100644
--- a/include/linux/u64_stats_sync.h
+++ b/include/linux/u64_stats_sync.h
@@ -40,8 +40,8 @@
* spin_lock_bh(...) or other synchronization to get exclusive access
* ...
* u64_stats_update_begin(&stats->syncp);
- * stats->bytes64 += len; // non atomic operation
- * stats->packets64++; // non atomic operation
+ * u64_stats_add(&stats->bytes64, len); // non atomic operation
+ * u64_stats_inc(&stats->packets64); // non atomic operation
* u64_stats_update_end(&stats->syncp);
*
* While a consumer (reader) should use following template to get consistent
@@ -52,8 +52,8 @@
*
* do {
* start = u64_stats_fetch_begin(&stats->syncp);
- * tbytes = stats->bytes64; // non atomic operation
- * tpackets = stats->packets64; // non atomic operation
+ * tbytes = u64_stats_read(&stats->bytes64); // non atomic operation
+ * tpackets = u64_stats_read(&stats->packets64); // non atomic operation
* } while (u64_stats_fetch_retry(&stats->syncp, start));
*
*
@@ -68,6 +68,49 @@ struct u64_stats_sync {
#endif
};
+#if BITS_PER_LONG == 64
+#include <asm/local64.h>
+
+typedef struct {
+ local64_t v;
+} u64_stats_t ;
+
+static inline u64 u64_stats_read(const u64_stats_t *p)
+{
+ return local64_read(&p->v);
+}
+
+static inline void u64_stats_add(u64_stats_t *p, unsigned long val)
+{
+ local64_add(val, &p->v);
+}
+
+static inline void u64_stats_inc(u64_stats_t *p)
+{
+ local64_inc(&p->v);
+}
+
+#else
+
+typedef struct {
+ u64 v;
+} u64_stats_t;
+
+static inline u64 u64_stats_read(const u64_stats_t *p)
+{
+ return p->v;
+}
+
+static inline void u64_stats_add(u64_stats_t *p, unsigned long val)
+{
+ p->v += val;
+}
+
+static inline void u64_stats_inc(u64_stats_t *p)
+{
+ p->v++;
+}
+#endif
static inline void u64_stats_init(struct u64_stats_sync *syncp)
{
--
2.24.0.432.g9d3f5f5b63-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 8/9] tun: switch to u64_stats_t
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
` (6 preceding siblings ...)
2019-11-08 0:27 ` [PATCH net-next 7/9] u64_stats: provide u64_stats_t type Eric Dumazet
@ 2019-11-08 0:27 ` Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 9/9] net: use u64_stats_t in struct pcpu_lstats Eric Dumazet
2019-11-08 4:04 ` [PATCH net-next 0/9] net: introduce u64_stats_t David Miller
9 siblings, 0 replies; 11+ messages in thread
From: Eric Dumazet @ 2019-11-08 0:27 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
In order to fix this data-race found by KCSAN [1],
switch to u64_stats_t helpers. They provide all
the needed annotations, without adding extra cost.
[1]
BUG: KCSAN: data-race in tun_get_user / tun_net_get_stats64
read to 0xffffe8ffffd8aca8 of 8 bytes by task 4882 on cpu 0:
tun_net_get_stats64+0x9b/0x230 drivers/net/tun.c:1171
dev_get_stats+0x89/0x1e0 net/core/dev.c:9103
rtnl_fill_stats+0x56/0x370 net/core/rtnetlink.c:1177
rtnl_fill_ifinfo+0xd3b/0x2100 net/core/rtnetlink.c:1667
rtmsg_ifinfo_build_skb+0xb0/0x150 net/core/rtnetlink.c:3472
rtmsg_ifinfo_event.part.0+0x4e/0xb0 net/core/rtnetlink.c:3504
rtmsg_ifinfo_event net/core/rtnetlink.c:3515 [inline]
rtmsg_ifinfo+0x85/0x90 net/core/rtnetlink.c:3513
__dev_notify_flags+0x18b/0x200 net/core/dev.c:7649
dev_change_flags+0xb8/0xe0 net/core/dev.c:7691
dev_ifsioc+0x201/0x6a0 net/core/dev_ioctl.c:237
dev_ioctl+0x149/0x660 net/core/dev_ioctl.c:489
sock_do_ioctl+0xdb/0x230 net/socket.c:1061
sock_ioctl+0x3a3/0x5e0 net/socket.c:1189
vfs_ioctl fs/ioctl.c:46 [inline]
file_ioctl fs/ioctl.c:509 [inline]
do_vfs_ioctl+0x991/0xc60 fs/ioctl.c:696
write to 0xffffe8ffffd8aca8 of 8 bytes by task 4883 on cpu 1:
tun_get_user+0x1d94/0x2ba0 drivers/net/tun.c:2002
tun_chr_write_iter+0x79/0xd0 drivers/net/tun.c:2022
call_write_iter include/linux/fs.h:1895 [inline]
new_sync_write+0x388/0x4a0 fs/read_write.c:483
__vfs_write+0xb1/0xc0 fs/read_write.c:496
__kernel_write+0xb8/0x240 fs/read_write.c:515
write_pipe_buf+0xb6/0xf0 fs/splice.c:794
splice_from_pipe_feed fs/splice.c:500 [inline]
__splice_from_pipe+0x248/0x480 fs/splice.c:624
splice_from_pipe+0xbb/0x100 fs/splice.c:659
default_file_splice_write+0x45/0x90 fs/splice.c:806
do_splice_from fs/splice.c:848 [inline]
direct_splice_actor+0xa0/0xc0 fs/splice.c:1020
splice_direct_to_actor+0x215/0x510 fs/splice.c:975
do_splice_direct+0x161/0x1e0 fs/splice.c:1063
do_sendfile+0x384/0x7f0 fs/read_write.c:1464
Reported by Kernel Concurrency Sanitizer on:
CPU: 1 PID: 4883 Comm: syz-executor.1 Not tainted 5.4.0-rc3+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/tun.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index dab6cccfeb521f53c9c9958bc4d8e0001d235506..dcb63f1f9110762191a2c658ce7af7b5b474d868 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -136,10 +136,10 @@ struct tap_filter {
#define TUN_FLOW_EXPIRE (3 * HZ)
struct tun_pcpu_stats {
- u64 rx_packets;
- u64 rx_bytes;
- u64 tx_packets;
- u64 tx_bytes;
+ u64_stats_t rx_packets;
+ u64_stats_t rx_bytes;
+ u64_stats_t tx_packets;
+ u64_stats_t tx_bytes;
struct u64_stats_sync syncp;
u32 rx_dropped;
u32 tx_dropped;
@@ -1167,10 +1167,10 @@ tun_net_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
p = per_cpu_ptr(tun->pcpu_stats, i);
do {
start = u64_stats_fetch_begin(&p->syncp);
- rxpackets = p->rx_packets;
- rxbytes = p->rx_bytes;
- txpackets = p->tx_packets;
- txbytes = p->tx_bytes;
+ rxpackets = u64_stats_read(&p->rx_packets);
+ rxbytes = u64_stats_read(&p->rx_bytes);
+ txpackets = u64_stats_read(&p->tx_packets);
+ txbytes = u64_stats_read(&p->tx_bytes);
} while (u64_stats_fetch_retry(&p->syncp, start));
stats->rx_packets += rxpackets;
@@ -1998,8 +1998,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
stats = get_cpu_ptr(tun->pcpu_stats);
u64_stats_update_begin(&stats->syncp);
- stats->rx_packets++;
- stats->rx_bytes += len;
+ u64_stats_inc(&stats->rx_packets);
+ u64_stats_add(&stats->rx_bytes, len);
u64_stats_update_end(&stats->syncp);
put_cpu_ptr(stats);
@@ -2052,8 +2052,8 @@ static ssize_t tun_put_user_xdp(struct tun_struct *tun,
stats = get_cpu_ptr(tun->pcpu_stats);
u64_stats_update_begin(&stats->syncp);
- stats->tx_packets++;
- stats->tx_bytes += ret;
+ u64_stats_inc(&stats->tx_packets);
+ u64_stats_add(&stats->tx_bytes, ret);
u64_stats_update_end(&stats->syncp);
put_cpu_ptr(tun->pcpu_stats);
@@ -2147,8 +2147,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
/* caller is in process context, */
stats = get_cpu_ptr(tun->pcpu_stats);
u64_stats_update_begin(&stats->syncp);
- stats->tx_packets++;
- stats->tx_bytes += skb->len + vlan_hlen;
+ u64_stats_inc(&stats->tx_packets);
+ u64_stats_add(&stats->tx_bytes, skb->len + vlan_hlen);
u64_stats_update_end(&stats->syncp);
put_cpu_ptr(tun->pcpu_stats);
@@ -2511,8 +2511,8 @@ static int tun_xdp_one(struct tun_struct *tun,
*/
stats = this_cpu_ptr(tun->pcpu_stats);
u64_stats_update_begin(&stats->syncp);
- stats->rx_packets++;
- stats->rx_bytes += datasize;
+ u64_stats_inc(&stats->rx_packets);
+ u64_stats_add(&stats->rx_bytes, datasize);
u64_stats_update_end(&stats->syncp);
if (rxhash)
--
2.24.0.432.g9d3f5f5b63-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH net-next 9/9] net: use u64_stats_t in struct pcpu_lstats
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
` (7 preceding siblings ...)
2019-11-08 0:27 ` [PATCH net-next 8/9] tun: switch to u64_stats_t Eric Dumazet
@ 2019-11-08 0:27 ` Eric Dumazet
2019-11-08 4:04 ` [PATCH net-next 0/9] net: introduce u64_stats_t David Miller
9 siblings, 0 replies; 11+ messages in thread
From: Eric Dumazet @ 2019-11-08 0:27 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet
In order to fix the data-race found by KCSAN, we
can use the new u64_stats_t type and its accessors instead
of plain u64 fields. This will still generate optimal code
for both 32 and 64 bit platforms.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/net/loopback.c | 4 ++--
include/linux/netdevice.h | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 47ad2478b9f350f8bf3b103bd2a9a956379c75fa..a1c77cc0041657de79b562c84408acabf9e8b99b 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -106,8 +106,8 @@ void dev_lstats_read(struct net_device *dev, u64 *packets, u64 *bytes)
lb_stats = per_cpu_ptr(dev->lstats, i);
do {
start = u64_stats_fetch_begin_irq(&lb_stats->syncp);
- tpackets = lb_stats->packets;
- tbytes = lb_stats->bytes;
+ tpackets = u64_stats_read(&lb_stats->packets);
+ tbytes = u64_stats_read(&lb_stats->bytes);
} while (u64_stats_fetch_retry_irq(&lb_stats->syncp, start));
*bytes += tbytes;
*packets += tpackets;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 461a36220cf46d62114efac0c4fb2b7b9a2ee386..f857f01234f774d70d8c2425498e1fbc9909d88e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2396,8 +2396,8 @@ struct pcpu_sw_netstats {
} __aligned(4 * sizeof(u64));
struct pcpu_lstats {
- u64 packets;
- u64 bytes;
+ u64_stats_t packets;
+ u64_stats_t bytes;
struct u64_stats_sync syncp;
} __aligned(2 * sizeof(u64));
@@ -2408,8 +2408,8 @@ static inline void dev_lstats_add(struct net_device *dev, unsigned int len)
struct pcpu_lstats *lstats = this_cpu_ptr(dev->lstats);
u64_stats_update_begin(&lstats->syncp);
- lstats->bytes += len;
- lstats->packets++;
+ u64_stats_add(&lstats->bytes, len);
+ u64_stats_inc(&lstats->packets);
u64_stats_update_end(&lstats->syncp);
}
--
2.24.0.432.g9d3f5f5b63-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 0/9] net: introduce u64_stats_t
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
` (8 preceding siblings ...)
2019-11-08 0:27 ` [PATCH net-next 9/9] net: use u64_stats_t in struct pcpu_lstats Eric Dumazet
@ 2019-11-08 4:04 ` David Miller
9 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2019-11-08 4:04 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet
From: Eric Dumazet <edumazet@google.com>
Date: Thu, 7 Nov 2019 16:27:13 -0800
> KCSAN found a data-race in per-cpu u64 stats accounting.
>
> (The stack traces are included in the 8th patch :
> tun: switch to u64_stats_t)
>
> This patch series first consolidate code in five patches.
> Then the last three patches address the data-race resolution.
Series applied, thanks Eric.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-11-08 4:04 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 0:27 [PATCH net-next 0/9] net: introduce u64_stats_t Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 1/9] net: provide dev_lstats_read() helper Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 2/9] net: provide dev_lstats_add() helper Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 3/9] net: nlmon: use standard dev_lstats_add() and dev_lstats_read() Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 4/9] veth: " Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 5/9] vsockmon: " Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 6/9] net: dummy: " Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 7/9] u64_stats: provide u64_stats_t type Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 8/9] tun: switch to u64_stats_t Eric Dumazet
2019-11-08 0:27 ` [PATCH net-next 9/9] net: use u64_stats_t in struct pcpu_lstats Eric Dumazet
2019-11-08 4:04 ` [PATCH net-next 0/9] net: introduce u64_stats_t 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.