[PATCH net-next v3 0/3] net: core: Unify dstats with tstats and lstats, implement generic dstats collection

[PATCH net-next v3 0/3] net: core: Unify dstats with tstats and lstats, implement generic dstats collection

[Jeremy Kerr]

The struct pcpu_dstats ("dstats") has a few variations from the other
two stats types (struct pcpu_sw_netstats and struct pcpu_lstats), and
doesn't have generic helpers for collecting the per-cpu stats into a
struct rtnl_link_stats64.

This change unifies dstats with the other types, adds a collection
implementation to the core, and updates the single driver (vrf) to use
this generic implementation.

Of course, questions/comments/etc are most welcome!

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
v3:
- rather than exposing helpers, perform dstat collection implicitly when
  type == NETDEV_PCPU_STAT_DSTAT
- Link to v2:
  https://lore.kernel.org/r/20240605-dstats-v2-0-7fae03f813f3@codeconstruct.com.au

---
v2:
- use correct percpu var in dev_fetch_dstats
- use correct accessor in vfr rx drop accounting
- v1: https://lore.kernel.org/r/20240605-dstats-v1-0-1024396e1670@codeconstruct.com.au

---
Jeremy Kerr (3):
      net: core,vrf: Change pcpu_dstat fields to u64_stats_t
      net: core: Implement dstats-type stats collections
      net: vrf: move to generic dstat helpers

 drivers/net/vrf.c         | 56 ++++++++++++++---------------------------------
 include/linux/netdevice.h | 12 +++++-----
 net/core/dev.c            | 50 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 45 deletions(-)
---
base-commit: 32f88d65f01bf6f45476d7edbe675e44fb9e1d58
change-id: 20240605-dstats-b6e08c318555

Best regards,
-- 
Jeremy Kerr <jk@codeconstruct.com.au>

[PATCH net-next v3 1/3] net: core,vrf: Change pcpu_dstat fields to u64_stats_t

[Jeremy Kerr]

The pcpu_sw_netstats and pcpu_lstats structs both contain a set of
u64_stats_t fields for individual stats, but pcpu_dstats uses u64s
instead.

Make this consistent by using u64_stats_t across all stats types.

The per-cpu dstats are only used by the vrf driver at present, so update
that driver as part of this change.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

---
v2:
 - use proper accessor in rx drop accounting
---
 drivers/net/vrf.c         | 38 ++++++++++++++++++++++----------------
 include/linux/netdevice.h | 12 ++++++------
 2 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 3a252ac5dd28..5018831b2a79 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -126,8 +126,8 @@ static void vrf_rx_stats(struct net_device *dev, int len)
 	struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
 
 	u64_stats_update_begin(&dstats->syncp);
-	dstats->rx_packets++;
-	dstats->rx_bytes += len;
+	u64_stats_inc(&dstats->rx_packets);
+	u64_stats_add(&dstats->rx_bytes, len);
 	u64_stats_update_end(&dstats->syncp);
 }
 
