[net-next v7 0/4] page_pool: Add stats counters

[net-next v7 0/4] page_pool: Add stats counters

[Joe Damato]

Greetings:

Welcome to v7.

This revision splits stats into two structures and tries to pick the right
placement within the page_pool struct. The allocation path stats have been
changed to be per-pool (not per-cpu), and the recycle stats are per-pool
per-cpu.

pahole reports:

	/* --- cacheline 3 boundary (192 bytes) --- */
	struct page_pool_alloc_stats alloc_stats;        /*   192    48 */
	u32                        xdp_mem_id;           /*   240     4 */

	/* --- cacheline 24 boundary (1536 bytes) --- */
	struct page_pool_recycle_stats * recycle_stats;  /*  1536     8 */
	atomic_t                   pages_state_release_cnt; /*  1544     4 */
	refcount_t                 user_cnt;             /*  1548     4 */
	u64                        destroy_cnt;          /*  1552     8 */

	/* size: 1600, cachelines: 25, members: 17 */
	/* sum members: 1492, holes: 2, sum holes: 68 */
	/* padding: 40 */

The page_pool_get_stats API has been updated to fill a wrapper struct
(struct page_pool_stats) which encapsulates both allocation and recycle
structs. It can be extended to include other stats in the future without
API breakage.

mlx5 driver patch to use this new API is included in this series.

Benchmarks have been re-run. As always, results between runs are highly
variable; you'll find results showing that stats disabled are both faster
and slower than stats enabled in back to back benchmark runs.

Raw benchmark output with stats off [1] and stats on [2] are available for
examination.

Test system:
	- 2x Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz
	- 2 NUMA zones, with 18 cores per zone and 2 threads per core

bench_page_pool_simple results, loops=200000000
test name			stats enabled		stats disabled
				cycles	nanosec		cycles	nanosec

for_loop			0	0.335		0	0.336
atomic_inc 			14	6.106		13	6.022
lock				30	13.365		32	13.968

no-softirq-page_pool01		75	32.884		74	32.308
no-softirq-page_pool02		79	34.696		74	32.302
no-softirq-page_pool03		110	48.005		105	46.073

tasklet_page_pool01_fast_path	14	6.156		14	6.211
tasklet_page_pool02_ptr_ring	41	18.028		39	17.391
tasklet_page_pool03_slow	107	46.646		105	46.123

bench_page_pool_cross_cpu results, loops=20000000 returning_cpus=4:
test name			stats enabled		stats disabled
				cycles	nanosec		cycles	nanosec

page_pool_cross_cpu CPU(0)	3973	1731.596	4015	1750.015
page_pool_cross_cpu CPU(1)	3976	1733.217	4022	1752.864
page_pool_cross_cpu CPU(2)	3973	1731.615	4016	1750.433
page_pool_cross_cpu CPU(3)	3976	1733.218	4021	1752.806
page_pool_cross_cpu CPU(4)	994	433.305		1005	438.217

page_pool_cross_cpu average	3378	-		3415	-

bench_page_pool_cross_cpu results, loops=20000000 returning_cpus=8:
test name			stats enabled		stats disabled
				cycles	nanosec		cycles	nanosec

page_pool_cross_cpu CPU(0)	6969	3037.488	6909	3011.463
page_pool_cross_cpu CPU(1)	6974	3039.469	6913	3012.961
page_pool_cross_cpu CPU(2)	6969	3037.575	6910	3011.585
page_pool_cross_cpu CPU(3)	6974	3039.415	6913	3012.961
page_pool_cross_cpu CPU(4)	6969	3037.288	6909	3011.368
page_pool_cross_cpu CPU(5)	6972	3038.732	6913	3012.920
page_pool_cross_cpu CPU(6)	6969	3037.350	6909	3011.386
page_pool_cross_cpu CPU(7)	6973	3039.356	6913	3012.921
page_pool_cross_cpu CPU(8)	871	379.934		864	376.620

page_pool_cross_cpu average	6293	-		6239	-

Thanks.

[1]: https://gist.githubusercontent.com/jdamato-fsly/d7c34b9fa7be1ce132a266b0f2b92aea/raw/327dcd71d11ece10238fbf19e0472afbcbf22fd4/v7_stats_disabled
[2]: https://gist.githubusercontent.com/jdamato-fsly/d7c34b9fa7be1ce132a266b0f2b92aea/raw/327dcd71d11ece10238fbf19e0472afbcbf22fd4/v7_stats_enabled

v6 -> v7:
	- stats split out into two structs one single per-page pool struct
	  for allocation path stats and one per-cpu pointer for recycle
	  path stats.
	- page_pool_get_stats updated to use a wrapper struct to gather
	  stats for allocation and recycle stats with a single argument.
	- placement of structs adjusted
	- mlx5 driver modified to use page_pool_get_stats API

v5 -> v6:
	- Per cpu page_pool_stats struct pointer is now marked as
	  ____cacheline_aligned_in_smp. Placement of the field in the
	  struct is unchanged; it is the last field.

v4 -> v5:
	- Fixed the description of the kernel option in Kconfig.
	- Squashed commits 1-10 from v4 into a single commit for easier
	  review.
	- Changed the comment style of the comment for
	  the this_cpu_inc_alloc_stat macro.
	- Changed the return type of page_pool_get_stats from struct
	  page_pool_stat * to bool.

v3 -> v4:
	- Restructured stats to be per-cpu per-pool.
	- Global stats and proc file were removed.
	- Exposed an API (page_pool_get_stats) for batching the pool stats.

