[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Yk6WD+rEIdWL8tpW@lore-desk>
Date: Thu, 7 Apr 2022 09:43:11 +0200
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Joe Damato <jdamato@...tly.com>
Cc: netdev@...r.kernel.org, lorenzo.bianconi@...hat.com,
davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
jbrouer@...hat.com, ilias.apalodimas@...aro.org
Subject: Re: [PATCH net-next] page_pool: Add recycle stats to
page_pool_put_page_bulk
> 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@...nel.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
> >
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists