* [net-next v7 1/4] page_pool: Add allocation stats
2022-02-25 17:41 [net-next v7 0/4] page_pool: Add stats counters Joe Damato
@ 2022-02-25 17:41 ` Joe Damato
2022-02-28 7:07 ` Jesper Dangaard Brouer
2022-02-25 17:41 ` [net-next v7 2/4] page_pool: Add recycle stats Joe Damato
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Joe Damato @ 2022-02-25 17:41 UTC (permalink / raw)
To: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer
Cc: Joe Damato
Add per-pool statistics counters for the allocation path of a page pool.
These stats are incremented in softirq context, so no locking or per-cpu
variables are needed.
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 <jdamato@fastly.com>
---
include/net/page_pool.h | 18 ++++++++++++++++++
net/Kconfig | 13 +++++++++++++
net/core/page_pool.c | 24 ++++++++++++++++++++----
3 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 97c3c19..1f27e8a4 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -84,6 +84,19 @@ struct page_pool_params {
void *init_arg;
};
+#ifdef CONFIG_PAGE_POOL_STATS
+struct page_pool_alloc_stats {
+ 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 */
+};
+#endif
+
struct page_pool {
struct page_pool_params p;
@@ -96,6 +109,11 @@ struct page_pool {
unsigned int frag_offset;
struct page *frag_page;
long frag_users;
+
+#ifdef CONFIG_PAGE_POOL_STATS
+ /* these stats are incremented while in softirq context */
+ struct page_pool_alloc_stats alloc_stats;
+#endif
u32 xdp_mem_id;
/*
diff --git a/net/Kconfig b/net/Kconfig
index 8a1f9d0..6b78f69 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 page allocation and recycling
+ in page pools. This option incurs additional CPU cost in allocation
+ and recycle 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..0fa4b76 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -26,6 +26,13 @@
#define BIAS_MAX LONG_MAX
+#ifdef CONFIG_PAGE_POOL_STATS
+/* alloc_stat_inc is intended to be used in softirq context */
+#define alloc_stat_inc(pool, __stat) (pool->alloc_stats.__stat++)
+#else
+#define alloc_stat_inc(pool, __stat)
+#endif
+
static int page_pool_init(struct page_pool *pool,
const struct page_pool_params *params)
{
@@ -117,8 +124,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)) {
+ alloc_stat_inc(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 +154,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);
+ alloc_stat_inc(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];
+ alloc_stat_inc(pool, refill);
+ }
return page;
}
@@ -166,6 +178,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];
+ alloc_stat_inc(pool, fast);
} else {
page = page_pool_refill_alloc_cache(pool);
}
@@ -239,6 +252,7 @@ static struct page *__page_pool_alloc_page_order(struct page_pool *pool,
return NULL;
}
+ alloc_stat_inc(pool, slow_high_order);
page_pool_set_pp_info(pool, page);
/* Track how many pages are held 'in-flight' */
@@ -293,10 +307,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
+ alloc_stat_inc(pool, slow);
+ } else {
page = NULL;
+ }
/* When page just alloc'ed is should/must have refcnt 1. */
return page;
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [net-next v7 1/4] page_pool: Add allocation stats
2022-02-25 17:41 ` [net-next v7 1/4] page_pool: Add allocation stats Joe Damato
@ 2022-02-28 7:07 ` Jesper Dangaard Brouer
2022-02-28 7:28 ` Ilias Apalodimas
0 siblings, 1 reply; 17+ messages in thread
From: Jesper Dangaard Brouer @ 2022-02-28 7:07 UTC (permalink / raw)
To: Joe Damato, netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux
Cc: brouer
On 25/02/2022 18.41, Joe Damato wrote:
> Add per-pool statistics counters for the allocation path of a page pool.
> These stats are incremented in softirq context, so no locking or per-cpu
> variables are needed.
>
> 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<jdamato@fastly.com>
> ---
> include/net/page_pool.h | 18 ++++++++++++++++++
> net/Kconfig | 13 +++++++++++++
> net/core/page_pool.c | 24 ++++++++++++++++++++----
> 3 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index 97c3c19..1f27e8a4 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -84,6 +84,19 @@ struct page_pool_params {
> void *init_arg;
> };
>
> +#ifdef CONFIG_PAGE_POOL_STATS
> +struct page_pool_alloc_stats {
> + 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 */
> +};
> +#endif
> +
> struct page_pool {
> struct page_pool_params p;
>
> @@ -96,6 +109,11 @@ struct page_pool {
> unsigned int frag_offset;
> struct page *frag_page;
> long frag_users;
> +
> +#ifdef CONFIG_PAGE_POOL_STATS
> + /* these stats are incremented while in softirq context */
> + struct page_pool_alloc_stats alloc_stats;
> +#endif
> u32 xdp_mem_id;
I like the cache-line placement.
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next v7 1/4] page_pool: Add allocation stats
2022-02-28 7:07 ` Jesper Dangaard Brouer
@ 2022-02-28 7:28 ` Ilias Apalodimas
0 siblings, 0 replies; 17+ messages in thread
From: Ilias Apalodimas @ 2022-02-28 7:28 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Joe Damato, netdev, kuba, davem, hawk, saeed, ttoukan.linux, brouer
On Mon, 28 Feb 2022 at 09:07, Jesper Dangaard Brouer <jbrouer@redhat.com> wrote:
>
>
> On 25/02/2022 18.41, Joe Damato wrote:
> > Add per-pool statistics counters for the allocation path of a page pool.
> > These stats are incremented in softirq context, so no locking or per-cpu
> > variables are needed.
> >
> > 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<jdamato@fastly.com>
> > ---
> > include/net/page_pool.h | 18 ++++++++++++++++++
> > net/Kconfig | 13 +++++++++++++
> > net/core/page_pool.c | 24 ++++++++++++++++++++----
> > 3 files changed, 51 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> > index 97c3c19..1f27e8a4 100644
> > --- a/include/net/page_pool.h
> > +++ b/include/net/page_pool.h
> > @@ -84,6 +84,19 @@ struct page_pool_params {
> > void *init_arg;
> > };
> >
> > +#ifdef CONFIG_PAGE_POOL_STATS
> > +struct page_pool_alloc_stats {
> > + 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 */
> > +};
> > +#endif
> > +
> > struct page_pool {
> > struct page_pool_params p;
> >
> > @@ -96,6 +109,11 @@ struct page_pool {
> > unsigned int frag_offset;
> > struct page *frag_page;
> > long frag_users;
> > +
> > +#ifdef CONFIG_PAGE_POOL_STATS
> > + /* these stats are incremented while in softirq context */
> > + struct page_pool_alloc_stats alloc_stats;
> > +#endif
> > u32 xdp_mem_id;
>
> I like the cache-line placement.
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [net-next v7 2/4] page_pool: Add recycle stats
2022-02-25 17:41 [net-next v7 0/4] page_pool: Add stats counters Joe Damato
2022-02-25 17:41 ` [net-next v7 1/4] page_pool: Add allocation stats Joe Damato
@ 2022-02-25 17:41 ` Joe Damato
2022-02-28 7:20 ` Jesper Dangaard Brouer
2022-02-25 17:41 ` [net-next v7 3/4] page_pool: Add function to batch and return stats Joe Damato
2022-02-25 17:41 ` [net-next v7 4/4] mlx5: add support for page_pool_get_stats Joe Damato
3 siblings, 1 reply; 17+ messages in thread
From: Joe Damato @ 2022-02-25 17:41 UTC (permalink / raw)
To: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer
Cc: Joe Damato
Add per-cpu stats tracking page pool recycling events:
- cached: recycling placed page in the page pool cache
- cache_full: page pool cache was full
- 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
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
include/net/page_pool.h | 16 ++++++++++++++++
net/core/page_pool.c | 28 +++++++++++++++++++++++++++-
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 1f27e8a4..298af95 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -95,6 +95,18 @@ struct page_pool_alloc_stats {
u64 refill; /* allocations via successful refill */
u64 waive; /* failed refills due to numa zone mismatch */
};
+
+struct page_pool_recycle_stats {
+ u64 cached; /* recycling placed page in the cache. */
+ u64 cache_full; /* cache was full */
+ u64 ring; /* recycling placed page back into ptr ring */
+ u64 ring_full; /* page was released from page-pool because
+ * PTR ring was full.
+ */
+ u64 released_refcnt; /* page released because of elevated
+ * refcnt
+ */
+};
#endif
struct page_pool {
@@ -144,6 +156,10 @@ struct page_pool {
*/
struct ptr_ring ring;
+#ifdef CONFIG_PAGE_POOL_STATS
+ /* recycle stats are per-cpu to avoid locking */
+ struct page_pool_recycle_stats __percpu *recycle_stats;
+#endif
atomic_t pages_state_release_cnt;
/* A page_pool is strictly tied to a single RX-queue being
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 0fa4b76..27233bf 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -29,8 +29,15 @@
#ifdef CONFIG_PAGE_POOL_STATS
/* alloc_stat_inc is intended to be used in softirq context */
#define alloc_stat_inc(pool, __stat) (pool->alloc_stats.__stat++)
+/* recycle_stat_inc is safe to use when preemption is possible. */
+#define recycle_stat_inc(pool, __stat) \
+ do { \
+ struct page_pool_recycle_stats __percpu *s = pool->recycle_stats; \
+ this_cpu_inc(s->__stat); \
+ } while (0)
#else
#define alloc_stat_inc(pool, __stat)
+#define recycle_stat_inc(pool, __stat)
#endif
static int page_pool_init(struct page_pool *pool,
@@ -80,6 +87,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->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
+ if (!pool->recycle_stats)
+ return -ENOMEM;
+#endif
+
if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0)
return -ENOMEM;
@@ -410,6 +423,11 @@ static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)
else
ret = ptr_ring_produce_bh(&pool->ring, page);
+#ifdef CONFIG_PAGE_POOL_STATS
+ if (ret == 0)
+ recycle_stat_inc(pool, ring);
+#endif
+
return (ret == 0) ? true : false;
}
@@ -421,11 +439,14 @@ static bool page_pool_recycle_in_ring(struct page_pool *pool, struct page *page)
static bool page_pool_recycle_in_cache(struct page *page,
struct page_pool *pool)
{
- if (unlikely(pool->alloc.count == PP_ALLOC_CACHE_SIZE))
+ if (unlikely(pool->alloc.count == PP_ALLOC_CACHE_SIZE)) {
+ recycle_stat_inc(pool, cache_full);
return false;
+ }
/* Caller MUST have verified/know (page_ref_count(page) == 1) */
pool->alloc.cache[pool->alloc.count++] = page;
+ recycle_stat_inc(pool, cached);
return true;
}
@@ -475,6 +496,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
* doing refcnt based recycle tricks, meaning another process
* will be invoking put_page.
*/
+ recycle_stat_inc(pool, released_refcnt);
/* Do not replace this with page_pool_return_page() */
page_pool_release_page(pool, page);
put_page(page);
@@ -488,6 +510,7 @@ void page_pool_put_defragged_page(struct page_pool *pool, struct page *page,
page = __page_pool_put_page(pool, page, dma_sync_size, allow_direct);
if (page && !page_pool_recycle_in_ring(pool, page)) {
/* Cache full, fallback to free pages */
+ recycle_stat_inc(pool, ring_full);
page_pool_return_page(pool, page);
}
}
@@ -636,6 +659,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->recycle_stats);
+#endif
kfree(pool);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [net-next v7 2/4] page_pool: Add recycle stats
2022-02-25 17:41 ` [net-next v7 2/4] page_pool: Add recycle stats Joe Damato
@ 2022-02-28 7:20 ` Jesper Dangaard Brouer
2022-02-28 7:28 ` Ilias Apalodimas
0 siblings, 1 reply; 17+ messages in thread
From: Jesper Dangaard Brouer @ 2022-02-28 7:20 UTC (permalink / raw)
To: Joe Damato, netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux
Cc: brouer
On 25/02/2022 18.41, Joe Damato wrote:
> Add per-cpu stats tracking page pool recycling events:
> - cached: recycling placed page in the page pool cache
> - cache_full: page pool cache was full
> - 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
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
> include/net/page_pool.h | 16 ++++++++++++++++
> net/core/page_pool.c | 28 +++++++++++++++++++++++++++-
> 2 files changed, 43 insertions(+), 1 deletion(-)
LGTM - thanks for working on this
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next v7 2/4] page_pool: Add recycle stats
2022-02-28 7:20 ` Jesper Dangaard Brouer
@ 2022-02-28 7:28 ` Ilias Apalodimas
0 siblings, 0 replies; 17+ messages in thread
From: Ilias Apalodimas @ 2022-02-28 7:28 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Joe Damato, netdev, kuba, davem, hawk, saeed, ttoukan.linux, brouer
On Mon, 28 Feb 2022 at 09:20, Jesper Dangaard Brouer <jbrouer@redhat.com> wrote:
>
>
> On 25/02/2022 18.41, Joe Damato wrote:
> > Add per-cpu stats tracking page pool recycling events:
> > - cached: recycling placed page in the page pool cache
> > - cache_full: page pool cache was full
> > - 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
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> > include/net/page_pool.h | 16 ++++++++++++++++
> > net/core/page_pool.c | 28 +++++++++++++++++++++++++++-
> > 2 files changed, 43 insertions(+), 1 deletion(-)
>
> LGTM - thanks for working on this
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [net-next v7 3/4] page_pool: Add function to batch and return stats
2022-02-25 17:41 [net-next v7 0/4] page_pool: Add stats counters Joe Damato
2022-02-25 17:41 ` [net-next v7 1/4] page_pool: Add allocation stats Joe Damato
2022-02-25 17:41 ` [net-next v7 2/4] page_pool: Add recycle stats Joe Damato
@ 2022-02-25 17:41 ` Joe Damato
2022-02-28 7:22 ` Jesper Dangaard Brouer
2022-02-25 17:41 ` [net-next v7 4/4] mlx5: add support for page_pool_get_stats Joe Damato
3 siblings, 1 reply; 17+ messages in thread
From: Joe Damato @ 2022-02-25 17:41 UTC (permalink / raw)
To: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer
Cc: Joe Damato
Adds a function page_pool_get_stats which can be used by drivers to obtain
stats for a specified page_pool.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
include/net/page_pool.h | 17 +++++++++++++++++
net/core/page_pool.c | 25 +++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 298af95..ea5fb70 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -107,6 +107,23 @@ struct page_pool_recycle_stats {
* refcnt
*/
};
+
+/* This struct wraps the above stats structs so users of the
+ * page_pool_get_stats API can pass a single argument when requesting the
+ * stats for the page pool.
+ */
+struct page_pool_stats {
+ struct page_pool_alloc_stats alloc_stats;
+ struct page_pool_recycle_stats recycle_stats;
+};
+
+/*
+ * 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 call page_pool_get_stats to get stats for the specified pool.
+ */
+bool page_pool_get_stats(struct page_pool *pool,
+ struct page_pool_stats *stats);
#endif
struct page_pool {
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 27233bf..f4f8f5f 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -35,6 +35,31 @@
struct page_pool_recycle_stats __percpu *s = pool->recycle_stats; \
this_cpu_inc(s->__stat); \
} while (0)
+
+bool page_pool_get_stats(struct page_pool *pool,
+ struct page_pool_stats *stats)
+{
+ int cpu = 0;
+
+ if (!stats)
+ return false;
+
+ memcpy(&stats->alloc_stats, &pool->alloc_stats, sizeof(pool->alloc_stats));
+
+ for_each_possible_cpu(cpu) {
+ const struct page_pool_recycle_stats *pcpu =
+ per_cpu_ptr(pool->recycle_stats, cpu);
+
+ stats->recycle_stats.cached += pcpu->cached;
+ stats->recycle_stats.cache_full += pcpu->cache_full;
+ stats->recycle_stats.ring += pcpu->ring;
+ stats->recycle_stats.ring_full += pcpu->ring_full;
+ stats->recycle_stats.released_refcnt += pcpu->released_refcnt;
+ }
+
+ return true;
+}
+EXPORT_SYMBOL(page_pool_get_stats);
#else
#define alloc_stat_inc(pool, __stat)
#define recycle_stat_inc(pool, __stat)
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [net-next v7 3/4] page_pool: Add function to batch and return stats
2022-02-25 17:41 ` [net-next v7 3/4] page_pool: Add function to batch and return stats Joe Damato
@ 2022-02-28 7:22 ` Jesper Dangaard Brouer
2022-02-28 7:29 ` Ilias Apalodimas
0 siblings, 1 reply; 17+ messages in thread
From: Jesper Dangaard Brouer @ 2022-02-28 7:22 UTC (permalink / raw)
To: Joe Damato, netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux
Cc: brouer
On 25/02/2022 18.41, Joe Damato wrote:
> Adds a function page_pool_get_stats which can be used by drivers to obtain
> stats for a specified page_pool.
>
> Signed-off-by: Joe Damato<jdamato@fastly.com>
> ---
> include/net/page_pool.h | 17 +++++++++++++++++
> net/core/page_pool.c | 25 +++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next v7 3/4] page_pool: Add function to batch and return stats
2022-02-28 7:22 ` Jesper Dangaard Brouer
@ 2022-02-28 7:29 ` Ilias Apalodimas
0 siblings, 0 replies; 17+ messages in thread
From: Ilias Apalodimas @ 2022-02-28 7:29 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Joe Damato, netdev, kuba, davem, hawk, saeed, ttoukan.linux, brouer
On Mon, 28 Feb 2022 at 09:22, Jesper Dangaard Brouer <jbrouer@redhat.com> wrote:
>
>
> On 25/02/2022 18.41, Joe Damato wrote:
> > Adds a function page_pool_get_stats which can be used by drivers to obtain
> > stats for a specified page_pool.
> >
> > Signed-off-by: Joe Damato<jdamato@fastly.com>
> > ---
> > include/net/page_pool.h | 17 +++++++++++++++++
> > net/core/page_pool.c | 25 +++++++++++++++++++++++++
> > 2 files changed, 42 insertions(+)
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [net-next v7 4/4] mlx5: add support for page_pool_get_stats
2022-02-25 17:41 [net-next v7 0/4] page_pool: Add stats counters Joe Damato
` (2 preceding siblings ...)
2022-02-25 17:41 ` [net-next v7 3/4] page_pool: Add function to batch and return stats Joe Damato
@ 2022-02-25 17:41 ` Joe Damato
2022-02-28 7:28 ` Jesper Dangaard Brouer
3 siblings, 1 reply; 17+ messages in thread
From: Joe Damato @ 2022-02-25 17:41 UTC (permalink / raw)
To: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer
Cc: Joe Damato
This change adds support for the page_pool_get_stats API to mlx5. If the
user has enabled CONFIG_PAGE_POOL_STATS in their kernel, ethtool will
output page pool stats.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 76 ++++++++++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 27 +++++++-
2 files changed, 102 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index 3e5d8c7..56eedf5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -37,6 +37,10 @@
#include "en/ptp.h"
#include "en/port.h"
+#ifdef CONFIG_PAGE_POOL_STATS
+#include <net/page_pool.h>
+#endif
+
static unsigned int stats_grps_num(struct mlx5e_priv *priv)
{
return !priv->profile->stats_grps_num ? 0 :
@@ -183,6 +187,19 @@ static const struct counter_desc sw_stats_desc[] = {
{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_congst_umr) },
{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_arfs_err) },
{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_recover) },
+#ifdef CONFIG_PAGE_POOL_STATS
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_fast) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_slow) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_slow_high_order) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_empty) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_refill) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_waive) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_rec_cached) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_rec_cache_full) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_rec_ring) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_rec_ring_full) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_page_pool_rec_released_ref) },
+#endif
#ifdef CONFIG_MLX5_EN_TLS
{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_tls_decrypted_packets) },
{ MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_tls_decrypted_bytes) },
@@ -349,6 +366,20 @@ static void mlx5e_stats_grp_sw_update_stats_rq_stats(struct mlx5e_sw_stats *s,
s->rx_congst_umr += rq_stats->congst_umr;
s->rx_arfs_err += rq_stats->arfs_err;
s->rx_recover += rq_stats->recover;
+#ifdef CONFIG_PAGE_POOL_STATS
+ s->rx_page_pool_fast += rq_stats->page_pool_fast;
+ s->rx_page_pool_slow += rq_stats->page_pool_slow;
+ s->rx_page_pool_empty += rq_stats->page_pool_empty;
+ s->rx_page_pool_refill += rq_stats->page_pool_refill;
+ s->rx_page_pool_waive += rq_stats->page_pool_waive;
+
+ s->rx_page_pool_slow_high_order += rq_stats->page_pool_slow_high_order;
+ s->rx_page_pool_rec_cached += rq_stats->page_pool_rec_cached;
+ s->rx_page_pool_rec_cache_full += rq_stats->page_pool_rec_cache_full;
+ s->rx_page_pool_rec_ring += rq_stats->page_pool_rec_ring;
+ s->rx_page_pool_rec_ring_full += rq_stats->page_pool_rec_ring_full;
+ s->rx_page_pool_rec_released_ref += rq_stats->page_pool_rec_released_ref;
+#endif
#ifdef CONFIG_MLX5_EN_TLS
s->rx_tls_decrypted_packets += rq_stats->tls_decrypted_packets;
s->rx_tls_decrypted_bytes += rq_stats->tls_decrypted_bytes;
@@ -455,6 +486,35 @@ static void mlx5e_stats_grp_sw_update_stats_qos(struct mlx5e_priv *priv,
}
}
+#ifdef CONFIG_PAGE_POOL_STATS
+static void mlx5e_stats_update_stats_rq_page_pool(struct mlx5e_channel *c)
+{
+ struct mlx5e_rq_stats *rq_stats = c->rq.stats;
+ struct page_pool *pool = c->rq.page_pool;
+ struct page_pool_stats stats = { 0 };
+
+ if (!page_pool_get_stats(pool, &stats))
+ return;
+
+ rq_stats->page_pool_fast = stats.alloc_stats.fast;
+ rq_stats->page_pool_slow = stats.alloc_stats.slow;
+ rq_stats->page_pool_slow_high_order = stats.alloc_stats.slow_high_order;
+ rq_stats->page_pool_empty = stats.alloc_stats.empty;
+ rq_stats->page_pool_waive = stats.alloc_stats.waive;
+ rq_stats->page_pool_refill = stats.alloc_stats.refill;
+
+ rq_stats->page_pool_rec_cached = stats.recycle_stats.cached;
+ rq_stats->page_pool_rec_cache_full = stats.recycle_stats.cache_full;
+ rq_stats->page_pool_rec_ring = stats.recycle_stats.ring;
+ rq_stats->page_pool_rec_ring_full = stats.recycle_stats.ring_full;
+ rq_stats->page_pool_rec_released_ref = stats.recycle_stats.released_refcnt;
+}
+#else
+static void mlx5e_stats_update_stats_rq_page_pool(struct mlx5e_channel *c)
+{
+}
+#endif
+
static MLX5E_DECLARE_STATS_GRP_OP_UPDATE_STATS(sw)
{
struct mlx5e_sw_stats *s = &priv->stats.sw;
@@ -465,8 +525,11 @@ static MLX5E_DECLARE_STATS_GRP_OP_UPDATE_STATS(sw)
for (i = 0; i < priv->stats_nch; i++) {
struct mlx5e_channel_stats *channel_stats =
priv->channel_stats[i];
+
int j;
+ mlx5e_stats_update_stats_rq_page_pool(priv->channels.c[i]);
+
mlx5e_stats_grp_sw_update_stats_rq_stats(s, &channel_stats->rq);
mlx5e_stats_grp_sw_update_stats_xdpsq(s, &channel_stats->rq_xdpsq);
mlx5e_stats_grp_sw_update_stats_ch_stats(s, &channel_stats->ch);
@@ -1887,6 +1950,19 @@ static const struct counter_desc rq_stats_desc[] = {
{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, congst_umr) },
{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, arfs_err) },
{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, recover) },
+#ifdef CONFIG_PAGE_POOL_STATS
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
+#endif
#ifdef CONFIG_MLX5_EN_TLS
{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, tls_decrypted_packets) },
{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, tls_decrypted_bytes) },
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
index 14eaf92..9f66425 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
@@ -205,7 +205,19 @@ struct mlx5e_sw_stats {
u64 ch_aff_change;
u64 ch_force_irq;
u64 ch_eq_rearm;
-
+#ifdef CONFIG_PAGE_POOL_STATS
+ u64 rx_page_pool_fast;
+ u64 rx_page_pool_slow;
+ u64 rx_page_pool_slow_high_order;
+ u64 rx_page_pool_empty;
+ u64 rx_page_pool_refill;
+ u64 rx_page_pool_waive;
+ u64 rx_page_pool_rec_cached;
+ u64 rx_page_pool_rec_cache_full;
+ u64 rx_page_pool_rec_ring;
+ u64 rx_page_pool_rec_ring_full;
+ u64 rx_page_pool_rec_released_ref;
+#endif
#ifdef CONFIG_MLX5_EN_TLS
u64 tx_tls_encrypted_packets;
u64 tx_tls_encrypted_bytes;
@@ -352,6 +364,19 @@ struct mlx5e_rq_stats {
u64 congst_umr;
u64 arfs_err;
u64 recover;
+#ifdef CONFIG_PAGE_POOL_STATS
+ u64 page_pool_fast;
+ u64 page_pool_slow;
+ u64 page_pool_slow_high_order;
+ u64 page_pool_empty;
+ u64 page_pool_refill;
+ u64 page_pool_waive;
+ u64 page_pool_rec_cached;
+ u64 page_pool_rec_cache_full;
+ u64 page_pool_rec_ring;
+ u64 page_pool_rec_ring_full;
+ u64 page_pool_rec_released_ref;
+#endif
#ifdef CONFIG_MLX5_EN_TLS
u64 tls_decrypted_packets;
u64 tls_decrypted_bytes;
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats
2022-02-25 17:41 ` [net-next v7 4/4] mlx5: add support for page_pool_get_stats Joe Damato
@ 2022-02-28 7:28 ` Jesper Dangaard Brouer
2022-02-28 8:38 ` Joe Damato
0 siblings, 1 reply; 17+ messages in thread
From: Jesper Dangaard Brouer @ 2022-02-28 7:28 UTC (permalink / raw)
To: Joe Damato, netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux
Cc: brouer
On 25/02/2022 18.41, Joe Damato wrote:
> +#ifdef CONFIG_PAGE_POOL_STATS
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
> + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
> +#endif
The naming: "page_pool_rec_xxx".
What does the "rec" stand for?
Users of ethtool -S stats... will they know "rec" is "recycle" ?
p.s. we need acks from driver maintainer(s).
--Jesper
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats
2022-02-28 7:28 ` Jesper Dangaard Brouer
@ 2022-02-28 8:38 ` Joe Damato
2022-02-28 9:08 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 17+ messages in thread
From: Joe Damato @ 2022-02-28 8:38 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer
On Sun, Feb 27, 2022 at 11:28 PM Jesper Dangaard Brouer
<jbrouer@redhat.com> wrote:
>
>
>
> On 25/02/2022 18.41, Joe Damato wrote:
> > +#ifdef CONFIG_PAGE_POOL_STATS
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
> > +#endif
>
> The naming: "page_pool_rec_xxx".
> What does the "rec" stand for?
rec stands for recycle.
ethtool strings have a limited size (ETH_GSTRING_LEN - 32 bytes) and
the full word "recycle" didn't fit for some of the stats once the
queue number is prepended elsewhere in the driver code.
> Users of ethtool -S stats... will they know "rec" is "recycle" ?
I am open to other names or adding documentation to the driver docs to
explain the meaning.
> p.s. we need acks from driver maintainer(s).
I've CC'd Tariq and Saaed; I hope they'll take a look when they have a
chance and weigh in on the naming. I am happy to adjust the names as
they wish and submit a v8, or take this code as-is and then iterate on
the names in a separate change.
I think the latter option would be easiest, but I am happy to do
whatever you all prefer.
Thanks,
Joe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats
2022-02-28 8:38 ` Joe Damato
@ 2022-02-28 9:08 ` Toke Høiland-Jørgensen
2022-03-01 0:51 ` Joe Damato
0 siblings, 1 reply; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-02-28 9:08 UTC (permalink / raw)
To: Joe Damato, Jesper Dangaard Brouer
Cc: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer
Joe Damato <jdamato@fastly.com> writes:
> On Sun, Feb 27, 2022 at 11:28 PM Jesper Dangaard Brouer
> <jbrouer@redhat.com> wrote:
>>
>>
>>
>> On 25/02/2022 18.41, Joe Damato wrote:
>> > +#ifdef CONFIG_PAGE_POOL_STATS
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
>> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
>> > +#endif
>>
>> The naming: "page_pool_rec_xxx".
>> What does the "rec" stand for?
>
> rec stands for recycle.
>
> ethtool strings have a limited size (ETH_GSTRING_LEN - 32 bytes) and
> the full word "recycle" didn't fit for some of the stats once the
> queue number is prepended elsewhere in the driver code.
>
>> Users of ethtool -S stats... will they know "rec" is "recycle" ?
>
> I am open to other names or adding documentation to the driver docs to
> explain the meaning.
You could shorten the 'page_pool_' prefix to 'ppool_' or even 'pp_' and
gain some characters that way?
-Toke
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats
2022-02-28 9:08 ` Toke Høiland-Jørgensen
@ 2022-03-01 0:51 ` Joe Damato
2022-03-01 11:23 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 17+ messages in thread
From: Joe Damato @ 2022-03-01 0:51 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Jesper Dangaard Brouer, netdev, kuba, ilias.apalodimas, davem,
hawk, saeed, ttoukan.linux, brouer
On Mon, Feb 28, 2022 at 1:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Joe Damato <jdamato@fastly.com> writes:
>
> > On Sun, Feb 27, 2022 at 11:28 PM Jesper Dangaard Brouer
> > <jbrouer@redhat.com> wrote:
> >>
> >>
> >>
> >> On 25/02/2022 18.41, Joe Damato wrote:
> >> > +#ifdef CONFIG_PAGE_POOL_STATS
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
> >> > +#endif
> >>
> >> The naming: "page_pool_rec_xxx".
> >> What does the "rec" stand for?
> >
> > rec stands for recycle.
> >
> > ethtool strings have a limited size (ETH_GSTRING_LEN - 32 bytes) and
> > the full word "recycle" didn't fit for some of the stats once the
> > queue number is prepended elsewhere in the driver code.
> >
> >> Users of ethtool -S stats... will they know "rec" is "recycle" ?
> >
> > I am open to other names or adding documentation to the driver docs to
> > explain the meaning.
>
> You could shorten the 'page_pool_' prefix to 'ppool_' or even 'pp_' and
> gain some characters that way?
I had considered this, but I thought that 'pp' was possibly as terse as 'rec'.
If you all think these are more clear, I can send a v8 of this series
that changes the strings from the above to this instead:
rx_pp_alloc_fast
rx_pp_alloc_slow
rx_pp_alloc_...
and
rx_pp_recyle_cached
rx_pp_recycle_cache_full
rx_pp_recycle_...
With this name scheme, it looks like all the stat names seem to fit. I
have no idea if this is more or less clear to the user though :)
I'll leave it up to the mlx5 maintainers; I am happy to do whatever
they prefer to get this series in.
Thanks,
Joe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats
2022-03-01 0:51 ` Joe Damato
@ 2022-03-01 11:23 ` Toke Høiland-Jørgensen
2022-03-01 17:00 ` Joe Damato
0 siblings, 1 reply; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-03-01 11:23 UTC (permalink / raw)
To: Joe Damato
Cc: Jesper Dangaard Brouer, netdev, kuba, ilias.apalodimas, davem,
hawk, saeed, ttoukan.linux, brouer
Joe Damato <jdamato@fastly.com> writes:
> On Mon, Feb 28, 2022 at 1:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Joe Damato <jdamato@fastly.com> writes:
>>
>> > On Sun, Feb 27, 2022 at 11:28 PM Jesper Dangaard Brouer
>> > <jbrouer@redhat.com> wrote:
>> >>
>> >>
>> >>
>> >> On 25/02/2022 18.41, Joe Damato wrote:
>> >> > +#ifdef CONFIG_PAGE_POOL_STATS
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
>> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
>> >> > +#endif
>> >>
>> >> The naming: "page_pool_rec_xxx".
>> >> What does the "rec" stand for?
>> >
>> > rec stands for recycle.
>> >
>> > ethtool strings have a limited size (ETH_GSTRING_LEN - 32 bytes) and
>> > the full word "recycle" didn't fit for some of the stats once the
>> > queue number is prepended elsewhere in the driver code.
>> >
>> >> Users of ethtool -S stats... will they know "rec" is "recycle" ?
>> >
>> > I am open to other names or adding documentation to the driver docs to
>> > explain the meaning.
>>
>> You could shorten the 'page_pool_' prefix to 'ppool_' or even 'pp_' and
>> gain some characters that way?
>
> I had considered this, but I thought that 'pp' was possibly as terse as 'rec'.
>
> If you all think these are more clear, I can send a v8 of this series
> that changes the strings from the above to this instead:
>
> rx_pp_alloc_fast
> rx_pp_alloc_slow
> rx_pp_alloc_...
>
> and
>
> rx_pp_recyle_cached
> rx_pp_recycle_cache_full
> rx_pp_recycle_...
>
> With this name scheme, it looks like all the stat names seem to fit. I
> have no idea if this is more or less clear to the user though :)
My thinking was that at least 'pp_' is obviously opaque, so it will be
clear to readers that if they don't know what it is they have to look it
up. Whereas 'rec_' looks like it could be 'record' or something like
that, and it'll make people guess (wrongly).
> I'll leave it up to the mlx5 maintainers; I am happy to do whatever
> they prefer to get this series in.
Right, but this is also going to create precedence for other drivers
that add the page pool-based stats, so spending a bit of time agreeing
on the colour of the bikeshed may be worthwhile here... :)
-Toke
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [net-next v7 4/4] mlx5: add support for page_pool_get_stats
2022-03-01 11:23 ` Toke Høiland-Jørgensen
@ 2022-03-01 17:00 ` Joe Damato
0 siblings, 0 replies; 17+ messages in thread
From: Joe Damato @ 2022-03-01 17:00 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Jesper Dangaard Brouer, netdev, kuba, ilias.apalodimas, davem,
hawk, saeed, ttoukan.linux, brouer
On Tue, Mar 1, 2022 at 3:23 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Joe Damato <jdamato@fastly.com> writes:
>
> > On Mon, Feb 28, 2022 at 1:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Joe Damato <jdamato@fastly.com> writes:
> >>
> >> > On Sun, Feb 27, 2022 at 11:28 PM Jesper Dangaard Brouer
> >> > <jbrouer@redhat.com> wrote:
> >> >>
> >> >>
> >> >>
> >> >> On 25/02/2022 18.41, Joe Damato wrote:
> >> >> > +#ifdef CONFIG_PAGE_POOL_STATS
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_fast) },
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow) },
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_slow_high_order) },
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_empty) },
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_refill) },
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_waive) },
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cached) },
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_cache_full) },
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring) },
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_ring_full) },
> >> >> > + { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, page_pool_rec_released_ref) },
> >> >> > +#endif
> >> >>
> >> >> The naming: "page_pool_rec_xxx".
> >> >> What does the "rec" stand for?
> >> >
> >> > rec stands for recycle.
> >> >
> >> > ethtool strings have a limited size (ETH_GSTRING_LEN - 32 bytes) and
> >> > the full word "recycle" didn't fit for some of the stats once the
> >> > queue number is prepended elsewhere in the driver code.
> >> >
> >> >> Users of ethtool -S stats... will they know "rec" is "recycle" ?
> >> >
> >> > I am open to other names or adding documentation to the driver docs to
> >> > explain the meaning.
> >>
> >> You could shorten the 'page_pool_' prefix to 'ppool_' or even 'pp_' and
> >> gain some characters that way?
> >
> > I had considered this, but I thought that 'pp' was possibly as terse as 'rec'.
> >
> > If you all think these are more clear, I can send a v8 of this series
> > that changes the strings from the above to this instead:
> >
> > rx_pp_alloc_fast
> > rx_pp_alloc_slow
> > rx_pp_alloc_...
> >
> > and
> >
> > rx_pp_recyle_cached
> > rx_pp_recycle_cache_full
> > rx_pp_recycle_...
> >
> > With this name scheme, it looks like all the stat names seem to fit. I
> > have no idea if this is more or less clear to the user though :)
>
> My thinking was that at least 'pp_' is obviously opaque, so it will be
> clear to readers that if they don't know what it is they have to look it
> up. Whereas 'rec_' looks like it could be 'record' or something like
> that, and it'll make people guess (wrongly).
Fair enough.
> > I'll leave it up to the mlx5 maintainers; I am happy to do whatever
> > they prefer to get this series in.
>
> Right, but this is also going to create precedence for other drivers
> that add the page pool-based stats, so spending a bit of time agreeing
> on the colour of the bikeshed may be worthwhile here... :)
Sure, that's fine with me.
I'll give it a few days - hoping to hear back from the mellanox folks
on what they prefer before submitting a v8 with the changes I
mentioned above.
My goal is to avoid a few extra rounds of back and forth if possible :)
Thanks,
Joe
^ permalink raw reply [flat|nested] 17+ messages in thread