v2 -> v3:
	- patch 8/10 ("Add stat tracking cache refill") fixed placement of
	  counter increment.
	- patch 10/10 ("net-procfs: Show page pool stats in proc") updated:
		- fix unused label warning from kernel test robot,
		- fixed page_pool_seq_show to only display the refill stat
		  once,
		- added a remove_proc_entry for page_pool_stat to
		  dev_proc_net_exit.

v1 -> v2:
	- A new kernel config option has been added, which defaults to N,
	   preventing this code from being compiled in by default
	- The stats structure has been converted to a per-cpu structure
	- The stats are now exported via proc (/proc/net/page_pool_stat)

Joe Damato (4):
  page_pool: Add allocation stats
  page_pool: Add recycle stats
  page_pool: Add function to batch and return stats
  mlx5: add support for page_pool_get_stats

 drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 76 +++++++++++++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 27 +++++++-
 include/net/page_pool.h                            | 51 ++++++++++++++
 net/Kconfig                                        | 13 ++++
 net/core/page_pool.c                               | 77 ++++++++++++++++++++--
 5 files changed, 238 insertions(+), 6 deletions(-)

-- 
2.7.4

[net-next v7 1/4] page_pool: Add allocation stats

[Joe Damato]

Add per-pool statistics counters for the allocation path of a page pool.
These stats are incremented in softirq context, so no locking or per-cpu
variables are needed.

This code is disabled by default and a kernel config option is provided for
users who wish to enable them.

The statistics added are:
	- fast: successful fast path allocations
	- slow: slow path order-0 allocations
	- slow_high_order: slow path high order allocations
	- empty: ptr ring is empty, so a slow path allocation was forced.
	- refill: an allocation which triggered a refill of the cache
	- waive: pages obtained from the ptr ring that cannot be added to
	  the cache due to a NUMA mismatch.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 include/net/page_pool.h | 18 ++++++++++++++++++
 net/Kconfig             | 13 +++++++++++++
 net/core/page_pool.c    | 24 ++++++++++++++++++++----
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 97c3c19..1f27e8a4 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -84,6 +84,19 @@ struct page_pool_params {
 	void *init_arg;
 };
 
+#ifdef CONFIG_PAGE_POOL_STATS
+struct page_pool_alloc_stats {
+	u64 fast; /* fast path allocations */
+	u64 slow; /* slow-path order 0 allocations */
+	u64 slow_high_order; /* slow-path high order allocations */
+	u64 empty; /* failed refills due to empty ptr ring, forcing
+		    * slow path allocation
+		    */
+	u64 refill; /* allocations via successful refill */
+	u64 waive;  /* failed refills due to numa zone mismatch */
+};
+#endif
+
 struct page_pool {
 	struct page_pool_params p;
 
@@ -96,6 +109,11 @@ struct page_pool {
 	unsigned int frag_offset;
 	struct page *frag_page;
 	long frag_users;
+
+#ifdef CONFIG_PAGE_POOL_STATS
+	/* these stats are incremented while in softirq context */
+	struct page_pool_alloc_stats alloc_stats;
+#endif
 	u32 xdp_mem_id;
 
 	/*
diff --git a/net/Kconfig b/net/Kconfig
index 8a1f9d0..6b78f69 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -434,6 +434,19 @@ config NET_DEVLINK
 config PAGE_POOL
 	bool
 
+config PAGE_POOL_STATS
+	default n
+	bool "Page pool stats"
+	depends on PAGE_POOL
+	help
+	  Enable page pool statistics to track page allocation and recycling
+	  in page pools. This option incurs additional CPU cost in allocation
+	  and recycle paths and additional memory cost to store the statistics.
+	  These statistics are only available if this option is enabled and if
+	  the driver using the page pool supports exporting this data.
+
+	  If unsure, say N.
+
 config FAILOVER
 	tristate "Generic failover module"
 	help
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index e25d359..0fa4b76 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -26,6 +26,13 @@
 
 #define BIAS_MAX	LONG_MAX
 
+#ifdef CONFIG_PAGE_POOL_STATS
+/* alloc_stat_inc is intended to be used in softirq context */
+#define alloc_stat_inc(pool, __stat)	(pool->alloc_stats.__stat++)
+#else
+#define alloc_stat_inc(pool, __stat)
+#endif
+
 static int page_pool_init(struct page_pool *pool,
 			  const struct page_pool_params *params)
 {
@@ -117,8 +124,10 @@ static struct page *page_pool_refill_alloc_cache(struct page_pool *pool)
 	int pref_nid; /* preferred NUMA node */
 
 	/* Quicker fallback, avoid locks when ring is empty */
-	if (__ptr_ring_empty(r))
+	if (__ptr_ring_empty(r)) {
+		alloc_stat_inc(pool, empty);
 		return NULL;
+	}
 
 	/* Softirq guarantee CPU and thus NUMA node is stable. This,
 	 * assumes CPU refilling driver RX-ring will also run RX-NAPI.
@@ -145,14 +154,17 @@ static struct page *page_pool_refill_alloc_cache(struct page_pool *pool)
 			 * This limit stress on page buddy alloactor.
 			 */
 			page_pool_return_page(pool, page);
+			alloc_stat_inc(pool, waive);
 			page = NULL;
 			break;
 		}
 	} while (pool->alloc.count < PP_ALLOC_CACHE_REFILL);
 
 	/* Return last page */
-	if (likely(pool->alloc.count > 0))
+	if (likely(pool->alloc.count > 0)) {
 		page = pool->alloc.cache[--pool->alloc.count];
+		alloc_stat_inc(pool, refill);
+	}
 
 	return page;
 }
