Re: [PATCH net-next] page_pool: Add recycle stats to page_pool_put_page_bulk

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> On Tue, Apr 05, 2022 at 10:52:55PM +0200, Lorenzo Bianconi wrote:
> > Add missing recycle stats to page_pool_put_page_bulk routine.
> 
> Thanks for proposing this change. I did miss this path when adding
> stats.
> 
> I'm sort of torn on this. It almost seems that we might want to track
> bulking events separately as their own stat.
> 
> Maybe Ilias has an opinion on this; I did implement the stats, but I'm not
> a maintainer of the page_pool so I'm not sure what I think matters all
> that much ;) 
> 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  net/core/page_pool.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index 1943c0f0307d..4af55d28ffa3 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -36,6 +36,12 @@
> >  		this_cpu_inc(s->__stat);						\
> >  	} while (0)
> >  
> > +#define recycle_stat_add(pool, __stat, val)						\
> > +	do {										\
> > +		struct page_pool_recycle_stats __percpu *s = pool->recycle_stats;	\
> > +		this_cpu_add(s->__stat, val);						\
> > +	} while (0)
> > +
> >  bool page_pool_get_stats(struct page_pool *pool,
> >  			 struct page_pool_stats *stats)
> >  {
> > @@ -63,6 +69,7 @@ EXPORT_SYMBOL(page_pool_get_stats);
> >  #else
> >  #define alloc_stat_inc(pool, __stat)
> >  #define recycle_stat_inc(pool, __stat)
> > +#define recycle_stat_add(pool, __stat, val)
> >  #endif
> >  
> >  static int page_pool_init(struct page_pool *pool,
> > @@ -566,9 +573,13 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
> >  	/* Bulk producer into ptr_ring page_pool cache */
> >  	page_pool_ring_lock(pool);
> >  	for (i = 0; i < bulk_len; i++) {
> > -		if (__ptr_ring_produce(&pool->ring, data[i]))
> > -			break; /* ring full */
> > +		if (__ptr_ring_produce(&pool->ring, data[i])) {
> > +			/* ring full */
> > +			recycle_stat_inc(pool, ring_full);
> > +			break;
> > +		}
> >  	}
> > +	recycle_stat_add(pool, ring, i);
> 
> If we do go with this approach (instead of adding bulking-specific stats),
> we might want to replicate this change in __page_pool_alloc_pages_slow; we
> currently only count the single allocation returned by the slow path, but
> the rest of the pages which refilled the cache are not counted.

Hi Joe,

do you mean to add an event like "bulk_ring_refill" and just count one for
this? I guess the "bulk_ring_refill" event is just a ring refill on "n" pages
so I think it is more meaningful to increment ring refill counter of "n".
What do you think?

Regards,
Lorenzo

> 
> >  	page_pool_ring_unlock(pool);
> >  
> >  	/* Hopefully all pages was return into ptr_ring */
> > -- 
> > 2.35.1
> > 

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