[net-next v4 00/11] page_pool: Add page_pool stat counters

[net-next v4 00/11] page_pool: Add page_pool stat counters

[Joe Damato]

Greetings:

Welcome to v4.

This revision changes stats to be per-cpu and per-pool after getting
feedback from Ilias, Tariq, and Jesper.

This revision exposes a single API: page_pool_get_stats which fills in a
caller provided struct page_pool_stats with the summed per-cpu stats for
the specified pool. The intention is that drivers will use this API to
access pool stats so that they can be exposed to the user via ethtool,
debugfs, etc.

I chose this method because pulling struct page_pool_stats into page_pool.c
would require exposing a large number of APIs for accessing individual
field values. The obvious downside is that changing the struct fields could
break driver code. I don't have a strong sense for what the page_pool
maintainers would prefer, so I chose the simplest thing for v4.

I re-ran the benchmarks using a larger number of loops and cpus, as
documented below. I've summarized the results below; links to the raw
output with this series applied with stats off [1] and stats on [2] are
included 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.209		13	6.027
lock				31	13.571		31	13.935

no-softirq-page_pool01		70	30.783		68	29.959
no-softirq-page_pool02		73	32.124		72	31.412
no-softirq-page_pool03		112	49.060		108	47.373

tasklet_page_pool01_fast_path	14	6.413		13	5.833
tasklet_page_pool02_ptr_ring	41	17.939		39	17.370
tasklet_page_pool03_slow	110	48.179		108	47.173

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)	4101	1787.639	3990	1739.178
page_pool_cross_cpu CPU(1)	4107	1790.281	3987	1737.756
page_pool_cross_cpu CPU(2)	4102	1787.777	3991	1739.731
page_pool_cross_cpu CPU(3)	4108	1790.455	3991	1739.703
page_pool_cross_cpu CPU(4)	1027	447.614		997	434.933

page_pool_cross_cpu average	3489	-		3391	-

Thanks.

[1]: https://gist.githubusercontent.com/jdamato-fsly/343814d33ed75372e98782b796b607a4/raw/9f440a0b38f3eff37b76b914934620625172a0f4/v4%2520with%2520stats%2520off
[2]: https://gist.githubusercontent.com/jdamato-fsly/343814d33ed75372e98782b796b607a4/raw/9f440a0b38f3eff37b76b914934620625172a0f4/v4%2520with%2520stats%2520on

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 (11):
  page_pool: kconfig: Add flag for page pool stats
  page_pool: Add per-pool-per-cpu struct.
  page_pool: Allocate and free stats structure
  page_pool: Add macro for incrementing alloc stats
  page_pool: Add fast path stat
  page_pool: Add slow path order-0 stat
  page_pool: Add slow path high order alloc stat
  page_pool: Add stat tracking empty ring
  page_pool: Add stat tracking cache refill
  page_pool: Add a stat tracking waived pages
  page_pool: Add function to batch and return stats

 include/net/page_pool.h | 27 +++++++++++++++++++++
 net/Kconfig             | 12 ++++++++++
 net/core/page_pool.c    | 64 +++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 99 insertions(+), 4 deletions(-)

-- 
2.7.4

[net-next v4 01/11] page_pool: kconfig: Add flag for page pool stats

[Joe Damato]

Control enabling / disabling page_pool_stats with a kernel config option.
Option is defaulted to N.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 net/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/Kconfig b/net/Kconfig
index 8a1f9d0..604b3eb 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -434,6 +434,18 @@ 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 allocations. Stats are exported
+	  to the file /proc/net/page_pool_stat. Users can examine these
+	  stats to better understand how their drivers and the kernel's
+	  page allocator, and the page pool interact with each other.
+
+	  If unsure, say N.
+
 config FAILOVER
 	tristate "Generic failover module"
 	help
-- 
2.7.4

[net-next v4 02/11] page_pool: Add per-pool-per-cpu struct.

[Joe Damato]

A per pool per cpu struct is added. Empty for now as members will be added
next.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 include/net/page_pool.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 79a8055..93e587d 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -135,7 +135,15 @@ struct page_pool {
 	refcount_t user_cnt;
 
 	u64 destroy_cnt;
+#ifdef CONFIG_PAGE_POOL_STATS
+	struct page_pool_stats __percpu *stats;
+#endif
+};
+
+#ifdef CONFIG_PAGE_POOL_STATS
+struct page_pool_stats {
 };