@@ -166,6 +178,7 @@ static struct page *__page_pool_get_cached(struct page_pool *pool)
 	if (likely(pool->alloc.count)) {
 		/* Fast-path */
 		page = pool->alloc.cache[--pool->alloc.count];
+		alloc_stat_inc(pool, fast);
 	} else {
 		page = page_pool_refill_alloc_cache(pool);
 	}
@@ -239,6 +252,7 @@ static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
 		return NULL;
 	}
 
+	alloc_stat_inc(pool, slow_high_order);
 	page_pool_set_pp_info(pool, page);
 
 	/* Track how many pages are held 'in-flight' */
@@ -293,10 +307,12 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 	}
 
 	/* Return last page */
-	if (likely(pool->alloc.count > 0))
+	if (likely(pool->alloc.count > 0)) {
 		page = pool->alloc.cache[--pool->alloc.count];
-	else
+		alloc_stat_inc(pool, slow);
+	} else {
 		page = NULL;
+	}
 
 	/* When page just alloc'ed is should/must have refcnt 1. */
 	return page;
-- 
2.7.4

[net-next v7 2/4] page_pool: Add recycle stats

[Joe Damato]

Add per-cpu stats tracking page pool recycling events:
	- cached: recycling placed page in the page pool cache
	- cache_full: page pool cache was full
	- ring: page placed into the ptr ring
	- ring_full: page released from page pool because the ptr ring was full
	- released_refcnt: page released (and not recycled) because refcnt > 1

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 include/net/page_pool.h | 16 ++++++++++++++++
 net/core/page_pool.c    | 28 +++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 1f27e8a4..298af95 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -95,6 +95,18 @@ struct page_pool_alloc_stats {
 	u64 refill; /* allocations via successful refill */
 	u64 waive;  /* failed refills due to numa zone mismatch */
 };
+
+struct page_pool_recycle_stats {
+	u64 cached;	/* recycling placed page in the cache. */
+	u64 cache_full; /* cache was full */
+	u64 ring;	/* recycling placed page back into ptr ring */
+	u64 ring_full;	/* page was released from page-pool because
+			 * PTR ring was full.
+			 */
+	u64 released_refcnt; /* page released because of elevated
+			      * refcnt
+			      */
+};
 #endif
 
 struct page_pool {
@@ -144,6 +156,10 @@ struct page_pool {
 	 */
 	struct ptr_ring ring;
 
+#ifdef CONFIG_PAGE_POOL_STATS
+	/* recycle stats are per-cpu to avoid locking */
+	struct page_pool_recycle_stats __percpu *recycle_stats;
+#endif
 	atomic_t pages_state_release_cnt;
 
 	/* A page_pool is strictly tied to a single RX-queue being
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 0fa4b76..27233bf 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -29,8 +29,15 @@
 #ifdef CONFIG_PAGE_POOL_STATS
 /* alloc_stat_inc is intended to be used in softirq context */
 #define alloc_stat_inc(pool, __stat)	(pool->alloc_stats.__stat++)
+/* recycle_stat_inc is safe to use when preemption is possible. */
+#define recycle_stat_inc(pool, __stat)							\
+	do {										\
+		struct page_pool_recycle_stats __percpu *s = pool->recycle_stats;	\
+		this_cpu_inc(s->__stat);						\
+	} while (0)
 #else
 #define alloc_stat_inc(pool, __stat)
+#define recycle_stat_inc(pool, __stat)
 #endif
 
 static int page_pool_init(struct page_pool *pool,
@@ -80,6 +87,12 @@ static int page_pool_init(struct page_pool *pool,
 	    pool->p.flags & PP_FLAG_PAGE_FRAG)
 		return -EINVAL;
 
+#ifdef CONFIG_PAGE_POOL_STATS
+	pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
+	if (!pool->recycle_stats)
+		return -ENOMEM;
+#endif
+
 	if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0)
 		return -ENOMEM;
 
@@ -410,6 +423,11 @@ static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)
 	else
 		ret = ptr_ring_produce_bh(&pool->ring, page);
 
+#ifdef CONFIG_PAGE_POOL_STATS
+	if (ret == 0)
+		recycle_stat_inc(pool, ring);
+#endif
+
 	return (ret == 0) ? true : false;
 }
 
@@ -421,11 +439,14 @@ static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)
 static bool page_pool_recycle_in_cache(struct page *page,
 				       struct page_pool *pool)
 {
-	if (unlikely(pool->alloc.count == PP_ALLOC_CACHE_SIZE))
+	if (unlikely(pool->alloc.count == PP_ALLOC_CACHE_SIZE)) {
+		recycle_stat_inc(pool, cache_full);
 		return false;
+	}
 
 	/* Caller MUST have verified/know (page_ref_count(page) == 1) */
 	pool->alloc.cache[pool->alloc.count++] = page;
+	recycle_stat_inc(pool, cached);
 	return true;
 }
 
@@ -475,6 +496,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
 	 * doing refcnt based recycle tricks, meaning another process
 	 * will be invoking put_page.
 	 */
+	recycle_stat_inc(pool, released_refcnt);
 	/* Do not replace this with page_pool_return_page() */
 	page_pool_release_page(pool, page);
 	put_page(page);
@@ -488,6 +510,7 @@ void page_pool_put_defragged_page(struct page_pool *pool, struct page *page,
 	page = __page_pool_put_page(pool, page, dma_sync_size, allow_direct);
 	if (page && !page_pool_recycle_in_ring(pool, page)) {
 		/* Cache full, fallback to free pages */
+		recycle_stat_inc(pool, ring_full);
 		page_pool_return_page(pool, page);
 	}
 }
