[net-next v6 0/2] page_pool: Add page pool stats

10 messages, 4 authors, 2022-02-23 · open the first message on its own page

[net-next v6 0/2] page_pool: Add page pool stats

From: Joe Damato <hidden>
Date: 2022-02-23 00:03:20

Greetings:

Welcome to v6.

As a reminder: this patch series adds basic per-cpu per-page pool stats
tracking, if enabled at compile time. An API is provided,
page_pool_get_stats, which fills a caller provided page_pool_stats struct
with the totaled stats for the specified page pool. The intention is that
drivers can call this API, if they choose, and export these statistics to
users via ethtool/debugfs/etc. Users can then examine and monitor this
information to get a better sense of how their RX queues, page pools, and
the kernel page allocator are interacting with each other.

This revision contains a single change from v5 based on Jesper's feedback:
the struct page_pool_stats pointer in struct page_pool is now marked as
____cacheline_aligned_in_smp. Its position is unchanged: it remains the
last field in the page_pool struct, but is now located on its own
cache-line. Other locations are possible, such as: sharing the cache-line
with xdp_mem_id, but I didn't hear back on placement preference so I went
with a new cache-line for the page_pool_stats pointer.

With this series applied, but with stats disabled, pahole reports the
following data about struct page_pool:

/* size: 1600, cachelines: 25, members: 15 */
/* sum members: 1436, holes: 2, sum holes: 116 */
/* padding: 48 */

With this series applied, but with stats enabled, pahole shows that the
stats pointer is on its own cache-line:

/* --- cacheline 25 boundary (1600 bytes) --- */
struct page_pool_stats *   stats;                /*  1600     8 */

And, the page_pool struct has grown in size slightly (as expected):

/* size: 1664, cachelines: 26, members: 16 */
/* sum members: 1444, holes: 3, sum holes: 164 */
/* padding: 56 */

I re-ran bench_page_pool_simple and bench_page_pool_cross_cpu with the same
arguments as previous revisions (see below). I also ran
bench_page_pool_cross_cpu a second time, but with 8 returning cpus. Links
to the raw output with this series applied with stats off [1] and stats on
[2] are included for examination. Note that the benchmarks were slightly
modified [3] to produce more verbose output, but no functional changes were
made.

Back to back runs of the same benchmark reveal variability in the results.
The runs below show that the kernel with the extra code enabled is slightly
faster than with a kernel built with this code disabled. Re-running the
benchmarks again on each kernel can (and has) shown different results;
including results which show that disabling stats is slightly faster.

This code is defaulted to disabled unless explicitly enabled
by a user in their kernel configuration. Any additional resource
consumption this code generates is limited to users who have explicitly
opted-in.

Jesper: my apologies if I'm missing something obvious with running the
benchmarks, but please let me know if there is anything else you'd like me
to provide. Also, feel free to let me know if you think this code is not
desired at all -- in which case, I'll stop sending updated revisions :)

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 			13	6.051		13	6.071
lock				31	13.938		32	13.985

no-softirq-page_pool01		75	32.856		74	32.305
no-softirq-page_pool02		75	32.894		74	32.343
no-softirq-page_pool03		107	46.651		107	46.710

tasklet_page_pool01_fast_path	14	6.386		13	5.829
tasklet_page_pool02_ptr_ring	41	17.998		39	17.390
tasklet_page_pool03_slow	107	47.040		107	46.830

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)	4050	1765.133	4090	1782.700
page_pool_cross_cpu CPU(1)	4043	1762.174	4090	1782.636
page_pool_cross_cpu CPU(2)	4050	1765.171	4090	1782.762
page_pool_cross_cpu CPU(3)	4050	1765.074	4087	1781.393
page_pool_cross_cpu CPU(4)	1012	441.293		1022	445.691

page_pool_cross_cpu average	3441	-		3475	-

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)	7458	3250.426	7531	3282.485
page_pool_cross_cpu CPU(1)	7473	3256.957	7531	3282.409
page_pool_cross_cpu CPU(2)	7459	3251.038	7532	3282.635
page_pool_cross_cpu CPU(3)	7473	3257.048	7530	3282.006
page_pool_cross_cpu CPU(4)	7460	3251.341	7531	3282.372
page_pool_cross_cpu CPU(5)	7474	3257.719	7532	3282.831
page_pool_cross_cpu CPU(6)	7462	3252.377	7531	3282.369
page_pool_cross_cpu CPU(7)	7478	3259.245	7532	3282.799
page_pool_cross_cpu CPU(8)	934	407.406		941	410.354