+#endif
 
 struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
 
-- 
2.7.4

[net-next v4 03/11] page_pool: Allocate and free stats structure

[Joe Damato]

This commit only alloc_percpu and free_percpu for the stats
structure.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 net/core/page_pool.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index bd62c01..5f822b0 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -73,6 +73,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->stats = alloc_percpu(struct page_pool_stats);
+	if (!pool->stats)
+		return -ENOMEM;
+#endif
+
 	if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0)
 		return -ENOMEM;
 
@@ -623,6 +629,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->stats);
+#endif
 	kfree(pool);
 }
 
-- 
2.7.4

[net-next v4 04/11] page_pool: Add macro for incrementing alloc stats

[Joe Damato]

Add macro incrementing per pool per cpu stats.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 net/core/page_pool.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 5f822b0..180e48b 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -26,6 +26,19 @@
 
 #define BIAS_MAX	LONG_MAX
 
+#ifdef CONFIG_PAGE_POOL_STATS
+/*
+ * this_cpu_inc_alloc_stat is intended to be used in softirq context
+ */
+#define this_cpu_inc_alloc_stat(pool, __stat)				\
+	do {								\
+		struct page_pool_stats __percpu *s = pool->stats;	\
+		__this_cpu_inc(s->alloc.__stat);			\
+	} while (0)
+#else
+#define this_cpu_inc_alloc_stat(pool, __stat)
+#endif
+
 static int page_pool_init(struct page_pool *pool,
 			  const struct page_pool_params *params)
 {
-- 
2.7.4

[net-next v4 05/11] page_pool: Add fast path stat

[Joe Damato]

Increment the pool's fast path allocation stat when this event occurs.

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

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 93e587d..864a480 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -142,6 +142,9 @@ struct page_pool {
 
 #ifdef CONFIG_PAGE_POOL_STATS
 struct page_pool_stats {
+	struct {
+		u64 fast; /* fast path allocations */
+	} alloc;
 };
 #endif
 
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 180e48b..b5bf41f 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -185,6 +185,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];
+		this_cpu_inc_alloc_stat(pool, fast);
 	} else {
 		page = page_pool_refill_alloc_cache(pool);
 	}
-- 
2.7.4

[net-next v4 06/11] page_pool: Add slow path order-0 stat

[Joe Damato]

Increment slow path order-0 allocation stat when this event occurs.

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

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 864a480..7d5f202 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -144,6 +144,7 @@ struct page_pool {
 struct page_pool_stats {
 	struct {
 		u64 fast; /* fast path allocations */
+		u64 slow; /* slow-path order 0 allocations */
 	} alloc;
 };
 #endif
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index b5bf41f..264d8c9 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -313,10 +313,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
+		this_cpu_inc_alloc_stat(pool, slow);
+	} else {
 		page = NULL;
+	}
 
 	/* When page just alloc'ed is should/must have refcnt 1. */
 	return page;
-- 
2.7.4

[net-next v4 07/11] page_pool: Add slow path high order alloc stat

[Joe Damato]

Track high order allocations in the slow path which cause an interaction
with the buddy allocator.

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

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 7d5f202..0c4cb49 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -145,6 +145,7 @@ struct page_pool_stats {
 	struct {
 		u64 fast; /* fast path allocations */
 		u64 slow; /* slow-path order 0 allocations */
+		u64 slow_high_order; /* slow-path high order allocations */
 	} alloc;
 };
 #endif
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 264d8c9..b7d0995 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -259,6 +259,7 @@ static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
 		return NULL;
 	}
 
+	this_cpu_inc_alloc_stat(pool, slow_high_order);
 	page_pool_set_pp_info(pool, page);
 
 	/* Track how many pages are held 'in-flight' */
-- 
2.7.4

[net-next v4 08/11] page_pool: Add stat tracking empty ring

[Joe Damato]

Add a stat tracking when the ptr ring is empty. When this occurs, the cache
could not be refilled and a slow path allocation was forced.

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

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 0c4cb49..b243480 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -146,6 +146,9 @@ struct page_pool_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
+			    */
 	} alloc;
 };
 #endif
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index b7d0995..b25ded1 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -136,8 +136,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)) {
+		this_cpu_inc_alloc_stat(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.
-- 
2.7.4

[net-next v4 09/11] page_pool: Add stat tracking cache refill

[Joe Damato]

Add a stat tracking succesfull allocations which triggered a refill.

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

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index b243480..65cd0ca 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -149,6 +149,7 @@ struct page_pool_stats {
 		u64 empty; /* failed refills due to empty ptr ring, forcing
 			    * slow path allocation
 			    */
+		u64 refill; /* allocations via successful refill */
 	} alloc;
 };
 #endif
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index b25ded1..4fe48ec 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -172,8 +172,10 @@ static struct page *page_pool_refill_alloc_cache(struct page_pool *pool)
 	} 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];
