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

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

[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
helper (dev_get_dstats64) for ->ndo_get_stats64, and updates the single
driver (vrf) to use this helper.

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

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
v2:
- use correct percpu var in dev_fetch_dstats
- use correct accessor in vfr rx drop accounting
- Link to 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         | 57 +++++++++++++++--------------------------------
 include/linux/netdevice.h | 15 ++++++++-----
 net/core/dev.c            | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 45 deletions(-)
---
base-commit: 32f88d65f01bf6f45476d7edbe675e44fb9e1d58
change-id: 20240605-dstats-b6e08c318555

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

[PATCH net-next v2 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

[PATCH net-next v2 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 helpers for dstats-type collection, based on
the VRF driver's own (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>

---
v2:
 - use correct percpu var ("stats", not "dstats") in dev_fetch_dstats
---
 include/linux/netdevice.h |  3 +++
 net/core/dev.c            | 56 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f148a01dd1d1..fdc3d8a6c0f4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4690,6 +4690,9 @@ void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
 void dev_fetch_sw_netstats(struct rtnl_link_stats64 *s,
 			   const struct pcpu_sw_netstats __percpu *netstats);
 void dev_get_tstats64(struct net_device *dev, struct rtnl_link_stats64 *s);
+void dev_fetch_dstats(struct rtnl_link_stats64 *s,
+		      const struct pcpu_dstats __percpu *dstats);
+void dev_get_dstats64(struct net_device *dev, struct rtnl_link_stats64 *s);
 
 enum {
 	NESTED_SYNC_IMM_BIT,
diff --git a/net/core/dev.c b/net/core/dev.c
index e1bb6d7856d9..7536a80aefcc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10849,6 +10849,62 @@ void dev_get_tstats64(struct net_device *dev, struct rtnl_link_stats64 *s)
 }
 EXPORT_SYMBOL_GPL(dev_get_tstats64);
 
+/**
+ *	dev_fetch_dstats - collate per-cpu network dstats statistics
+ *	@s: place to store stats
+ *	@dstats: per-cpu network stats to read from
+ *
+ *	Read per-cpu network statistics from dev->dstats and populate the
+ *	related fields in @s.
+ */
+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;
+	}
+}
+EXPORT_SYMBOL_GPL(dev_fetch_dstats);
+
+/**
+ *	dev_get_dstats64 - ndo_get_stats64 implementation for dtstats-based
+ *	account.
+ *	@dev: device to get statistics from
+ *	@s: place to store stats
+ *
+ *	Populate @s from dev->stats and dev->dstats. Can be used as
+ *	ndo_get_stats64() callback.
+ */
+void dev_get_dstats64(struct net_device *dev, struct rtnl_link_stats64 *s)
+{
+	netdev_stats_to_stats64(s, &dev->stats);
+	dev_fetch_dstats(s, dev->dstats);
+}
+EXPORT_SYMBOL_GPL(dev_get_dstats64);
+
 struct netdev_queue *dev_ingress_queue_create(struct net_device *dev)
 {
 	struct netdev_queue *queue = dev_ingress_queue(dev);

-- 
2.39.2

[PATCH net-next v2 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 helper for this.

Switch to the generic helper.

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>
---
 drivers/net/vrf.c | 29 +----------------------------
 1 file changed, 1 insertion(+), 28 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 5018831b2a79..b1083620aeea 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,7 @@ 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_get_stats64	= dev_get_dstats64,
 	.ndo_add_slave		= vrf_add_slave,
 	.ndo_del_slave		= vrf_del_slave,
 };

-- 
2.39.2

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

[Jakub Kicinski]

On Wed, 05 Jun 2024 17:42:56 +0800 Jeremy Kerr wrote:
> This change unifies dstats with the other types, adds a collection
> helper (dev_get_dstats64) for ->ndo_get_stats64, and updates the single
> driver (vrf) to use this helper.

I think you can go further, instead of exporting the helpers and
hooking them in the driver, just add the handling based on
pcpu_stat_type directly in dev_get_stats().

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

[Jeremy Kerr]

Hi Jakub,

> > This change unifies dstats with the other types, adds a collection
> > helper (dev_get_dstats64) for ->ndo_get_stats64, and updates the
> > single
> > driver (vrf) to use this helper.
> 
> I think you can go further, instead of exporting the helpers and
> hooking them in the driver, just add the handling based on
> pcpu_stat_type directly in dev_get_stats().

OK, makes sense!

If we're not exporting the helpers, that means that drivers that use
dstats wouldn't have a facility to customise the stats collection
through a ndo_get_stats64() callback though (as can be done with
tstats). Are you ok with that?

[the set of drivers that need that is currently zero, so more of a
hypothetical future problem...]

Cheers,


Jeremy

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

[Jakub Kicinski]

On Thu, 06 Jun 2024 10:11:51 +0800 Jeremy Kerr wrote:
> If we're not exporting the helpers, that means that drivers that use
> dstats wouldn't have a facility to customise the stats collection
> through a ndo_get_stats64() callback though (as can be done with
> tstats). Are you ok with that?
> 
> [the set of drivers that need that is currently zero, so more of a
> hypothetical future problem...]

Right, but I think "no exports unless there is an in-tree user"
is still a rule. A bit of a risk that someone will roll their own
per-cpu stats pointlessly if we lack this export. But let's try
to catch that in review..

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

[Jeremy Kerr]

Hi Jakub,

> > If we're not exporting the helpers, that means that drivers that
> > use
> > dstats wouldn't have a facility to customise the stats collection
> > through a ndo_get_stats64() callback though (as can be done with
> > tstats). Are you ok with that?
> > 
> > [the set of drivers that need that is currently zero, so more of a
> > hypothetical future problem...]
> 
> Right, but I think "no exports unless there is an in-tree user"
> is still a rule. A bit of a risk that someone will roll their own
> per-cpu stats pointlessly if we lack this export. But let's try
> to catch that in review..

OK, sounds good! I'll send a v3 shortly.

Cheers,


Jeremy

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

[Jakub Kicinski]

On Thu, 06 Jun 2024 11:01:01 +0800 Jeremy Kerr wrote:
> > Right, but I think "no exports unless there is an in-tree user"
> > is still a rule. A bit of a risk that someone will roll their own
> > per-cpu stats pointlessly if we lack this export. But let's try
> > to catch that in review..  
> 
> OK, sounds good! I'll send a v3 shortly.

About that.. :) We do advise to wait 24 hours before sending next
versions in case there is more feedback / someone disagrees:

https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#tl-dr

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

[Jeremy Kerr]

Hi Jakub,

> > > Right, but I think "no exports unless there is an in-tree user"
> > > is still a rule. A bit of a risk that someone will roll their own
> > > per-cpu stats pointlessly if we lack this export. But let's try
> > > to catch that in review..  
> > 
> > OK, sounds good! I'll send a v3 shortly.
> 
> About that.. :) We do advise to wait 24 hours before sending next
> versions in case there is more feedback / someone disagrees:

OK, for large values of "shortly" then :D

(sorry, I got a bit enthusiastic about v2, given the brown-paper-bag
bug in v1)

Cheers,


Jeremy