[PATCH net-next] net: page_pool: add pages and released_pages counters

[PATCH net-next] net: page_pool: add pages and released_pages counters

[Lorenzo Bianconi]

Introduce pages and released_pages counters to page_pool ethtool stats
in order to track the number of allocated and released pages from the
pool.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 Documentation/networking/page_pool.rst | 2 ++
 include/net/page_pool.h                | 2 ++
 net/core/page_pool.c                   | 8 ++++++++
 3 files changed, 12 insertions(+)

diff --git a/Documentation/networking/page_pool.rst b/Documentation/networking/page_pool.rst
index 30f1344e7cca..71b3eb4555f1 100644
--- a/Documentation/networking/page_pool.rst
+++ b/Documentation/networking/page_pool.rst
@@ -138,6 +138,7 @@ The ``struct page_pool_alloc_stats`` has the following fields:
   * ``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.
+  * ``pages``: number of allocated pages to the pool
 
 The ``struct page_pool_recycle_stats`` has the following fields:
   * ``cached``: recycling placed page in the page pool cache
@@ -145,6 +146,7 @@ The ``struct page_pool_recycle_stats`` has the following fields:
   * ``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
+  * ``released_pages``: number of released pages from the pool
 
 Coding examples
 ===============
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index ddfa0b328677..b16d5320d963 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -94,6 +94,7 @@ struct page_pool_alloc_stats {
 		    */
 	u64 refill; /* allocations via successful refill */
 	u64 waive;  /* failed refills due to numa zone mismatch */
+	u64 pages; /* number of allocated pages to the pool */
 };
 
 struct page_pool_recycle_stats {
@@ -106,6 +107,7 @@ struct page_pool_recycle_stats {
 	u64 released_refcnt; /* page released because of elevated
 			      * refcnt
 			      */
+	u64 released_pages; /* number of released pages from the pool */
 };
 
 /* This struct wraps the above stats structs so users of the
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 193c18799865..418d443d7e7c 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -50,11 +50,13 @@ static const char pp_stats[][ETH_GSTRING_LEN] = {
 	"rx_pp_alloc_empty",
 	"rx_pp_alloc_refill",
 	"rx_pp_alloc_waive",
+	"rx_pp_alloc_pages",
 	"rx_pp_recycle_cached",
 	"rx_pp_recycle_cache_full",
 	"rx_pp_recycle_ring",
 	"rx_pp_recycle_ring_full",
 	"rx_pp_recycle_released_ref",
+	"rx_pp_released_pages",
 };
 
 bool page_pool_get_stats(struct page_pool *pool,
@@ -72,6 +74,7 @@ bool page_pool_get_stats(struct page_pool *pool,
 	stats->alloc_stats.empty += pool->alloc_stats.empty;
 	stats->alloc_stats.refill += pool->alloc_stats.refill;
 	stats->alloc_stats.waive += pool->alloc_stats.waive;
+	stats->alloc_stats.pages += pool->alloc_stats.pages;
 
 	for_each_possible_cpu(cpu) {
 		const struct page_pool_recycle_stats *pcpu =
@@ -82,6 +85,7 @@ bool page_pool_get_stats(struct page_pool *pool,
 		stats->recycle_stats.ring += pcpu->ring;
 		stats->recycle_stats.ring_full += pcpu->ring_full;
 		stats->recycle_stats.released_refcnt += pcpu->released_refcnt;
+		stats->recycle_stats.released_pages += pcpu->released_pages;
 	}
 
 	return true;
@@ -117,11 +121,13 @@ u64 *page_pool_ethtool_stats_get(u64 *data, void *stats)
 	*data++ = pool_stats->alloc_stats.empty;
 	*data++ = pool_stats->alloc_stats.refill;
 	*data++ = pool_stats->alloc_stats.waive;
+	*data++ = pool_stats->alloc_stats.pages;
 	*data++ = pool_stats->recycle_stats.cached;
 	*data++ = pool_stats->recycle_stats.cache_full;
 	*data++ = pool_stats->recycle_stats.ring;
 	*data++ = pool_stats->recycle_stats.ring_full;
 	*data++ = pool_stats->recycle_stats.released_refcnt;
+	*data++ = pool_stats->recycle_stats.released_pages;
 
 	return data;
 }
@@ -411,6 +417,7 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 		pool->pages_state_hold_cnt++;
 		trace_page_pool_state_hold(pool, page,
 					   pool->pages_state_hold_cnt);
+		alloc_stat_inc(pool, pages);
 	}
 
 	/* Return last page */
@@ -493,6 +500,7 @@ void page_pool_release_page(struct page_pool *pool, struct page *page)
 	 */
 	count = atomic_inc_return_relaxed(&pool->pages_state_release_cnt);
 	trace_page_pool_state_release(pool, page, count);
+	recycle_stat_inc(pool, released_pages);
 }
 EXPORT_SYMBOL(page_pool_release_page);
 
-- 
2.39.2

Re: [PATCH net-next] net: page_pool: add pages and released_pages counters

[Jakub Kicinski]

On Thu, 13 Apr 2023 23:46:03 +0200 Lorenzo Bianconi wrote:
> @@ -411,6 +417,7 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
>  		pool->pages_state_hold_cnt++;
>  		trace_page_pool_state_hold(pool, page,
>  					   pool->pages_state_hold_cnt);
> +		alloc_stat_inc(pool, pages);
>  	}
>  
>  	/* Return last page */