@@ -150,11 +150,11 @@ static void vrf_get_stats64(struct net_device *dev,
 		dstats = per_cpu_ptr(dev->dstats, i);
 		do {
 			start = u64_stats_fetch_begin(&dstats->syncp);
-			tbytes = dstats->tx_bytes;
-			tpkts = dstats->tx_packets;
-			tdrops = dstats->tx_drops;
-			rbytes = dstats->rx_bytes;
-			rpkts = dstats->rx_packets;
+			tbytes = u64_stats_read(&dstats->tx_bytes);
+			tpkts = u64_stats_read(&dstats->tx_packets);
+			tdrops = u64_stats_read(&dstats->tx_drops);
+			rbytes = u64_stats_read(&dstats->rx_bytes);
+			rpkts = u64_stats_read(&dstats->rx_packets);
 		} while (u64_stats_fetch_retry(&dstats->syncp, start));
 		stats->tx_bytes += tbytes;
 		stats->tx_packets += tpkts;
@@ -408,10 +408,15 @@ static int vrf_local_xmit(struct sk_buff *skb, struct net_device *dev,
 
 	skb->protocol = eth_type_trans(skb, dev);
 
-	if (likely(__netif_rx(skb) == NET_RX_SUCCESS))
+	if (likely(__netif_rx(skb) == NET_RX_SUCCESS)) {
 		vrf_rx_stats(dev, len);
-	else
-		this_cpu_inc(dev->dstats->rx_drops);
+	} else {
+		struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
+
+		u64_stats_update_begin(&dstats->syncp);
+		u64_stats_inc(&dstats->rx_drops);
+		u64_stats_update_end(&dstats->syncp);
+	}
 
 	return NETDEV_TX_OK;
 }
@@ -599,19 +604,20 @@ static netdev_tx_t is_ip_tx_frame(struct sk_buff *skb, struct net_device *dev)
 
 static netdev_tx_t vrf_xmit(struct sk_buff *skb, struct net_device *dev)
 {
+	struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
+
 	int len = skb->len;
 	netdev_tx_t ret = is_ip_tx_frame(skb, dev);
 
+	u64_stats_update_begin(&dstats->syncp);
 	if (likely(ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)) {
-		struct pcpu_dstats *dstats = this_cpu_ptr(dev->dstats);
 
-		u64_stats_update_begin(&dstats->syncp);
-		dstats->tx_packets++;
-		dstats->tx_bytes += len;
-		u64_stats_update_end(&dstats->syncp);
+		u64_stats_inc(&dstats->tx_packets);
+		u64_stats_add(&dstats->tx_bytes, len);
 	} else {
-		this_cpu_inc(dev->dstats->tx_drops);
+		u64_stats_inc(&dstats->tx_drops);
 	}
+	u64_stats_update_end(&dstats->syncp);
 
 	return ret;
 }
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d20c6c99eb88..f148a01dd1d1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2731,12 +2731,12 @@ struct pcpu_sw_netstats {
 } __aligned(4 * sizeof(u64));
 
 struct pcpu_dstats {
-	u64			rx_packets;
-	u64			rx_bytes;
-	u64			rx_drops;
-	u64			tx_packets;
-	u64			tx_bytes;
-	u64			tx_drops;
+	u64_stats_t		rx_packets;
+	u64_stats_t		rx_bytes;
+	u64_stats_t		rx_drops;
+	u64_stats_t		tx_packets;
+	u64_stats_t		tx_bytes;
+	u64_stats_t		tx_drops;
 	struct u64_stats_sync	syncp;
 } __aligned(8 * sizeof(u64));
 

-- 
2.39.2

Re: [PATCH net-next v3 1/3] net: core,vrf: Change pcpu_dstat fields to u64_stats_t

[Simon Horman]

On Fri, Jun 07, 2024 at 06:25:24PM +0800, Jeremy Kerr wrote:
> The pcpu_sw_netstats and pcpu_lstats structs both contain a set of
> u64_stats_t fields for individual stats, but pcpu_dstats uses u64s
> instead.
> 
> Make this consistent by using u64_stats_t across all stats types.
> 
> The per-cpu dstats are only used by the vrf driver at present, so update
> that driver as part of this change.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> 
> ---
> v2:
>  - use proper accessor in rx drop accounting

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net-next v3 2/3] net: core: Implement dstats-type stats collections

[Jeremy Kerr]

We currently have dev_get_tstats64() for collecting per-cpu stats of
type pcpu_sw_netstats ("tstats"). However, tstats doesn't allow for
accounting tx/rx drops. We do have a stats variant that does have stats
for dropped packets: struct pcpu_dstats, but there are no core helpers
for using those stats.

The VRF driver uses dstats, by providing its own collation/fetch
functions to do so.

This change adds a common implementation for dstats-type collection,
used when pcpu_stat_type == NETDEV_PCPU_STAT_DSTAT. This is based on the
VRF driver's existing stats collator (plus the unused tx_drops stat from
there). We will switch the VRF driver to use this in the next change.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

---
v3:
 - As suggested by Jakub: don't expose dev_get_dstats64 as a helper, but
   invoke automatically for NETDEV_PCPU_STAT_DSTAT devices.
v2:
 - use correct percpu var ("stats", not "dstats") in dev_fetch_dstats
---
 net/core/dev.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index e1bb6d7856d9..3dcfc8d6c51a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10702,6 +10702,54 @@ void netdev_run_todo(void)
 		wake_up(&netdev_unregistering_wq);
 }
 
