* [net-next v8 0/4] page_pool: Add stats counters
@ 2022-03-01 22:10 Joe Damato
2022-03-01 22:10 ` [net-next v8 1/4] page_pool: Add allocation stats Joe Damato
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Joe Damato @ 2022-03-01 22:10 UTC (permalink / raw)
To: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer, leon, linux-rdma, saeedm
Cc: Joe Damato
Greetings:
Welcome to v8.
This revision updates the ethtool name strings of page pool stats in the
mlx5 driver to be more user friendly.
I've included Jesper's ACK tag and Ilias' Reviewed-by tags to the first 3
commits, but otherwise no other changes have been made.
Benchmark output from the v7 cover [1] is pasted below, as it is still
relevant since no functional changes have been made in this revision:
Benchmarks have been re-run. As always, results between runs are highly
variable; you'll find results showing that stats disabled are both faster
and slower than stats enabled in back to back benchmark runs.
Raw benchmark output with stats off [2] and stats on [3] are available for
examination.
Test system:
- 2x Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz
- 2 NUMA zones, with 18 cores per zone and 2 threads per core
bench_page_pool_simple results, loops=200000000
test name stats enabled stats disabled
cycles nanosec cycles nanosec
for_loop 0 0.335 0 0.336
atomic_inc 14 6.106 13 6.022
lock 30 13.365 32 13.968
no-softirq-page_pool01 75 32.884 74 32.308
no-softirq-page_pool02 79 34.696 74 32.302
no-softirq-page_pool03 110 48.005 105 46.073
tasklet_page_pool01_fast_path 14 6.156 14 6.211
tasklet_page_pool02_ptr_ring 41 18.028 39 17.391
tasklet_page_pool03_slow 107 46.646 105 46.123
bench_page_pool_cross_cpu results, loops=20000000 returning_cpus=4:
test name stats enabled stats disabled
cycles nanosec cycles nanosec
page_pool_cross_cpu CPU(0) 3973 1731.596 4015 1750.015
page_pool_cross_cpu CPU(1) 3976 1733.217 4022 1752.864
page_pool_cross_cpu CPU(2) 3973 1731.615 4016 1750.433
page_pool_cross_cpu CPU(3) 3976 1733.218 4021 1752.806
page_pool_cross_cpu CPU(4) 994 433.305 1005 438.217
page_pool_cross_cpu average 3378 - 3415 -
bench_page_pool_cross_cpu results, loops=20000000 returning_cpus=8:
test name stats enabled stats disabled
cycles nanosec cycles nanosec
page_pool_cross_cpu CPU(0) 6969 3037.488 6909 3011.463
page_pool_cross_cpu CPU(1) 6974 3039.469 6913 3012.961
page_pool_cross_cpu CPU(2) 6969 3037.575 6910 3011.585
page_pool_cross_cpu CPU(3) 6974 3039.415 6913 3012.961
page_pool_cross_cpu CPU(4) 6969 3037.288 6909 3011.368
page_pool_cross_cpu CPU(5) 6972 3038.732 6913 3012.920
page_pool_cross_cpu CPU(6) 6969 3037.350 6909 3011.386
page_pool_cross_cpu CPU(7) 6973 3039.356 6913 3012.921
page_pool_cross_cpu CPU(8) 871 379.934 864 376.620
page_pool_cross_cpu average 6293 - 6239 -
Thanks.
[1]: https://lore.kernel.org/all/1645810914-35485-1-git-send-email-jdamato@fastly.com/
[2]: https://gist.githubusercontent.com/jdamato-fsly/d7c34b9fa7be1ce132a266b0f2b92aea/raw/327dcd71d11ece10238fbf19e0472afbcbf22fd4/v7_stats_disabled
[3]: https://gist.githubusercontent.com/jdamato-fsly/d7c34b9fa7be1ce132a266b0f2b92aea/raw/327dcd71d11ece10238fbf19e0472afbcbf22fd4/v7_stats_enabled
v7 -> v8:
- Rename mlx5 ethtool stats so that users have a better idea of
their meaning.
v6 -> v7:
- stats split out into two structs one single per-page pool struct
for allocation path stats and one per-cpu pointer for recycle
path stats.
- page_pool_get_stats updated to use a wrapper struct to gather
stats for allocation and recycle stats with a single argument.
- placement of structs adjusted
- mlx5 driver modified to use page_pool_get_stats API
v5 -> v6:
- Per cpu page_pool_stats struct pointer is now marked as
____cacheline_aligned_in_smp. Placement of the field in the
struct is unchanged; it is the last field.
v4 -> v5:
- Fixed the description of the kernel option in Kconfig.
- Squashed commits 1-10 from v4 into a single commit for easier
review.
- Changed the comment style of the comment for
the this_cpu_inc_alloc_stat macro.
- Changed the return type of page_pool_get_stats from struct
page_pool_stat * to bool.
v3 -> v4:
- Restructured stats to be per-cpu per-pool.
- Global stats and proc file were removed.
- Exposed an API (page_pool_get_stats) for batching the pool stats.
v2 -> v3:
- patch 8/10 ("Add stat tracking cache refill") fixed placement of
counter increment.
- patch 10/10 ("net-procfs: Show page pool stats in proc") updated:
- fix unused label warning from kernel test robot,
- fixed page_pool_seq_show to only display the refill stat
once,
- added a remove_proc_entry for page_pool_stat to
dev_proc_net_exit.
v1 -> v2:
- A new kernel config option has been added, which defaults to N,
preventing this code from being compiled in by default
- The stats structure has been converted to a per-cpu structure
- The stats are now exported via proc (/proc/net/page_pool_stat)
Joe Damato (4):
page_pool: Add allocation stats
page_pool: Add recycle stats
page_pool: Add function to batch and return stats
mlx5: add support for page_pool_get_stats
drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 75 +++++++++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 27 +++++++-
include/net/page_pool.h | 51 ++++++++++++++
net/Kconfig | 13 ++++
net/core/page_pool.c | 77 ++++++++++++++++++++--
5 files changed, 237 insertions(+), 6 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [net-next v8 1/4] page_pool: Add allocation stats
2022-03-01 22:10 [net-next v8 0/4] page_pool: Add stats counters Joe Damato
@ 2022-03-01 22:10 ` Joe Damato
2022-03-01 23:50 ` Saeed Mahameed
2022-03-01 22:10 ` [net-next v8 2/4] page_pool: Add recycle stats Joe Damato
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Joe Damato @ 2022-03-01 22:10 UTC (permalink / raw)
To: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer, leon, linux-rdma, saeedm
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>
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
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] 12+ messages in thread
* [net-next v8 2/4] page_pool: Add recycle stats
2022-03-01 22:10 [net-next v8 0/4] page_pool: Add stats counters Joe Damato
2022-03-01 22:10 ` [net-next v8 1/4] page_pool: Add allocation stats Joe Damato
@ 2022-03-01 22:10 ` Joe Damato
2022-03-01 23:58 ` Saeed Mahameed
2022-03-01 22:10 ` [net-next v8 3/4] page_pool: Add function to batch and return stats Joe Damato
2022-03-01 22:10 ` [net-next v8 4/4] mlx5: add support for page_pool_get_stats Joe Damato
3 siblings, 1 reply; 12+ messages in thread
From: Joe Damato @ 2022-03-01 22:10 UTC (permalink / raw)
To: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer, leon, linux-rdma, saeedm
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>
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
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] 12+ messages in thread
* [net-next v8 3/4] page_pool: Add function to batch and return stats
2022-03-01 22:10 [net-next v8 0/4] page_pool: Add stats counters Joe Damato
2022-03-01 22:10 ` [net-next v8 1/4] page_pool: Add allocation stats Joe Damato
2022-03-01 22:10 ` [net-next v8 2/4] page_pool: Add recycle stats Joe Damato
@ 2022-03-01 22:10 ` Joe Damato
2022-03-01 22:10 ` [net-next v8 4/4] mlx5: add support for page_pool_get_stats Joe Damato
3 siblings, 0 replies; 12+ messages in thread
From: Joe Damato @ 2022-03-01 22:10 UTC (permalink / raw)
To: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer, leon, linux-rdma, saeedm
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>
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
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] 12+ messages in thread
* [net-next v8 4/4] mlx5: add support for page_pool_get_stats
2022-03-01 22:10 [net-next v8 0/4] page_pool: Add stats counters Joe Damato
` (2 preceding siblings ...)
2022-03-01 22:10 ` [net-next v8 3/4] page_pool: Add function to batch and return stats Joe Damato
@ 2022-03-01 22:10 ` Joe Damato
2022-03-02 1:02 ` Saeed Mahameed
3 siblings, 1 reply; 12+ messages in thread
From: Joe Damato @ 2022-03-01 22:10 UTC (permalink / raw)
To: netdev, kuba, ilias.apalodimas, davem, hawk, saeed,
ttoukan.linux, brouer, leon, linux-rdma, saeedm
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 | 75 ++++++++++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 27 +++++++-
2 files changed, 101 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..eb518ec 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_pp_alloc_fast) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_slow) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_slow_high_order) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_empty) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_refill) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_waive) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_cached) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_cache_full) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_ring) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_ring_full) },
+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_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,19 @@ 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_pp_alloc_fast += rq_stats->pp_alloc_fast;
+ s->rx_pp_alloc_slow += rq_stats->pp_alloc_slow;
+ s->rx_pp_alloc_empty += rq_stats->pp_alloc_empty;
+ s->rx_pp_alloc_refill += rq_stats->pp_alloc_refill;
+ s->rx_pp_alloc_waive += rq_stats->pp_alloc_waive;
+ s->rx_pp_alloc_slow_high_order += rq_stats->pp_alloc_slow_high_order;
+ s->rx_pp_recycle_cached += rq_stats->pp_recycle_cached;
+ s->rx_pp_recycle_cache_full += rq_stats->pp_recycle_cache_full;
+ s->rx_pp_recycle_ring += rq_stats->pp_recycle_ring;
+ s->rx_pp_recycle_ring_full += rq_stats->pp_recycle_ring_full;
+ s->rx_pp_recycle_released_ref += rq_stats->pp_recycle_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 +485,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->pp_alloc_fast = stats.alloc_stats.fast;
+ rq_stats->pp_alloc_slow = stats.alloc_stats.slow;
+ rq_stats->pp_alloc_slow_high_order = stats.alloc_stats.slow_high_order;
+ rq_stats->pp_alloc_empty = stats.alloc_stats.empty;
+ rq_stats->pp_alloc_waive = stats.alloc_stats.waive;
+ rq_stats->pp_alloc_refill = stats.alloc_stats.refill;
+
+ rq_stats->pp_recycle_cached = stats.recycle_stats.cached;
+ rq_stats->pp_recycle_cache_full = stats.recycle_stats.cache_full;
+ rq_stats->pp_recycle_ring = stats.recycle_stats.ring;
+ rq_stats->pp_recycle_ring_full = stats.recycle_stats.ring_full;
+ rq_stats->pp_recycle_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 +524,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 +1949,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, pp_alloc_fast) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_slow) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_slow_high_order) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_empty) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_refill) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_alloc_waive) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_recycle_cached) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_recycle_cache_full) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_recycle_ring) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_recycle_ring_full) },
+ { MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats, pp_recycle_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..a7a025d 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_pp_alloc_fast;
+ u64 rx_pp_alloc_slow;
+ u64 rx_pp_alloc_slow_high_order;
+ u64 rx_pp_alloc_empty;
+ u64 rx_pp_alloc_refill;
+ u64 rx_pp_alloc_waive;
+ u64 rx_pp_recycle_cached;
+ u64 rx_pp_recycle_cache_full;
+ u64 rx_pp_recycle_ring;
+ u64 rx_pp_recycle_ring_full;
+ u64 rx_pp_recycle_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 pp_alloc_fast;
+ u64 pp_alloc_slow;
+ u64 pp_alloc_slow_high_order;
+ u64 pp_alloc_empty;
+ u64 pp_alloc_refill;
+ u64 pp_alloc_waive;
+ u64 pp_recycle_cached;
+ u64 pp_recycle_cache_full;
+ u64 pp_recycle_ring;
+ u64 pp_recycle_ring_full;
+ u64 pp_recycle_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] 12+ messages in thread
* Re: [net-next v8 1/4] page_pool: Add allocation stats
2022-03-01 22:10 ` [net-next v8 1/4] page_pool: Add allocation stats Joe Damato
@ 2022-03-01 23:50 ` Saeed Mahameed
2022-03-02 1:51 ` Joe Damato
0 siblings, 1 reply; 12+ messages in thread
From: Saeed Mahameed @ 2022-03-01 23:50 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, kuba, ilias.apalodimas, davem, hawk, ttoukan.linux,
brouer, leon, linux-rdma, saeedm
On 01 Mar 14:10, 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.
>
Sorry for the late review Joe,
Why disabled by default ? if your benchmarks showed no diff.
IMHO If we believe in this, we should have it enabled by default.
>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.
>
Let's have this documented under kernel documentation.
https://docs.kernel.org/networking/page_pool.html
I would also mention the kconfig and any user knobs APIs introduced in
this series
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v8 2/4] page_pool: Add recycle stats
2022-03-01 22:10 ` [net-next v8 2/4] page_pool: Add recycle stats Joe Damato
@ 2022-03-01 23:58 ` Saeed Mahameed
0 siblings, 0 replies; 12+ messages in thread
From: Saeed Mahameed @ 2022-03-01 23:58 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, kuba, ilias.apalodimas, davem, hawk, ttoukan.linux,
brouer, leon, linux-rdma, saeedm
On 01 Mar 14:10, 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
>
Kernel documentation.
[...]
>
>@@ -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;
> }
>
To avoid the ifdef, it makes more sense to refactor to:
if (!ret) {
recycle_stat_inc(pool, ring);
return true;
}
return false;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v8 4/4] mlx5: add support for page_pool_get_stats
2022-03-01 22:10 ` [net-next v8 4/4] mlx5: add support for page_pool_get_stats Joe Damato
@ 2022-03-02 1:02 ` Saeed Mahameed
2022-03-02 1:50 ` Joe Damato
0 siblings, 1 reply; 12+ messages in thread
From: Saeed Mahameed @ 2022-03-02 1:02 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, kuba, ilias.apalodimas, davem, hawk, ttoukan.linux,
brouer, leon, linux-rdma, saeedm
On 01 Mar 14:10, Joe Damato wrote:
>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.
>
I was hoping to see something other than ethtool, a driver-less approach,
page_pool is a first class citizen, it collects own stats and should be
able to report own stats without the need for driver help.
I understand these stats are per driver ring, but we can always come up with
a naming convention in the page pool to allow correlating page-pool stats
with per ring driver stats.
Anyway i can't think of a simple hack, so this patch is a good temporary
compromise until we come up with the right approach.
>Signed-off-by: Joe Damato <jdamato@fastly.com>
>---
> drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 75 ++++++++++++++++++++++
> drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 27 +++++++-
> 2 files changed, 101 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..eb518ec 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_pp_alloc_fast) },
>+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_slow) },
>+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_slow_high_order) },
>+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_empty) },
>+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_refill) },
>+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_waive) },
>+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_cached) },
>+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_cache_full) },
>+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_ring) },
>+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_ring_full) },
>+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_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,19 @@ 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_pp_alloc_fast += rq_stats->pp_alloc_fast;
>+ s->rx_pp_alloc_slow += rq_stats->pp_alloc_slow;
>+ s->rx_pp_alloc_empty += rq_stats->pp_alloc_empty;
>+ s->rx_pp_alloc_refill += rq_stats->pp_alloc_refill;
>+ s->rx_pp_alloc_waive += rq_stats->pp_alloc_waive;
>+ s->rx_pp_alloc_slow_high_order += rq_stats->pp_alloc_slow_high_order;
>+ s->rx_pp_recycle_cached += rq_stats->pp_recycle_cached;
>+ s->rx_pp_recycle_cache_full += rq_stats->pp_recycle_cache_full;
>+ s->rx_pp_recycle_ring += rq_stats->pp_recycle_ring;
>+ s->rx_pp_recycle_ring_full += rq_stats->pp_recycle_ring_full;
>+ s->rx_pp_recycle_released_ref += rq_stats->pp_recycle_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 +485,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 };
>+
you can drop the 0, just {} should be enough.
>+ if (!page_pool_get_stats(pool, &stats))
>+ return;
>+
you can contain the whole page_pool_stats objects inside rq_stats object,
and avoid all the assignments below.
just do:
page_pool_get_stats(pool, &rq_stats.pp);
return;
>+ rq_stats->pp_alloc_fast = stats.alloc_stats.fast;
>+ rq_stats->pp_alloc_slow = stats.alloc_stats.slow;
>+ rq_stats->pp_alloc_slow_high_order = stats.alloc_stats.slow_high_order;
>+ rq_stats->pp_alloc_empty = stats.alloc_stats.empty;
>+ rq_stats->pp_alloc_waive = stats.alloc_stats.waive;
>+ rq_stats->pp_alloc_refill = stats.alloc_stats.refill;
>+
>+ rq_stats->pp_recycle_cached = stats.recycle_stats.cached;
>+ rq_stats->pp_recycle_cache_full = stats.recycle_stats.cache_full;
>+ rq_stats->pp_recycle_ring = stats.recycle_stats.ring;
>+ rq_stats->pp_recycle_ring_full = stats.recycle_stats.ring_full;
>+ rq_stats->pp_recycle_released_ref = stats.recycle_stats.released_refcnt;
>+}
>+#else
>+static void mlx5e_stats_update_stats_rq_page_pool(struct mlx5e_channel *c)
>+{
>+}
>+#endif
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v8 4/4] mlx5: add support for page_pool_get_stats
2022-03-02 1:02 ` Saeed Mahameed
@ 2022-03-02 1:50 ` Joe Damato
2022-03-02 4:36 ` Saeed Mahameed
0 siblings, 1 reply; 12+ messages in thread
From: Joe Damato @ 2022-03-02 1:50 UTC (permalink / raw)
To: Saeed Mahameed
Cc: netdev, kuba, ilias.apalodimas, davem, hawk, ttoukan.linux,
brouer, leon, linux-rdma, saeedm
On Tue, Mar 1, 2022 at 5:02 PM Saeed Mahameed <saeed@kernel.org> wrote:
>
> On 01 Mar 14:10, Joe Damato wrote:
> >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.
> >
>
> I was hoping to see something other than ethtool, a driver-less approach,
> page_pool is a first class citizen, it collects own stats and should be
> able to report own stats without the need for driver help.
>
> I understand these stats are per driver ring, but we can always come up with
> a naming convention in the page pool to allow correlating page-pool stats
> with per ring driver stats.
>
> Anyway i can't think of a simple hack, so this patch is a good temporary
> compromise until we come up with the right approach.
>
> >Signed-off-by: Joe Damato <jdamato@fastly.com>
> >---
> > drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 75 ++++++++++++++++++++++
> > drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 27 +++++++-
> > 2 files changed, 101 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..eb518ec 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_pp_alloc_fast) },
> >+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_slow) },
> >+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_slow_high_order) },
> >+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_empty) },
> >+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_refill) },
> >+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_alloc_waive) },
> >+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_cached) },
> >+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_cache_full) },
> >+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_ring) },
> >+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_ring_full) },
> >+ { MLX5E_DECLARE_STAT(struct mlx5e_sw_stats, rx_pp_recycle_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,19 @@ 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_pp_alloc_fast += rq_stats->pp_alloc_fast;
> >+ s->rx_pp_alloc_slow += rq_stats->pp_alloc_slow;
> >+ s->rx_pp_alloc_empty += rq_stats->pp_alloc_empty;
> >+ s->rx_pp_alloc_refill += rq_stats->pp_alloc_refill;
> >+ s->rx_pp_alloc_waive += rq_stats->pp_alloc_waive;
> >+ s->rx_pp_alloc_slow_high_order += rq_stats->pp_alloc_slow_high_order;
> >+ s->rx_pp_recycle_cached += rq_stats->pp_recycle_cached;
> >+ s->rx_pp_recycle_cache_full += rq_stats->pp_recycle_cache_full;
> >+ s->rx_pp_recycle_ring += rq_stats->pp_recycle_ring;
> >+ s->rx_pp_recycle_ring_full += rq_stats->pp_recycle_ring_full;
> >+ s->rx_pp_recycle_released_ref += rq_stats->pp_recycle_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 +485,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 };
> >+
> you can drop the 0, just {} should be enough.
>
> >+ if (!page_pool_get_stats(pool, &stats))
> >+ return;
> >+
>
> you can contain the whole page_pool_stats objects inside rq_stats object,
> and avoid all the assignments below.
>
> just do:
> page_pool_get_stats(pool, &rq_stats.pp);
> return;
I don't think I can because the maximum stat name size is 32 bytes
(ETH_GSTRING_LEN).
If I do what you are suggesting, I would need to do something like:
{ MLX5E_DECLARE_RX_STAT(struct mlx5e_rq_stats,
pp.recycle_stats.released_refcnt) }
which will generate a string of the form:
"rx%d_pp.recycle_stats.released_refcnt" which is well over 32 bytes,
especially with double-digit queue numbers.
The only options I see are:
- A new define that allows setting a custom field name
(MLX5E_DECLARE_RX_STAT_NAME_OVERRIDE ?), or
- Leaving the code as-is
Can you let me know what you prefer for the v9?
Thanks,
Joe
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v8 1/4] page_pool: Add allocation stats
2022-03-01 23:50 ` Saeed Mahameed
@ 2022-03-02 1:51 ` Joe Damato
2022-03-02 4:32 ` Saeed Mahameed
0 siblings, 1 reply; 12+ messages in thread
From: Joe Damato @ 2022-03-02 1:51 UTC (permalink / raw)
To: Saeed Mahameed
Cc: netdev, kuba, ilias.apalodimas, davem, hawk, ttoukan.linux,
brouer, leon, linux-rdma, saeedm
On Tue, Mar 1, 2022 at 3:50 PM Saeed Mahameed <saeed@kernel.org> wrote:
>
> On 01 Mar 14:10, 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.
> >
>
> Sorry for the late review Joe,
No worries, thanks for taking a look.
> Why disabled by default ? if your benchmarks showed no diff.
>
> IMHO If we believe in this, we should have it enabled by default.
I think keeping it disabled by default makes sense for three reasons:
- The benchmarks on my hardware don't show a difference, but less
powerful hardware may be more greatly impacted.
- The new code uses more memory when enabled for storing the stats.
- These stats are useful for debugging and performance
investigations, but generally speaking I think the vast majority of
everyday kernel users won't be looking at this data.
Advanced users who need this information (and are willing to pay the
cost in memory and potentially CPU) can enable the code relatively
easily, so I think keeping it defaulted to off makes sense.
> >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.
> >
>
> Let's have this documented under kernel documentation.
> https://docs.kernel.org/networking/page_pool.html
>
> I would also mention the kconfig and any user knobs APIs introduced in
> this series
Sure, I can add a doc commit in the v9 that explains the kernel config
option, the API, and the fields of the stats structures.
Thanks,
Joe
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v8 1/4] page_pool: Add allocation stats
2022-03-02 1:51 ` Joe Damato
@ 2022-03-02 4:32 ` Saeed Mahameed
0 siblings, 0 replies; 12+ messages in thread
From: Saeed Mahameed @ 2022-03-02 4:32 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, kuba, ilias.apalodimas, davem, hawk, ttoukan.linux,
brouer, leon, linux-rdma, saeedm
On 01 Mar 17:51, Joe Damato wrote:
>On Tue, Mar 1, 2022 at 3:50 PM Saeed Mahameed <saeed@kernel.org> wrote:
>>
>> On 01 Mar 14:10, 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.
>> >
>>
>> Sorry for the late review Joe,
>
>No worries, thanks for taking a look.
>
>> Why disabled by default ? if your benchmarks showed no diff.
>>
>> IMHO If we believe in this, we should have it enabled by default.
>
>I think keeping it disabled by default makes sense for three reasons:
> - The benchmarks on my hardware don't show a difference, but less
>powerful hardware may be more greatly impacted.
> - The new code uses more memory when enabled for storing the stats.
> - These stats are useful for debugging and performance
>investigations, but generally speaking I think the vast majority of
>everyday kernel users won't be looking at this data.
>
>Advanced users who need this information (and are willing to pay the
>cost in memory and potentially CPU) can enable the code relatively
>easily, so I think keeping it defaulted to off makes sense.
I partially agree, since we have other means to detect if page_pool is
effective or not without these stats.
But here is my .02$: the difference in performance when page_pool is
effective and when isn't is huge, these counters are useful on production
systems when the page pool is under stress.
Simply put, the benefits of the page_pool outweigh the overhead of counting
(if even measurable), these counters should've been added long time ago.
if you are aiming for debug only counters then you should've introduced this
feature as a static key (tracepoints) to be enabled on the fly and the
overhead is paid only when enabled for the debug period.
Anyway, not a huge deal :).
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v8 4/4] mlx5: add support for page_pool_get_stats
2022-03-02 1:50 ` Joe Damato
@ 2022-03-02 4:36 ` Saeed Mahameed
0 siblings, 0 replies; 12+ messages in thread
From: Saeed Mahameed @ 2022-03-02 4:36 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, kuba, ilias.apalodimas, davem, hawk, ttoukan.linux,
brouer, leon, linux-rdma, saeedm
On 01 Mar 17:50, Joe Damato wrote:
>On Tue, Mar 1, 2022 at 5:02 PM Saeed Mahameed <saeed@kernel.org> wrote:
>>
[...]
>The only options I see are:
> - A new define that allows setting a custom field name
>(MLX5E_DECLARE_RX_STAT_NAME_OVERRIDE ?), or
> - Leaving the code as-is
>
>Can you let me know what you prefer for the v9?
>
ack, leave as is.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-03-02 4:37 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-01 22:10 [net-next v8 0/4] page_pool: Add stats counters Joe Damato
2022-03-01 22:10 ` [net-next v8 1/4] page_pool: Add allocation stats Joe Damato
2022-03-01 23:50 ` Saeed Mahameed
2022-03-02 1:51 ` Joe Damato
2022-03-02 4:32 ` Saeed Mahameed
2022-03-01 22:10 ` [net-next v8 2/4] page_pool: Add recycle stats Joe Damato
2022-03-01 23:58 ` Saeed Mahameed
2022-03-01 22:10 ` [net-next v8 3/4] page_pool: Add function to batch and return stats Joe Damato
2022-03-01 22:10 ` [net-next v8 4/4] mlx5: add support for page_pool_get_stats Joe Damato
2022-03-02 1:02 ` Saeed Mahameed
2022-03-02 1:50 ` Joe Damato
2022-03-02 4:36 ` Saeed Mahameed
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.