@@ -636,6 +659,9 @@ static void page_pool_free(struct page_pool *pool)
 	if (pool->p.flags & PP_FLAG_DMA_MAP)
 		put_device(pool->p.dev);
 
+#ifdef CONFIG_PAGE_POOL_STATS
+	free_percpu(pool->recycle_stats);
+#endif
 	kfree(pool);
 }
 
-- 
2.7.4

[net-next v7 3/4] page_pool: Add function to batch and return stats

[Joe Damato]

Adds a function page_pool_get_stats which can be used by drivers to obtain
stats for a specified page_pool.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 include/net/page_pool.h | 17 +++++++++++++++++
 net/core/page_pool.c    | 25 +++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 298af95..ea5fb70 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -107,6 +107,23 @@ struct page_pool_recycle_stats {
 			      * refcnt
 			      */
 };
+
+/* This struct wraps the above stats structs so users of the
+ * page_pool_get_stats API can pass a single argument when requesting the
+ * stats for the page pool.
+ */
+struct page_pool_stats {
+	struct page_pool_alloc_stats alloc_stats;
+	struct page_pool_recycle_stats recycle_stats;
+};
+
+/*
+ * Drivers that wish to harvest page pool stats and report them to users
+ * (perhaps via ethtool, debugfs, or another mechanism) can allocate a
+ * struct page_pool_stats call page_pool_get_stats to get stats for the specified pool.
+ */
+bool page_pool_get_stats(struct page_pool *pool,
+			 struct page_pool_stats *stats);
 #endif
 
 struct page_pool {
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 27233bf..f4f8f5f 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -35,6 +35,31 @@
 		struct page_pool_recycle_stats __percpu *s = pool->recycle_stats;	\
 		this_cpu_inc(s->__stat);						\
 	} while (0)
+
+bool page_pool_get_stats(struct page_pool *pool,
+			 struct page_pool_stats *stats)
+{
+	int cpu = 0;
+
+	if (!stats)
+		return false;
+
+	memcpy(&stats->alloc_stats, &pool->alloc_stats, sizeof(pool->alloc_stats));
+
+	for_each_possible_cpu(cpu) {
+		const struct page_pool_recycle_stats *pcpu =
+			per_cpu_ptr(pool->recycle_stats, cpu);
+
+		stats->recycle_stats.cached += pcpu->cached;
+		stats->recycle_stats.cache_full += pcpu->cache_full;
+		stats->recycle_stats.ring += pcpu->ring;
+		stats->recycle_stats.ring_full += pcpu->ring_full;
+		stats->recycle_stats.released_refcnt += pcpu->released_refcnt;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(page_pool_get_stats);
 #else
 #define alloc_stat_inc(pool, __stat)
 #define recycle_stat_inc(pool, __stat)
-- 
2.7.4

[net-next v7 4/4] mlx5: add support for page_pool_get_stats

[Joe Damato]

This change adds support for the page_pool_get_stats API to mlx5. If the
user has enabled CONFIG_PAGE_POOL_STATS in their kernel, ethtool will
output page pool stats.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 76 ++++++++++++++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 27 +++++++-
 2 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index 3e5d8c7..56eedf5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -37,6 +37,10 @@
 #include "en/ptp.h"
 #include "en/port.h"
 
+#ifdef CONFIG_PAGE_POOL_STATS
+#include <net/page_pool.h>
+#endif
+
 static unsigned int stats_grps_num(struct mlx5e_priv *priv)
 {
 	return !priv->profile->stats_grps_num ? 0 :
@@ -183,6 +187,19 @@ static const struct counter_desc sw_stats_desc[] = {
 	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_congst_umr) },
 	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_arfs_err) },
 	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_recover) },
+#ifdef CONFIG_PAGE_POOL_STATS
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_fast) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_slow) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_slow_high_order) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_empty) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_refill) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_waive) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_rec_cached) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_rec_cache_full) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_rec_ring) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_rec_ring_full) },
+	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_rec_released_ref) },
+#endif
 #ifdef CONFIG_MLX5_EN_TLS
 	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_tls_decrypted_packets) },
 	{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_tls_decrypted_bytes) },
@@ -349,6 +366,20 @@ static void mlx5e_stats_grp_sw_update_stats_rq_stats(struct mlx5e_sw_stats *s,
 	s->rx_congst_umr              += rq_stats->congst_umr;
 	s->rx_arfs_err                += rq_stats->arfs_err;
 	s->rx_recover                 += rq_stats->recover;
+#ifdef CONFIG_PAGE_POOL_STATS
+	s->rx_page_pool_fast          += rq_stats->page_pool_fast;
+	s->rx_page_pool_slow          += rq_stats->page_pool_slow;
+	s->rx_page_pool_empty         += rq_stats->page_pool_empty;
+	s->rx_page_pool_refill        += rq_stats->page_pool_refill;
+	s->rx_page_pool_waive         += rq_stats->page_pool_waive;
+
+	s->rx_page_pool_slow_high_order		+= rq_stats->page_pool_slow_high_order;
+	s->rx_page_pool_rec_cached		+= rq_stats->page_pool_rec_cached;
+	s->rx_page_pool_rec_cache_full		+= rq_stats->page_pool_rec_cache_full;
+	s->rx_page_pool_rec_ring		+= rq_stats->page_pool_rec_ring;
+	s->rx_page_pool_rec_ring_full		+= rq_stats->page_pool_rec_ring_full;
+	s->rx_page_pool_rec_released_ref	+= rq_stats->page_pool_rec_released_ref;
+#endif
 #ifdef CONFIG_MLX5_EN_TLS
 	s->rx_tls_decrypted_packets   += rq_stats->tls_decrypted_packets;
 	s->rx_tls_decrypted_bytes     += rq_stats->tls_decrypted_bytes;
@@ -455,6 +486,35 @@ static void mlx5e_stats_grp_sw_update_stats_qos(struct mlx5e_priv *priv,
 	}
 }
 
+#ifdef CONFIG_PAGE_POOL_STATS
+static void mlx5e_stats_update_stats_rq_page_pool(struct mlx5e_channel *c)
+{
+	struct mlx5e_rq_stats *rq_stats = c->rq.stats;
+	struct page_pool *pool = c->rq.page_pool;
+	struct page_pool_stats stats = { 0 };
+
+	if (!page_pool_get_stats(pool, &stats))
+		return;
+
+	rq_stats->page_pool_fast = stats.alloc_stats.fast;
+	rq_stats->page_pool_slow = stats.alloc_stats.slow;
+	rq_stats->page_pool_slow_high_order = stats.alloc_stats.slow_high_order;
+	rq_stats->page_pool_empty = stats.alloc_stats.empty;
+	rq_stats->page_pool_waive = stats.alloc_stats.waive;
+	rq_stats->page_pool_refill = stats.alloc_stats.refill;
+
+	rq_stats->page_pool_rec_cached = stats.recycle_stats.cached;
+	rq_stats->page_pool_rec_cache_full = stats.recycle_stats.cache_full;
+	rq_stats->page_pool_rec_ring = stats.recycle_stats.ring;
+	rq_stats->page_pool_rec_ring_full = stats.recycle_stats.ring_full;
+	rq_stats->page_pool_rec_released_ref = stats.recycle_stats.released_refcnt;
+}
+#else
+static void mlx5e_stats_update_stats_rq_page_pool(struct mlx5e_channel *c)
+{
+}
+#endif
+
 static MLX5E_DECLARE_STATS_GRP_OP_UPDATE_STATS(sw)
 {
 	struct mlx5e_sw_stats *s = &priv->stats.sw;
@@ -465,8 +525,11 @@ static MLX5E_DECLARE_STATS_GRP_OP_UPDATE_STATS(sw)
 	for (i = 0; i < priv->stats_nch; i++) {
 		struct mlx5e_channel_stats *channel_stats =
 			priv->channel_stats[i];
+
 		int j;
 
+		mlx5e_stats_update_stats_rq_page_pool(priv->channels.c[i]);
+
 		mlx5e_stats_grp_sw_update_stats_rq_stats(s, &channel_stats->rq);
 		mlx5e_stats_grp_sw_update_stats_xdpsq(s, &channel_stats->rq_xdpsq);
 		mlx5e_stats_grp_sw_update_stats_ch_stats(s, &channel_stats->ch);
@@ -1887,6 +1950,19 @@ static const struct counter_desc rq_stats_desc[] = {
 	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, congst_umr) },
 	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, arfs_err) },
 	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, recover) },