+/* Collate per-cpu network dstats statistics
+ *
+ * Read per-cpu network statistics from dev->dstats and populate the related
+ * fields in @s.
+ */
+static void dev_fetch_dstats(struct rtnl_link_stats64 *s,
+			     const struct pcpu_dstats __percpu *dstats)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		u64 rx_packets, rx_bytes, rx_drops;
+		u64 tx_packets, tx_bytes, tx_drops;
+		const struct pcpu_dstats *stats;
+		unsigned int start;
+
+		stats = per_cpu_ptr(dstats, cpu);
+		do {
+			start = u64_stats_fetch_begin(&stats->syncp);
+			rx_packets = u64_stats_read(&stats->rx_packets);
+			rx_bytes   = u64_stats_read(&stats->rx_bytes);
+			rx_drops   = u64_stats_read(&stats->rx_drops);
+			tx_packets = u64_stats_read(&stats->tx_packets);
+			tx_bytes   = u64_stats_read(&stats->tx_bytes);
+			tx_drops   = u64_stats_read(&stats->tx_drops);
+		} while (u64_stats_fetch_retry(&stats->syncp, start));
+
+		s->rx_packets += rx_packets;
+		s->rx_bytes   += rx_bytes;
+		s->rx_dropped += rx_drops;
+		s->tx_packets += tx_packets;
+		s->tx_bytes   += tx_bytes;
+		s->tx_dropped += tx_drops;
+	}
+}
+
+/* ndo_get_stats64 implementation for dtstats-based accounting.
+ *
+ * Populate @s from dev->stats and dev->dstats. This is used internally by the
+ * core for NETDEV_PCPU_STAT_DSTAT-type stats collection.
+ */
+static void dev_get_dstats64(const struct net_device *dev,
+			     struct rtnl_link_stats64 *s)
+{
+	netdev_stats_to_stats64(s, &dev->stats);
+	dev_fetch_dstats(s, dev->dstats);
+}
+
 /* Convert net_device_stats to rtnl_link_stats64. rtnl_link_stats64 has
  * all the same fields in the same order as net_device_stats, with only
  * the type differing, but rtnl_link_stats64 may have additional fields
@@ -10778,6 +10826,8 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 		netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev));
 	} else if (dev->pcpu_stat_type == NETDEV_PCPU_STAT_TSTATS) {
 		dev_get_tstats64(dev, storage);
+	} else if (dev->pcpu_stat_type == NETDEV_PCPU_STAT_DSTATS) {
+		dev_get_dstats64(dev, storage);
 	} else {
 		netdev_stats_to_stats64(storage, &dev->stats);
 	}

-- 
2.39.2

Re: [PATCH net-next v3 2/3] net: core: Implement dstats-type stats collections

[Simon Horman]

On Fri, Jun 07, 2024 at 06:25:25PM +0800, Jeremy Kerr wrote:
> We currently have dev_get_tstats64() for collecting per-cpu stats of
> type pcpu_sw_netstats ("tstats"). However, tstats doesn't allow for
> accounting tx/rx drops. We do have a stats variant that does have stats
> for dropped packets: struct pcpu_dstats, but there are no core helpers
> for using those stats.
> 
> The VRF driver uses dstats, by providing its own collation/fetch
> functions to do so.
> 
> This change adds a common implementation for dstats-type collection,
> used when pcpu_stat_type == NETDEV_PCPU_STAT_DSTAT. This is based on the
> VRF driver's existing stats collator (plus the unused tx_drops stat from
> there). We will switch the VRF driver to use this in the next change.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net-next v3 3/3] net: vrf: move to generic dstat helpers

[Jeremy Kerr]

The vrf driver has its own dstats-to-rtnl_link_stats64 collection, but
we now have a generic implementation for dstats collection, so switch to
this.

In doing so, we fix a minor issue where the (non-percpu)
dev->stats->tx_errors value was never collected into rtnl_link_stats64,
as the generic dev_get_dstats64() consumes the starting values from
dev->stats.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

---
v3:
 - we no longer need ->ndo_get_stats64, as the collection is now
   performed by default for NETDEV_PCPU_STAT_DSTATS devices.
---
 drivers/net/vrf.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 5018831b2a79..9af316cdd8b3 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -137,33 +137,6 @@ static void vrf_tx_error(struct net_device *vrf_dev, struct sk_buff *skb)
 	kfree_skb(skb);
 }
 
-static void vrf_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, tpkts, tdrops, rbytes, rpkts;
-		unsigned int start;
-
-		dstats = per_cpu_ptr(dev->dstats, i);
-		do {
-			start = u64_stats_fetch_begin(&dstats->syncp);
-			tbytes = u64_stats_read(&dstats->tx_bytes);
-			tpkts = u64_stats_read(&dstats->tx_packets);
-			tdrops = u64_stats_read(&dstats->tx_drops);
-			rbytes = u64_stats_read(&dstats->rx_bytes);
-			rpkts = u64_stats_read(&dstats->rx_packets);
-		} while (u64_stats_fetch_retry(&dstats->syncp, start));
-		stats->tx_bytes += tbytes;
-		stats->tx_packets += tpkts;
-		stats->tx_dropped += tdrops;
-		stats->rx_bytes += rbytes;
-		stats->rx_packets += rpkts;
-	}
-}
-
 static struct vrf_map *netns_vrf_map(struct net *net)
 {
 	struct netns_vrf *nn_vrf = net_generic(net, vrf_net_id);
@@ -1201,7 +1174,6 @@ static const struct net_device_ops vrf_netdev_ops = {
 	.ndo_uninit		= vrf_dev_uninit,
 	.ndo_start_xmit		= vrf_xmit,
 	.ndo_set_mac_address	= eth_mac_addr,
-	.ndo_get_stats64	= vrf_get_stats64,
 	.ndo_add_slave		= vrf_add_slave,
 	.ndo_del_slave		= vrf_del_slave,
 };

-- 
2.39.2

Re: [PATCH net-next v3 3/3] net: vrf: move to generic dstat helpers

[Simon Horman]

On Fri, Jun 07, 2024 at 06:25:26PM +0800, Jeremy Kerr wrote:
> The vrf driver has its own dstats-to-rtnl_link_stats64 collection, but
> we now have a generic implementation for dstats collection, so switch to
> this.
> 
> In doing so, we fix a minor issue where the (non-percpu)
> dev->stats->tx_errors value was never collected into rtnl_link_stats64,
> as the generic dev_get_dstats64() consumes the starting values from
> dev->stats.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v3 0/3] net: core: Unify dstats with tstats and lstats, implement generic dstats collection

[David Ahern]

On 6/7/24 4:25 AM, Jeremy Kerr wrote:
> The struct pcpu_dstats ("dstats") has a few variations from the other
> two stats types (struct pcpu_sw_netstats and struct pcpu_lstats), and
> doesn't have generic helpers for collecting the per-cpu stats into a
> struct rtnl_link_stats64.
> 
> This change unifies dstats with the other types, adds a collection
> implementation to the core, and updates the single driver (vrf) to use
> this generic implementation.
> 
> Of course, questions/comments/etc are most welcome!
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---
> v3:
> - rather than exposing helpers, perform dstat collection implicitly when
>   type == NETDEV_PCPU_STAT_DSTAT
> - Link to v2:
>   https://lore.kernel.org/r/20240605-dstats-v2-0-7fae03f813f3@codeconstruct.com.au
> 
> ---
> v2:
> - use correct percpu var in dev_fetch_dstats
> - use correct accessor in vfr rx drop accounting
> - v1: https://lore.kernel.org/r/20240605-dstats-v1-0-1024396e1670@codeconstruct.com.au
> 
> ---
> Jeremy Kerr (3):
>       net: core,vrf: Change pcpu_dstat fields to u64_stats_t
>       net: core: Implement dstats-type stats collections
>       net: vrf: move to generic dstat helpers
> 
>  drivers/net/vrf.c         | 56 ++++++++++++++---------------------------------
>  include/linux/netdevice.h | 12 +++++-----
>  net/core/dev.c            | 50 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 73 insertions(+), 45 deletions(-)
> ---
> base-commit: 32f88d65f01bf6f45476d7edbe675e44fb9e1d58
> change-id: 20240605-dstats-b6e08c318555
> 
> Best regards,

For the set:
Reviewed-by: David Ahern <dsahern@kernel.org>

thanks

Re: [PATCH net-next v3 0/3] net: core: Unify dstats with tstats and lstats, implement generic dstats collection

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 07 Jun 2024 18:25:23 +0800 you wrote:
> The struct pcpu_dstats ("dstats") has a few variations from the other
> two stats types (struct pcpu_sw_netstats and struct pcpu_lstats), and
> doesn't have generic helpers for collecting the per-cpu stats into a
> struct rtnl_link_stats64.
> 
> This change unifies dstats with the other types, adds a collection
> implementation to the core, and updates the single driver (vrf) to use
> this generic implementation.
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/3] net: core,vrf: Change pcpu_dstat fields to u64_stats_t
    https://git.kernel.org/netdev/net-next/c/fa59dc2f6fc6
  - [net-next,v3,2/3] net: core: Implement dstats-type stats collections
    https://git.kernel.org/netdev/net-next/c/94b601bc4f85
  - [net-next,v3,3/3] net: vrf: move to generic dstat helpers
    https://git.kernel.org/netdev/net-next/c/2202576d4631

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html