| Message ID | 1643237300-44904-2-git-send-email-jdamato@fastly.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: page_pool: Add page_pool stat counters | expand |
On Wed, 26 Jan 2022 14:48:15 -0800 Joe Damato wrote: > Add a stats structure with a an internal alloc structure for holding > allocation path related stats. > > The alloc structure contains the stat 'fast'. This stat tracks fast > path allocations. > > A static inline accessor function is exposed for accessing this stat. > +/** > + * stats for tracking page_pool events. > + * > + * accessor functions for these stats provided below. > + * > + * Note that it is the responsibility of the API consumer to ensure that > + * the page_pool has not been destroyed while accessing stats fields. > + */ > +struct page_pool_stats { > + struct { > + u64 fast; /* fast path allocations */ > + } alloc; > +}; scripts/kernel-doc says: include/net/page_pool.h:75: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * stats for tracking page_pool events.
On Thu, Jan 27, 2022 at 8:32 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 26 Jan 2022 14:48:15 -0800 Joe Damato wrote: > > Add a stats structure with a an internal alloc structure for holding > > allocation path related stats. > > > > The alloc structure contains the stat 'fast'. This stat tracks fast > > path allocations. > > > > A static inline accessor function is exposed for accessing this stat. > > > +/** > > + * stats for tracking page_pool events. > > + * > > + * accessor functions for these stats provided below. > > + * > > + * Note that it is the responsibility of the API consumer to ensure that > > + * the page_pool has not been destroyed while accessing stats fields. > > + */ > > +struct page_pool_stats { > > + struct { > > + u64 fast; /* fast path allocations */ > > + } alloc; > > +}; > > scripts/kernel-doc says: > > include/net/page_pool.h:75: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst > * stats for tracking page_pool events. Thank you. I had only been running scripts/checkpatch, but will remember to also run kernel-doc in the future. I will correct the comments in the v2.
On 26/01/2022 23.48, Joe Damato wrote: > @@ -86,6 +100,7 @@ struct page_pool_params { > > struct page_pool { > struct page_pool_params p; > + struct page_pool_stats ps; > > struct delayed_work release_dw; The placement of page_pool_stats is problematic, due to cache-line placement. It will be sharing a cache-line with page_pool_params, which is read-mostly. As I say on benchmark email, this is likely the performance regression you saw. I *do* know you have changed location (to percpu) in newer patch versions, but I think it is important to point out, that we have to be very careful about cache-line placements, and which part of 'struct page_pool' is accessed by which part of the code RX or DMA-TX-completion "return" code. --Jesper
diff --git a/include/net/page_pool.h b/include/net/page_pool.h index 79a8055..3ae3dc4 100644 --- a/include/net/page_pool.h +++ b/include/net/page_pool.h @@ -71,6 +71,20 @@ struct pp_alloc_cache { struct page *cache[PP_ALLOC_CACHE_SIZE]; }; +/** + * stats for tracking page_pool events. + * + * accessor functions for these stats provided below. + * + * Note that it is the responsibility of the API consumer to ensure that + * the page_pool has not been destroyed while accessing stats fields. + */ +struct page_pool_stats { + struct { + u64 fast; /* fast path allocations */ + } alloc; +}; + struct page_pool_params { unsigned int flags; unsigned int order; @@ -86,6 +100,7 @@ struct page_pool_params { struct page_pool { struct page_pool_params p; + struct page_pool_stats ps; struct delayed_work release_dw; void (*disconnect)(void *); @@ -180,6 +195,12 @@ void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *), void page_pool_release_page(struct page_pool *pool, struct page *page); void page_pool_put_page_bulk(struct page_pool *pool, void **data, int count); + +static inline u64 page_pool_stats_get_fast(struct page_pool *pool) +{ + return pool->ps.alloc.fast; +} + #else static inline void page_pool_destroy(struct page_pool *pool) { @@ -199,6 +220,11 @@ static inline void page_pool_put_page_bulk(struct page_pool *pool, void **data, int count) { } + +static inline u64 page_pool_stats_get_fast(struct page_pool *pool) +{ + return 0; +} #endif void page_pool_put_page(struct page_pool *pool, struct page *page, diff --git a/net/core/page_pool.c b/net/core/page_pool.c index bd62c01..84c9566 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -166,6 +166,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]; + pool->ps.alloc.fast++; } else { page = page_pool_refill_alloc_cache(pool); }
Add a stats structure with a an internal alloc structure for holding allocation path related stats. The alloc structure contains the stat 'fast'. This stat tracks fast path allocations. A static inline accessor function is exposed for accessing this stat. Signed-off-by: Joe Damato <jdamato@fastly.com> --- include/net/page_pool.h | 26 ++++++++++++++++++++++++++ net/core/page_pool.c | 1 + 2 files changed, 27 insertions(+)