+#ifdef CONFIG_PAGE_POOL_STATS
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
+	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
+#endif
 #ifdef CONFIG_MLX5_EN_TLS
 	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, tls_decrypted_packets) },
 	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, tls_decrypted_bytes) },
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
index 14eaf92..9f66425 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
@@ -205,7 +205,19 @@ struct mlx5e_sw_stats {
 	u64 ch_aff_change;
 	u64 ch_force_irq;
 	u64 ch_eq_rearm;
-
+#ifdef CONFIG_PAGE_POOL_STATS
+	u64 rx_page_pool_fast;
+	u64 rx_page_pool_slow;
+	u64 rx_page_pool_slow_high_order;
+	u64 rx_page_pool_empty;
+	u64 rx_page_pool_refill;
+	u64 rx_page_pool_waive;
+	u64 rx_page_pool_rec_cached;
+	u64 rx_page_pool_rec_cache_full;
+	u64 rx_page_pool_rec_ring;
+	u64 rx_page_pool_rec_ring_full;
+	u64 rx_page_pool_rec_released_ref;
+#endif
 #ifdef CONFIG_MLX5_EN_TLS
 	u64 tx_tls_encrypted_packets;
 	u64 tx_tls_encrypted_bytes;
@@ -352,6 +364,19 @@ struct mlx5e_rq_stats {
 	u64 congst_umr;
 	u64 arfs_err;
 	u64 recover;
+#ifdef CONFIG_PAGE_POOL_STATS
+	u64 page_pool_fast;
+	u64 page_pool_slow;
+	u64 page_pool_slow_high_order;
+	u64 page_pool_empty;
+	u64 page_pool_refill;
+	u64 page_pool_waive;
+	u64 page_pool_rec_cached;
+	u64 page_pool_rec_cache_full;
+	u64 page_pool_rec_ring;
+	u64 page_pool_rec_ring_full;
+	u64 page_pool_rec_released_ref;
+#endif
 #ifdef CONFIG_MLX5_EN_TLS
 	u64 tls_decrypted_packets;
 	u64 tls_decrypted_bytes;
-- 
2.7.4

Re: [net-next v7 1/4] page_pool: Add allocation stats

[Jesper Dangaard Brouer]

On 25/02/2022 18.41, Joe Damato wrote:
> Add per-pool statistics counters for the allocation path of a page pool.
> These stats are incremented in softirq context, so no locking or per-cpu
> variables are needed.
> 
> This code is disabled by default and a kernel config option is provided for
> users who wish to enable them.
> 
> The statistics added are:
> 	- fast: successful fast path allocations
> 	- slow: slow path order-0 allocations
> 	- slow_high_order: slow path high order allocations
> 	- empty: ptr ring is empty, so a slow path allocation was forced.
> 	- refill: an allocation which triggered a refill of the cache
> 	- waive: pages obtained from the ptr ring that cannot be added to
> 	  the cache due to a NUMA mismatch.
> 
> Signed-off-by: Joe Damato<jdamato@fastly.com>
> ---
>   include/net/page_pool.h | 18 ++++++++++++++++++
>   net/Kconfig             | 13 +++++++++++++
>   net/core/page_pool.c    | 24 ++++++++++++++++++++----
>   3 files changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index 97c3c19..1f27e8a4 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -84,6 +84,19 @@ struct page_pool_params {
>   	void *init_arg;
>   };
>   
> +#ifdef CONFIG_PAGE_POOL_STATS
> +struct page_pool_alloc_stats {
> +	u64 fast; /* fast path allocations */
> +	u64 slow; /* slow-path order 0 allocations */
> +	u64 slow_high_order; /* slow-path high order allocations */
> +	u64 empty; /* failed refills due to empty ptr ring, forcing
> +		    * slow path allocation
> +		    */
> +	u64 refill; /* allocations via successful refill */
> +	u64 waive;  /* failed refills due to numa zone mismatch */
> +};
> +#endif
> +
>   struct page_pool {
>   	struct page_pool_params p;
>   
> @@ -96,6 +109,11 @@ struct page_pool {
>   	unsigned int frag_offset;
>   	struct page *frag_page;
>   	long frag_users;
> +
> +#ifdef CONFIG_PAGE_POOL_STATS
> +	/* these stats are incremented while in softirq context */
> +	struct page_pool_alloc_stats alloc_stats;
> +#endif
>   	u32 xdp_mem_id;

I like the cache-line placement.

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

Re: [net-next v7 2/4] page_pool: Add recycle stats

[Jesper Dangaard Brouer]

On 25/02/2022 18.41, Joe Damato wrote:
> Add per-cpu stats tracking page pool recycling events:
> 	- cached: recycling placed page in the page pool cache
> 	- cache_full: page pool cache was full
> 	- ring: page placed into the ptr ring
> 	- ring_full: page released from page pool because the ptr ring was full
> 	- released_refcnt: page released (and not recycled) because refcnt > 1
> 
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>   include/net/page_pool.h | 16 ++++++++++++++++
>   net/core/page_pool.c    | 28 +++++++++++++++++++++++++++-
>   2 files changed, 43 insertions(+), 1 deletion(-)

LGTM - thanks for working on this

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

Re: [net-next v7 3/4] page_pool: Add function to batch and return stats

[Jesper Dangaard Brouer]

On 25/02/2022 18.41, Joe Damato wrote:
> Adds a function page_pool_get_stats which can be used by drivers to obtain
> stats for a specified page_pool.
> 
> Signed-off-by: Joe Damato<jdamato@fastly.com>
> ---
>   include/net/page_pool.h | 17 +++++++++++++++++
>   net/core/page_pool.c    | 25 +++++++++++++++++++++++++
>   2 files changed, 42 insertions(+)

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

Re: [net-next v7 1/4] page_pool: Add allocation stats

[Ilias Apalodimas]

On Mon, 28 Feb 2022 at 09:07, Jesper Dangaard Brouer <jbrouer@redhat.com> wrote:
>
>
> On 25/02/2022 18.41, Joe Damato wrote:
> > Add per-pool statistics counters for the allocation path of a page pool.
> > These stats are incremented in softirq context, so no locking or per-cpu
> > variables are needed.
> >
> > This code is disabled by default and a kernel config option is provided for
> > users who wish to enable them.
> >
> > The statistics added are:
> >       - fast: successful fast path allocations
> >       - slow: slow path order-0 allocations
> >       - slow_high_order: slow path high order allocations
> >       - empty: ptr ring is empty, so a slow path allocation was forced.
> >       - refill: an allocation which triggered a refill of the cache
> >       - waive: pages obtained from the ptr ring that cannot be added to
> >         the cache due to a NUMA mismatch.
> >
> > Signed-off-by: Joe Damato<jdamato@fastly.com>
> > ---
> >   include/net/page_pool.h | 18 ++++++++++++++++++
> >   net/Kconfig             | 13 +++++++++++++
> >   net/core/page_pool.c    | 24 ++++++++++++++++++++----
> >   3 files changed, 51 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> > index 97c3c19..1f27e8a4 100644
> > --- a/include/net/page_pool.h
> > +++ b/include/net/page_pool.h
> > @@ -84,6 +84,19 @@ struct page_pool_params {
> >       void *init_arg;
> >   };
> >
> > +#ifdef CONFIG_PAGE_POOL_STATS
> > +struct page_pool_alloc_stats {
> > +     u64 fast; /* fast path allocations */
> > +     u64 slow; /* slow-path order 0 allocations */
> > +     u64 slow_high_order; /* slow-path high order allocations */
> > +     u64 empty; /* failed refills due to empty ptr ring, forcing
> > +                 * slow path allocation
> > +                 */
> > +     u64 refill; /* allocations via successful refill */
> > +     u64 waive;  /* failed refills due to numa zone mismatch */
> > +};
> > +#endif
> > +
> >   struct page_pool {
> >       struct page_pool_params p;
> >
> > @@ -96,6 +109,11 @@ struct page_pool {
> >       unsigned int frag_offset;
> >       struct page *frag_page;
> >       long frag_users;
> > +
> > +#ifdef CONFIG_PAGE_POOL_STATS
> > +     /* these stats are incremented while in softirq context */
> > +     struct page_pool_alloc_stats alloc_stats;
> > +#endif
> >       u32 xdp_mem_id;
>
> I like the cache-line placement.
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats

[Jesper Dangaard Brouer]

On 25/02/2022 18.41, Joe Damato wrote:
> +#ifdef CONFIG_PAGE_POOL_STATS
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
> +	{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
> +#endif

The naming: "page_pool_rec_xxx".
What does the "rec" stand for?

Users of ethtool -S stats... will they know "rec" is "recycle" ?

p.s. we need acks from driver maintainer(s).

--Jesper

Re: [net-next v7 2/4] page_pool: Add recycle stats

[Ilias Apalodimas]

On Mon, 28 Feb 2022 at 09:20, Jesper Dangaard Brouer <jbrouer@redhat.com> wrote:
>
>
> On 25/02/2022 18.41, Joe Damato wrote:
> > Add per-cpu stats tracking page pool recycling events:
> >       - cached: recycling placed page in the page pool cache
> >       - cache_full: page pool cache was full
> >       - ring: page placed into the ptr ring
> >       - ring_full: page released from page pool because the ptr ring was full
> >       - released_refcnt: page released (and not recycled) because refcnt > 1
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> >   include/net/page_pool.h | 16 ++++++++++++++++
> >   net/core/page_pool.c    | 28 +++++++++++++++++++++++++++-
> >   2 files changed, 43 insertions(+), 1 deletion(-)
>
> LGTM - thanks for working on this
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [net-next v7 3/4] page_pool: Add function to batch and return stats

[Ilias Apalodimas]

On Mon, 28 Feb 2022 at 09:22, Jesper Dangaard Brouer <jbrouer@redhat.com> wrote:
>
>
> On 25/02/2022 18.41, Joe Damato wrote:
> > Adds a function page_pool_get_stats which can be used by drivers to obtain
> > stats for a specified page_pool.
> >
> > Signed-off-by: Joe Damato<jdamato@fastly.com>
> > ---
> >   include/net/page_pool.h | 17 +++++++++++++++++
> >   net/core/page_pool.c    | 25 +++++++++++++++++++++++++
> >   2 files changed, 42 insertions(+)
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats

[Joe Damato]

On Sun, Feb 27, 2022 at 11:28 PM Jesper Dangaard Brouer
<jbrouer@redhat.com> wrote:
>
>
>
> On 25/02/2022 18.41, Joe Damato wrote:
> > +#ifdef CONFIG_PAGE_POOL_STATS
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
> > +#endif
>
> The naming: "page_pool_rec_xxx".
> What does the "rec" stand for?

rec stands for recycle.

ethtool strings have a limited size (ETH_GSTRING_LEN - 32 bytes) and
the full word "recycle" didn't fit for some of the stats once the
queue number is prepended elsewhere in the driver code.

> Users of ethtool -S stats... will they know "rec" is "recycle" ?

I am open to other names or adding documentation to the driver docs to
explain the meaning.

> p.s. we need acks from driver maintainer(s).

I've CC'd Tariq and Saaed; I hope they'll take a look when they have a
chance and weigh in on the naming. I am happy to adjust the names as
they wish and submit a v8, or take this code as-is and then iterate on
the names in a separate change.

I think the latter option would be easiest, but I am happy to do
whatever you all prefer.

Thanks,
Joe

Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats

[Toke Høiland-Jørgensen]

Joe Damato <jdamato@fastly.com> writes:

> On Sun, Feb 27, 2022 at 11:28 PM Jesper Dangaard Brouer
> <jbrouer@redhat.com> wrote:
>>
>>
>>
>> On 25/02/2022 18.41, Joe Damato wrote:
>> > +#ifdef CONFIG_PAGE_POOL_STATS
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
>> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
>> > +#endif
>>
>> The naming: "page_pool_rec_xxx".
>> What does the "rec" stand for?
>
> rec stands for recycle.
>
> ethtool strings have a limited size (ETH_GSTRING_LEN - 32 bytes) and
> the full word "recycle" didn't fit for some of the stats once the
> queue number is prepended elsewhere in the driver code.
>
>> Users of ethtool -S stats... will they know "rec" is "recycle" ?
>
> I am open to other names or adding documentation to the driver docs to
> explain the meaning.

You could shorten the 'page_pool_' prefix to 'ppool_' or even 'pp_' and
gain some characters that way?

-Toke

Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats

[Joe Damato]

On Mon, Feb 28, 2022 at 1:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Joe Damato <jdamato@fastly.com> writes:
>
> > On Sun, Feb 27, 2022 at 11:28 PM Jesper Dangaard Brouer
> > <jbrouer@redhat.com> wrote:
> >>
> >>
> >>
> >> On 25/02/2022 18.41, Joe Damato wrote:
> >> > +#ifdef CONFIG_PAGE_POOL_STATS
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
> >> > +#endif
> >>
> >> The naming: "page_pool_rec_xxx".
> >> What does the "rec" stand for?
> >
> > rec stands for recycle.
> >
> > ethtool strings have a limited size (ETH_GSTRING_LEN - 32 bytes) and
> > the full word "recycle" didn't fit for some of the stats once the
> > queue number is prepended elsewhere in the driver code.
> >
> >> Users of ethtool -S stats... will they know "rec" is "recycle" ?
> >
> > I am open to other names or adding documentation to the driver docs to
> > explain the meaning.
>
> You could shorten the 'page_pool_' prefix to 'ppool_' or even 'pp_' and
> gain some characters that way?

I had considered this, but I thought that 'pp' was possibly as terse as 'rec'.

If you all think these are more clear, I can send a v8 of this series
that changes the strings from the above to this instead:

rx_pp_alloc_fast
rx_pp_alloc_slow
rx_pp_alloc_...

and

rx_pp_recyle_cached
rx_pp_recycle_cache_full
rx_pp_recycle_...

With this name scheme, it looks like all the stat names seem to fit. I
have no idea if this is more or less clear to the user though :)

I'll leave it up to the mlx5 maintainers; I am happy to do whatever
they prefer to get this series in.

Thanks,
Joe

Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats

[Toke Høiland-Jørgensen]

Joe Damato <jdamato@fastly.com> writes:

> On Mon, Feb 28, 2022 at 1:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Joe Damato <jdamato@fastly.com> writes:
>>
>> > On Sun, Feb 27, 2022 at 11:28 PM Jesper Dangaard Brouer
>> > <jbrouer@redhat.com> wrote:
>> >>
>> >>
>> >>
>> >> On 25/02/2022 18.41, Joe Damato wrote:
>> >> > +#ifdef CONFIG_PAGE_POOL_STATS
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
>> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
>> >> > +#endif
>> >>
>> >> The naming: "page_pool_rec_xxx".
>> >> What does the "rec" stand for?
>> >
>> > rec stands for recycle.
>> >
>> > ethtool strings have a limited size (ETH_GSTRING_LEN - 32 bytes) and
>> > the full word "recycle" didn't fit for some of the stats once the
>> > queue number is prepended elsewhere in the driver code.
>> >
>> >> Users of ethtool -S stats... will they know "rec" is "recycle" ?
>> >
>> > I am open to other names or adding documentation to the driver docs to
>> > explain the meaning.
>>
>> You could shorten the 'page_pool_' prefix to 'ppool_' or even 'pp_' and
>> gain some characters that way?
>
> I had considered this, but I thought that 'pp' was possibly as terse as 'rec'.
>
> If you all think these are more clear, I can send a v8 of this series
> that changes the strings from the above to this instead:
>
> rx_pp_alloc_fast
> rx_pp_alloc_slow
> rx_pp_alloc_...
>
> and
>
> rx_pp_recyle_cached
> rx_pp_recycle_cache_full
> rx_pp_recycle_...
>
> With this name scheme, it looks like all the stat names seem to fit. I
> have no idea if this is more or less clear to the user though :)

My thinking was that at least 'pp_' is obviously opaque, so it will be
clear to readers that if they don't know what it is they have to look it
up. Whereas 'rec_' looks like it could be 'record' or something like
that, and it'll make people guess (wrongly).

> I'll leave it up to the mlx5 maintainers; I am happy to do whatever
> they prefer to get this series in.

Right, but this is also going to create precedence for other drivers
that add the page pool-based stats, so spending a bit of time agreeing
on the colour of the bikeshed may be worthwhile here... :)

-Toke

Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats

[Joe Damato]

On Tue, Mar 1, 2022 at 3:23 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Joe Damato <jdamato@fastly.com> writes:
>
> > On Mon, Feb 28, 2022 at 1:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Joe Damato <jdamato@fastly.com> writes:
> >>
> >> > On Sun, Feb 27, 2022 at 11:28 PM Jesper Dangaard Brouer
> >> > <jbrouer@redhat.com> wrote:
> >> >>
> >> >>
> >> >>
> >> >> On 25/02/2022 18.41, Joe Damato wrote:
> >> >> > +#ifdef CONFIG_PAGE_POOL_STATS
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
> >> >> > +     { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
> >> >> > +#endif
> >> >>
> >> >> The naming: "page_pool_rec_xxx".
> >> >> What does the "rec" stand for?
> >> >
> >> > rec stands for recycle.
> >> >
> >> > ethtool strings have a limited size (ETH_GSTRING_LEN - 32 bytes) and
> >> > the full word "recycle" didn't fit for some of the stats once the
> >> > queue number is prepended elsewhere in the driver code.
> >> >
> >> >> Users of ethtool -S stats... will they know "rec" is "recycle" ?
> >> >
> >> > I am open to other names or adding documentation to the driver docs to
> >> > explain the meaning.
> >>
> >> You could shorten the 'page_pool_' prefix to 'ppool_' or even 'pp_' and
> >> gain some characters that way?
> >
> > I had considered this, but I thought that 'pp' was possibly as terse as 'rec'.
> >
> > If you all think these are more clear, I can send a v8 of this series
> > that changes the strings from the above to this instead:
> >
> > rx_pp_alloc_fast
> > rx_pp_alloc_slow
> > rx_pp_alloc_...
> >
> > and
> >
> > rx_pp_recyle_cached
> > rx_pp_recycle_cache_full
> > rx_pp_recycle_...
> >
> > With this name scheme, it looks like all the stat names seem to fit. I
> > have no idea if this is more or less clear to the user though :)
>
> My thinking was that at least 'pp_' is obviously opaque, so it will be
> clear to readers that if they don't know what it is they have to look it
> up. Whereas 'rec_' looks like it could be 'record' or something like
> that, and it'll make people guess (wrongly).

Fair enough.

> > I'll leave it up to the mlx5 maintainers; I am happy to do whatever
> > they prefer to get this series in.
>
> Right, but this is also going to create precedence for other drivers
> that add the page pool-based stats, so spending a bit of time agreeing
> on the colour of the bikeshed may be worthwhile here... :)

Sure, that's fine with me.

I'll give it a few days - hoping to hear back from the mellanox folks
on what they prefer before submitting a v8 with the changes I
mentioned above.

My goal is to avoid a few extra rounds of back and forth if possible :)

Thanks,
Joe