What about high order? If we use bulk API for high order one day, 
will @slow_high_order not count calls like @slow does? So we should
bump the new counter for high order, too.

Which makes it very similar to pages_state_hold_cnt, just 64bit...

Re: [PATCH net-next] net: page_pool: add pages and released_pages counters

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

> On Thu, 13 Apr 2023 23:46:03 +0200 Lorenzo Bianconi wrote:
> > @@ -411,6 +417,7 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
> >  		pool->pages_state_hold_cnt++;
> >  		trace_page_pool_state_hold(pool, page,
> >  					   pool->pages_state_hold_cnt);
> > +		alloc_stat_inc(pool, pages);
> >  	}
> >  
> >  	/* Return last page */
> 
> What about high order? If we use bulk API for high order one day, 
> will @slow_high_order not count calls like @slow does? So we should
> bump the new counter for high order, too.

yes, right. AFAIU "slow_high_order" and "slow" just count number of
pages returned to the pool consumer and not the number of pages
allocated to the pool (as you said, since we do not use bulking
for high_order allocation there is no difference at the moment).
What I would like to track is the number of allocated pages
(of any order) so I guess we can just increment "pages" counter in
__page_pool_alloc_page_order() as well. Agree?

> 
> Which makes it very similar to pages_state_hold_cnt, just 64bit...

do you prefer to use pages_state_hold_cnt instead of adding a new
pages counter?

Regards,
Lorenzo


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next] net: page_pool: add pages and released_pages counters

[Jakub Kicinski]

On Sat, 15 Apr 2023 13:16:40 +0200 Lorenzo Bianconi wrote:
> > What about high order? If we use bulk API for high order one day, 
> > will @slow_high_order not count calls like @slow does? So we should
> > bump the new counter for high order, too.  
> 
> yes, right. AFAIU "slow_high_order" and "slow" just count number of
> pages returned to the pool consumer and not the number of pages
> allocated to the pool (as you said, since we do not use bulking
> for high_order allocation there is no difference at the moment).
> What I would like to track is the number of allocated pages
> (of any order) so I guess we can just increment "pages" counter in
> __page_pool_alloc_page_order() as well. Agree?

Yup, that sounds better.

> > Which makes it very similar to pages_state_hold_cnt, just 64bit...  
> 
> do you prefer to use pages_state_hold_cnt instead of adding a new
> pages counter?

No strong preference either way. It's a tradeoff between saving 4B 
and making the code a little more complex. Perhaps we should stick 
to simplicity and add the counter like you did. Nothing stops us from
optimizing later.

Re: [PATCH net-next] net: page_pool: add pages and released_pages counters

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

> On Sat, 15 Apr 2023 13:16:40 +0200 Lorenzo Bianconi wrote:
> > > What about high order? If we use bulk API for high order one day, 
> > > will @slow_high_order not count calls like @slow does? So we should
> > > bump the new counter for high order, too.  
> > 
> > yes, right. AFAIU "slow_high_order" and "slow" just count number of
> > pages returned to the pool consumer and not the number of pages
> > allocated to the pool (as you said, since we do not use bulking
> > for high_order allocation there is no difference at the moment).
> > What I would like to track is the number of allocated pages
> > (of any order) so I guess we can just increment "pages" counter in
> > __page_pool_alloc_page_order() as well. Agree?
> 
> Yup, that sounds better.

ack, fine. I am now wondering if these counters are useful just during
debugging or even in the normal use-case.
@Jesper, Ilias, Joe: what do you think?

Regards,
Lorenzo

> 
> > > Which makes it very similar to pages_state_hold_cnt, just 64bit...  
> > 
> > do you prefer to use pages_state_hold_cnt instead of adding a new
> > pages counter?
> 
> No strong preference either way. It's a tradeoff between saving 4B 
> and making the code a little more complex. Perhaps we should stick 
> to simplicity and add the counter like you did. Nothing stops us from
> optimizing later.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next] net: page_pool: add pages and released_pages counters

[Jakub Kicinski]

On Mon, 17 Apr 2023 23:39:40 +0200 Lorenzo Bianconi wrote:
> > Yup, that sounds better.  
> 
> ack, fine. I am now wondering if these counters are useful just during
> debugging or even in the normal use-case.

I was wondering about it too, FWIW, I think the most useful info is
the number of outstanding references. But it may be a little too
unflexible as a statistic. BPF attached to the trace point may be
a better choice there, and tracepoints are already in place.

Re: [PATCH net-next] net: page_pool: add pages and released_pages counters

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

> On Mon, 17 Apr 2023 23:39:40 +0200 Lorenzo Bianconi wrote:
> > > Yup, that sounds better.  
> > 
> > ack, fine. I am now wondering if these counters are useful just during
> > debugging or even in the normal use-case.
> 
> I was wondering about it too, FWIW, I think the most useful info is
> the number of outstanding references. But it may be a little too
> unflexible as a statistic. BPF attached to the trace point may be
> a better choice there, and tracepoints are already in place.

ack, I agree. Let's drop this patch in this case.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]