page_pool_cross_cpu average	6741	-		6799	-

Thanks.

[1]:
https://gist.githubusercontent.com/jdamato-fsly/c86d9bb4144fa12dbab41126772b167e/raw/d1fcf995bbc3f44d1362233fab1c18365922c47f/stats_disabled
[2]:
https://gist.githubusercontent.com/jdamato-fsly/c86d9bb4144fa12dbab41126772b167e/raw/d1fcf995bbc3f44d1362233fab1c18365922c47f/stats_enabled
[3]:
https://gist.githubusercontent.com/jdamato-fsly/26ec72238522074515d47011434d054f/raw/0774a02d29e33cd5876dbe240e23c7a118b743da/benchmark.patch

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 (2):
  page_pool: Add page_pool stats
  page_pool: Add function to batch and return stats

 include/net/page_pool.h | 27 +++++++++++++++++++++
 net/Kconfig             | 13 +++++++++++
 net/core/page_pool.c    | 62 +++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 98 insertions(+), 4 deletions(-)

-- 
2.7.4

[net-next v6 1/2] page_pool: Add page_pool stats

From: Joe Damato <hidden>
Date: 2022-02-23 00:03:26

Add per-cpu per-pool statistics counters for the allocation path of a page
pool.

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 <redacted>
---
 include/net/page_pool.h | 18 ++++++++++++++++++
 net/Kconfig             | 13 +++++++++++++
 net/core/page_pool.c    | 37 +++++++++++++++++++++++++++++++++----
 3 files changed, 64 insertions(+), 4 deletions(-)
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 97c3c19..bedc82f 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -135,7 +135,25 @@ struct page_pool {
 	refcount_t user_cnt;
 
 	u64 destroy_cnt;
+#ifdef CONFIG_PAGE_POOL_STATS
+	struct page_pool_stats __percpu *stats ____cacheline_aligned_in_smp;
+#endif
+};
+
+#ifdef CONFIG_PAGE_POOL_STATS
+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 */
+		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 */
+	} alloc;
 };
+#endif
 
 struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
 
diff --git a/net/Kconfig b/net/Kconfig
index 8a1f9d0..8c07308 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 allocations. This option
+	  incurs additional CPU cost in allocation 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..f29dff9 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -26,6 +26,17 @@
 
 #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)
 {
@@ -73,6 +84,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;
 
@@ -117,8 +134,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.
@@ -145,14 +164,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);
+			this_cpu_inc_alloc_stat(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];
+		this_cpu_inc_alloc_stat(pool, refill);
+	}
 
 	return page;
 }
@@ -166,6 +188,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);
 	}
@@ -239,6 +262,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' */
@@ -293,10 +317,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;
@@ -620,6 +646,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 v6 2/2] page_pool: Add function to batch and return stats

From: Joe Damato <hidden>
Date: 2022-02-23 00:03:27

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 <redacted>
---
 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 bedc82f..66c0634 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.
+ */
+bool 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 f29dff9..9cad108 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -33,6 +33,31 @@
 		struct page_pool_stats __percpu *s = pool->stats;	\
 		__this_cpu_inc(s->alloc.__stat);			\
 	} while (0)
+
+bool page_pool_get_stats(struct page_pool *pool,
+			 struct page_pool_stats *stats)
+{
+	int cpu = 0;
+
+	if (!stats)
+		return false;
+
+	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 true;
+}
+EXPORT_SYMBOL(page_pool_get_stats);
 #else
 #define this_cpu_inc_alloc_stat(pool, __stat)
 #endif
-- 
2.7.4

Re: [net-next v6 1/2] page_pool: Add page_pool stats

From: Jesper Dangaard Brouer <hidden>
Date: 2022-02-23 16:06:06


On 23/02/2022 01.00, Joe Damato wrote:
quoted hunk
Add per-cpu per-pool statistics counters for the allocation path of a page
pool.

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 <redacted>
---
  include/net/page_pool.h | 18 ++++++++++++++++++
  net/Kconfig             | 13 +++++++++++++
  net/core/page_pool.c    | 37 +++++++++++++++++++++++++++++++++----
  3 files changed, 64 insertions(+), 4 deletions(-)
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 97c3c19..bedc82f 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -135,7 +135,25 @@ struct page_pool {
  	refcount_t user_cnt;
  
  	u64 destroy_cnt;
+#ifdef CONFIG_PAGE_POOL_STATS
+	struct page_pool_stats __percpu *stats ____cacheline_aligned_in_smp;
+#endif
+};
Adding this to the end of the struct and using attribute 
____cacheline_aligned_in_smp cause the structure have a lot of wasted 
padding in the end.

I recommend using the tool pahole to see the struct layout.

+
+#ifdef CONFIG_PAGE_POOL_STATS
+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 */
+		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 */
+	} alloc;
  };
+#endif
All of these stats are for page_pool allocation "RX" side, which is 
protected by softirq/NAPI.
Thus, I find it unnecessary to do __percpu stats.


As Ilias have pointed out-before, the __percpu stats (first) becomes 
relevant once we want stats for the free/"return" path ... which is not 
part of this patchset.

--Jesper

Re: [net-next v6 1/2] page_pool: Add page_pool stats

From: Joe Damato <hidden>
Date: 2022-02-23 17:05:32

On Wed, Feb 23, 2022 at 8:06 AM Jesper Dangaard Brouer
[off-list ref] wrote:


On 23/02/2022 01.00, Joe Damato wrote:
quoted
Add per-cpu per-pool statistics counters for the allocation path of a page
pool.

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 <redacted>
---
  include/net/page_pool.h | 18 ++++++++++++++++++
  net/Kconfig             | 13 +++++++++++++
  net/core/page_pool.c    | 37 +++++++++++++++++++++++++++++++++----
  3 files changed, 64 insertions(+), 4 deletions(-)
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 97c3c19..bedc82f 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -135,7 +135,25 @@ struct page_pool {
      refcount_t user_cnt;

      u64 destroy_cnt;
+#ifdef CONFIG_PAGE_POOL_STATS
+     struct page_pool_stats __percpu *stats ____cacheline_aligned_in_smp;
+#endif
+};
Adding this to the end of the struct and using attribute
____cacheline_aligned_in_smp cause the structure have a lot of wasted
padding in the end.

I recommend using the tool pahole to see the struct layout.
I mentioned placement near xdp_mem_id in the cover letter for this
code and in my reply on the v5 [1] , but I didn't hear back, so I
really had no idea what you preferred.

I'll move it near xdp_mem_id for the v7, and hope that's what you are
saying here.
quoted
+
+#ifdef CONFIG_PAGE_POOL_STATS
+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 */
+             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 */
+     } alloc;
  };
+#endif
All of these stats are for page_pool allocation "RX" side, which is
protected by softirq/NAPI.
Yes.
Thus, I find it unnecessary to do __percpu stats.
Unnecessary, sure, but doesn't seem harmful and it allows us to expand
this to other stats in a future change.
As Ilias have pointed out-before, the __percpu stats (first) becomes
relevant once we want stats for the free/"return" path ... which is not
part of this patchset.
Does that need to be covered in this patchset?

I believe Ilias' response which mentioned the recycle stats indicates
that they can be added in the future [2]. I took this to mean a
separate patchset, assuming that this first change is accepted.

As I understand your comment, then: this change will not be accepted
unless it is expanded to include recycle stats or if the per-cpu
designation is removed.

Are the cache-line placement and per-cpu designations the only
remaining issues with this change?

[1]: https://lore.kernel.org/all/CALALjgzUQUuEVkNXous0kOcHHqiSrTem+n9MjQh6q-8+Azi-sg@mail.gmail.com/
[2]: https://lore.kernel.org/all/YfJhIpBGW6suBwkY@hades/

Re: [net-next v6 1/2] page_pool: Add page_pool stats

From: Jakub Kicinski <kuba@kernel.org>
Date: 2022-02-23 17:40:17

On Wed, 23 Feb 2022 09:05:06 -0800 Joe Damato wrote:
Are the cache-line placement and per-cpu designations the only
remaining issues with this change?
page_pool_get_stats() has no callers as is, I'm not sure how we can
merge it in current form. 

Maybe I'm missing bigger picture or some former discussion.

Re: [net-next v6 1/2] page_pool: Add page_pool stats

From: Joe Damato <hidden>
Date: 2022-02-23 17:45:23

On Wed, Feb 23, 2022 at 9:40 AM Jakub Kicinski [off-list ref] wrote:
On Wed, 23 Feb 2022 09:05:06 -0800 Joe Damato wrote:
quoted
Are the cache-line placement and per-cpu designations the only
remaining issues with this change?
page_pool_get_stats() has no callers as is, I'm not sure how we can
merge it in current form.

Maybe I'm missing bigger picture or some former discussion.
I wrote the mlx5 code to call this function and export the data via
ethtool. I had assumed the mlx5 changes should be in a separate
patchset. I can include that code as part of this change, if needed.

Thanks,
Joe

Re: [net-next v6 1/2] page_pool: Add page_pool stats

From: Jesper Dangaard Brouer <hidden>
Date: 2022-02-23 18:10:24


On 23/02/2022 18.45, Joe Damato wrote:
On Wed, Feb 23, 2022 at 9:40 AM Jakub Kicinski [off-list ref] wrote:
quoted
On Wed, 23 Feb 2022 09:05:06 -0800 Joe Damato wrote:
quoted
Are the cache-line placement and per-cpu designations the only
remaining issues with this change?
page_pool_get_stats() has no callers as is, I'm not sure how we can
merge it in current form.

Maybe I'm missing bigger picture or some former discussion.
I wrote the mlx5 code to call this function and export the data via
ethtool. I had assumed the mlx5 changes should be in a separate
patchset. I can include that code as part of this change, if needed.
I agree with Jakub we need to see how this is used by drivers.

--Jesper

Re: [net-next v6 1/2] page_pool: Add page_pool stats

From: Joe Damato <hidden>
Date: 2022-02-23 18:17:56

On Wed, Feb 23, 2022 at 10:10 AM Jesper Dangaard Brouer
[off-list ref] wrote:


On 23/02/2022 18.45, Joe Damato wrote:
quoted
On Wed, Feb 23, 2022 at 9:40 AM Jakub Kicinski [off-list ref] wrote:
quoted
On Wed, 23 Feb 2022 09:05:06 -0800 Joe Damato wrote:
quoted
Are the cache-line placement and per-cpu designations the only
remaining issues with this change?
page_pool_get_stats() has no callers as is, I'm not sure how we can
merge it in current form.

Maybe I'm missing bigger picture or some former discussion.
I wrote the mlx5 code to call this function and export the data via
ethtool. I had assumed the mlx5 changes should be in a separate
patchset. I can include that code as part of this change, if needed.
I agree with Jakub we need to see how this is used by drivers.
OK. I'll include the mlx5 code which uses this API in the v7.

Re: [net-next v6 1/2] page_pool: Add page_pool stats

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Date: 2022-02-23 19:02:00

Hi all

[...]
quoted
Signed-off-by: Joe Damato <redacted>
---
  include/net/page_pool.h | 18 ++++++++++++++++++
  net/Kconfig             | 13 +++++++++++++
  net/core/page_pool.c    | 37 +++++++++++++++++++++++++++++++++----
  3 files changed, 64 insertions(+), 4 deletions(-)
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 97c3c19..bedc82f 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -135,7 +135,25 @@ struct page_pool {
      refcount_t user_cnt;

      u64 destroy_cnt;
+#ifdef CONFIG_PAGE_POOL_STATS
+     struct page_pool_stats __percpu *stats ____cacheline_aligned_in_smp;
+#endif
+};
Adding this to the end of the struct and using attribute
____cacheline_aligned_in_smp cause the structure have a lot of wasted
padding in the end.

I recommend using the tool pahole to see the struct layout.

quoted
+
+#ifdef CONFIG_PAGE_POOL_STATS
+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 */
+             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 */
+     } alloc;
  };
+#endif
All of these stats are for page_pool allocation "RX" side, which is
protected by softirq/NAPI.
Thus, I find it unnecessary to do __percpu stats.


As Ilias have pointed out-before, the __percpu stats (first) becomes
relevant once we want stats for the free/"return" path ... which is not
part of this patchset.
Do we really mind though?  The point was to have the percpu variables
in order to plug in any stats that weren't napi-protected.  I think we
can always plug in the recycling stats later?  OTOH if you prefer
having them in now I can work with Joe and we can get that supported
as well.

Cheers
/Ilias
--Jesper
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help