+		this_cpu_inc_alloc_stat(pool, refill);
+	}
 
 	return page;
 }
-- 
2.7.4

[net-next v4 10/11] page_pool: Add a stat tracking waived pages

[Joe Damato]

Track how often pages obtained from the ring cannot be added to the cache
because of a NUMA mismatch.

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

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 65cd0ca..bb87706 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -150,6 +150,7 @@ struct page_pool_stats {
 			    * slow path allocation
 			    */
 		u64 refill; /* allocations via successful refill */
+		u64 waive;  /* failed refills due to numa zone mismatch */
 	} alloc;
 };
 #endif
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 4fe48ec..0bd084c 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -166,6 +166,7 @@ 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);
+			this_cpu_inc_alloc_stat(pool, waive);
 			page = NULL;
 			break;
 		}
-- 
2.7.4

[net-next v4 11/11] 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
the batched stats for a specified page pool.

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

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index bb87706..5257e46 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -153,6 +153,15 @@ struct page_pool_stats {
 		u64 waive;  /* failed refills due to numa zone mismatch */
 	} alloc;
 };
+
+/*
+ * 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 and call page_pool_get_stats to get the batched pcpu
+ * stats.
+ */
+struct page_pool_stats *page_pool_get_stats(struct page_pool *pool,
+					    struct page_pool_stats *stats);
 #endif
 
 struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 0bd084c..076593bb 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -35,6 +35,31 @@
 		struct page_pool_stats __percpu *s = pool->stats;	\
 		__this_cpu_inc(s->alloc.__stat);			\
 	} while (0)
+
+struct page_pool_stats *page_pool_get_stats(struct page_pool *pool,
+					    struct page_pool_stats *stats)
+{
+	int cpu = 0;
+
+	if (!stats)
+		return NULL;
+
+	for_each_possible_cpu(cpu) {
+		const struct page_pool_stats *pcpu =
+			per_cpu_ptr(pool->stats, cpu);
+
+		stats->alloc.fast += pcpu->alloc.fast;
+		stats->alloc.slow += pcpu->alloc.slow;
+		stats->alloc.slow_high_order +=
+			pcpu->alloc.slow_high_order;
+		stats->alloc.empty += pcpu->alloc.empty;
+		stats->alloc.refill += pcpu->alloc.refill;
+		stats->alloc.waive += pcpu->alloc.waive;
+	}
+
+	return stats;
+}
+EXPORT_SYMBOL(page_pool_get_stats);
 #else
 #define this_cpu_inc_alloc_stat(pool, __stat)
 #endif
-- 
2.7.4

Re: [net-next v4 01/11] page_pool: kconfig: Add flag for page pool stats

[Ilias Apalodimas]

On Thu, Feb 03, 2022 at 04:09:23PM -0800, Joe Damato wrote:
> Control enabling / disabling page_pool_stats with a kernel config option.
> Option is defaulted to N.
> 
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>  net/Kconfig | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/Kconfig b/net/Kconfig
> index 8a1f9d0..604b3eb 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -434,6 +434,18 @@ 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 allocations. Stats are exported
> +	  to the file /proc/net/page_pool_stat. Users can examine these

There's no proc anymore

> +	  stats to better understand how their drivers and the kernel's
> +	  page allocator, and the page pool interact with each other.
> +
> +	  If unsure, say N.
> +
>  config FAILOVER
>  	tristate "Generic failover module"
>  	help
> -- 
> 2.7.4
> 

Regards
/Ilias

Re: [net-next v4 10/11] page_pool: Add a stat tracking waived pages

[Ilias Apalodimas]

Hi Joe, 

On Thu, Feb 03, 2022 at 04:09:32PM -0800, Joe Damato wrote:
> Track how often pages obtained from the ring cannot be added to the cache
> because of a NUMA mismatch.
> 
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>  include/net/page_pool.h | 1 +
>  net/core/page_pool.c    | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index 65cd0ca..bb87706 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -150,6 +150,7 @@ struct page_pool_stats {
>  			    * slow path allocation
>  			    */
>  		u64 refill; /* allocations via successful refill */
> +		u64 waive;  /* failed refills due to numa zone mismatch */
>  	} alloc;
>  };
>  #endif
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 4fe48ec..0bd084c 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -166,6 +166,7 @@ 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);
> +			this_cpu_inc_alloc_stat(pool, waive);
>  			page = NULL;
>  			break;
>  		}
> -- 
> 2.7.4
> 

Personally i'd find it easier to read if patches 1-10 were squashed in a
single commit. 

Regards
/Ilias

Re: [net-next v4 11/11] page_pool: Add function to batch and return stats

[Ilias Apalodimas]

On Thu, Feb 03, 2022 at 04:09:33PM -0800, Joe Damato wrote:
> Adds a function page_pool_get_stats which can be used by drivers to obtain
> the batched stats for a specified page pool.
> 
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>  include/net/page_pool.h |  9 +++++++++
>  net/core/page_pool.c    | 25 +++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index bb87706..5257e46 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -153,6 +153,15 @@ struct page_pool_stats {
>  		u64 waive;  /* failed refills due to numa zone mismatch */
>  	} alloc;
>  };
> +
> +/*
> + * 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 and call page_pool_get_stats to get the batched pcpu
> + * stats.
> + */
> +struct page_pool_stats *page_pool_get_stats(struct page_pool *pool,
> +					    struct page_pool_stats *stats);
>  #endif
>  
>  struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 0bd084c..076593bb 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -35,6 +35,31 @@
>  		struct page_pool_stats __percpu *s = pool->stats;	\
>  		__this_cpu_inc(s->alloc.__stat);			\
>  	} while (0)
> +
> +struct page_pool_stats *page_pool_get_stats(struct page_pool *pool,
> +					    struct page_pool_stats *stats)
> +{
> +	int cpu = 0;
> +
> +	if (!stats)
> +		return NULL;
> +
> +	for_each_possible_cpu(cpu) {
> +		const struct page_pool_stats *pcpu =
> +			per_cpu_ptr(pool->stats, cpu);
> +
> +		stats->alloc.fast += pcpu->alloc.fast;
> +		stats->alloc.slow += pcpu->alloc.slow;
> +		stats->alloc.slow_high_order +=
> +			pcpu->alloc.slow_high_order;
> +		stats->alloc.empty += pcpu->alloc.empty;
> +		stats->alloc.refill += pcpu->alloc.refill;
> +		stats->alloc.waive += pcpu->alloc.waive;
> +	}
> +
> +	return stats;
> +}
> +EXPORT_SYMBOL(page_pool_get_stats);

You don't really need to return a pointer here. Just make the return code a
bool 

Regards
/Ilias
>  #else
>  #define this_cpu_inc_alloc_stat(pool, __stat)
>  #endif
> -- 
> 2.7.4
> 

Re: [net-next v4 00/11] page_pool: Add page_pool stat counters

[Toke Høiland-Jørgensen]

Joe Damato <jdamato@fastly.com> writes:

> Greetings:
>
> Welcome to v4.
>
> This revision changes stats to be per-cpu and per-pool after getting
> feedback from Ilias, Tariq, and Jesper.

FYI, Jesper is on holiday until the 12th, so it may be be a little while
until he gets back to you on this :)

-Toke

Re: [net-next v4 01/11] page_pool: kconfig: Add flag for page pool stats

[Joe Damato]

On Thu, Feb 3, 2022 at 11:42 PM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Thu, Feb 03, 2022 at 04:09:23PM -0800, Joe Damato wrote:
> > Control enabling / disabling page_pool_stats with a kernel config option.
> > Option is defaulted to N.
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> >  net/Kconfig | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/net/Kconfig b/net/Kconfig
> > index 8a1f9d0..604b3eb 100644
> > --- a/net/Kconfig
> > +++ b/net/Kconfig
> > @@ -434,6 +434,18 @@ 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 allocations. Stats are exported
> > +       to the file /proc/net/page_pool_stat. Users can examine these
>
> There's no proc anymore

Ah, yes. Thanks :) I've fixed this in my v5 branch.

Re: [net-next v4 10/11] page_pool: Add a stat tracking waived pages

[Joe Damato]

On Thu, Feb 3, 2022 at 11:44 PM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Joe,
>
> On Thu, Feb 03, 2022 at 04:09:32PM -0800, Joe Damato wrote:
> > Track how often pages obtained from the ring cannot be added to the cache
> > because of a NUMA mismatch.
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> >  include/net/page_pool.h | 1 +
> >  net/core/page_pool.c    | 1 +
> >  2 files changed, 2 insertions(+)
> >
> > diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> > index 65cd0ca..bb87706 100644
> > --- a/include/net/page_pool.h
> > +++ b/include/net/page_pool.h
> > @@ -150,6 +150,7 @@ struct page_pool_stats {
> >                           * slow path allocation
> >                           */
> >               u64 refill; /* allocations via successful refill */
> > +             u64 waive;  /* failed refills due to numa zone mismatch */
> >       } alloc;
> >  };
> >  #endif
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index 4fe48ec..0bd084c 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -166,6 +166,7 @@ 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);
> > +                     this_cpu_inc_alloc_stat(pool, waive);
> >                       page = NULL;
> >                       break;
> >               }
> > --
> > 2.7.4
> >
>
> Personally i'd find it easier to read if patches 1-10 were squashed in a
> single commit.

Thanks for the feedback. I've squashed patches 1-10 to a single commit
in my v5 branch.

Re: [net-next v4 11/11] page_pool: Add function to batch and return stats

[Joe Damato]

On Thu, Feb 3, 2022 at 11:46 PM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Thu, Feb 03, 2022 at 04:09:33PM -0800, Joe Damato wrote:
> > Adds a function page_pool_get_stats which can be used by drivers to obtain
> > the batched stats for a specified page pool.
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> >  include/net/page_pool.h |  9 +++++++++
> >  net/core/page_pool.c    | 25 +++++++++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> > index bb87706..5257e46 100644
> > --- a/include/net/page_pool.h
> > +++ b/include/net/page_pool.h
> > @@ -153,6 +153,15 @@ struct page_pool_stats {
> >               u64 waive;  /* failed refills due to numa zone mismatch */
> >       } alloc;
> >  };
> > +
> > +/*
> > + * 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 and call page_pool_get_stats to get the batched pcpu
> > + * stats.
> > + */
> > +struct page_pool_stats *page_pool_get_stats(struct page_pool *pool,
> > +                                         struct page_pool_stats *stats);
> >  #endif
> >
> >  struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index 0bd084c..076593bb 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -35,6 +35,31 @@
> >               struct page_pool_stats __percpu *s = pool->stats;       \
> >               __this_cpu_inc(s->alloc.__stat);                        \
> >       } while (0)
> > +
> > +struct page_pool_stats *page_pool_get_stats(struct page_pool *pool,
> > +                                         struct page_pool_stats *stats)
> > +{
> > +     int cpu = 0;
> > +
> > +     if (!stats)
> > +             return NULL;
> > +
> > +     for_each_possible_cpu(cpu) {
> > +             const struct page_pool_stats *pcpu =
> > +                     per_cpu_ptr(pool->stats, cpu);
> > +
> > +             stats->alloc.fast += pcpu->alloc.fast;
> > +             stats->alloc.slow += pcpu->alloc.slow;
> > +             stats->alloc.slow_high_order +=
> > +                     pcpu->alloc.slow_high_order;
> > +             stats->alloc.empty += pcpu->alloc.empty;
> > +             stats->alloc.refill += pcpu->alloc.refill;
> > +             stats->alloc.waive += pcpu->alloc.waive;
> > +     }
> > +
> > +     return stats;
> > +}
> > +EXPORT_SYMBOL(page_pool_get_stats);
>
> You don't really need to return a pointer here. Just make the return code a
> bool

OK. Updated page_pool_get_stats to return a bool in my v5 branch.

Re: [net-next v4 00/11] page_pool: Add page_pool stat counters

[Joe Damato]

On Fri, Feb 4, 2022 at 5:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Joe Damato <jdamato@fastly.com> writes:
>
> > Greetings:
> >
> > Welcome to v4.
> >
> > This revision changes stats to be per-cpu and per-pool after getting
> > feedback from Ilias, Tariq, and Jesper.
>
> FYI, Jesper is on holiday until the 12th, so it may be be a little while
> until he gets back to you on this :)

Thanks for letting me know.

I've queued some changes per Ilias' feedback for the v5, but perhaps I
should hold off on submitting those until Jesper has returned and has
a chance to take a look at this revision.

Thanks,
Joe