| Message ID | 20231122034420.1158898-10-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: page_pool: add netlink-based introspection | expand |
On Wed, Nov 22, 2023 at 4:44 AM Jakub Kicinski <kuba@kernel.org> wrote: > > Advanced deployments need the ability to check memory use > of various system components. It makes it possible to make informed > decisions about memory allocation and to find regressions and leaks. > > Report memory use of page pools. Report both number of references > and bytes held. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > Documentation/netlink/specs/netdev.yaml | 13 +++++++++++++ > include/uapi/linux/netdev.h | 2 ++ > net/core/page_pool.c | 13 +++++++++---- > net/core/page_pool_priv.h | 2 ++ > net/core/page_pool_user.c | 8 ++++++++ > 5 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml > index 82fbe81f7a49..85209e19dca9 100644 > --- a/Documentation/netlink/specs/netdev.yaml > +++ b/Documentation/netlink/specs/netdev.yaml > @@ -114,6 +114,17 @@ name: netdev > checks: > min: 1 > max: u32-max > + - > + name: inflight > + type: uint > + doc: | > + Number of outstanding references to this page pool (allocated > + but yet to be freed pages). > + - > + name: inflight-mem > + type: uint 4GB limit seems small, should not we make this 64bit right away ? > + doc: | > + Amount of memory held by inflight pages. > Thanks.
On 11/22/23 04:44, Jakub Kicinski wrote: > Advanced deployments need the ability to check memory use > of various system components. It makes it possible to make informed > decisions about memory allocation and to find regressions and leaks. > > Report memory use of page pools. Report both number of references > and bytes held. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > Documentation/netlink/specs/netdev.yaml | 13 +++++++++++++ > include/uapi/linux/netdev.h | 2 ++ > net/core/page_pool.c | 13 +++++++++---- > net/core/page_pool_priv.h | 2 ++ > net/core/page_pool_user.c | 8 ++++++++ > 5 files changed, 34 insertions(+), 4 deletions(-) > I like it, but see comment/suggestion below. Acked-by: Jesper Dangaard Brouer <hawk@kernel.org> > diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml > index 82fbe81f7a49..85209e19dca9 100644 > --- a/Documentation/netlink/specs/netdev.yaml > +++ b/Documentation/netlink/specs/netdev.yaml > @@ -114,6 +114,17 @@ name: netdev > checks: > min: 1 > max: u32-max > + - > + name: inflight > + type: uint > + doc: | > + Number of outstanding references to this page pool (allocated > + but yet to be freed pages). Maybe it is worth explaining in this doc that these inflight references also cover elements in (ptr) ring (and alloc-cache) ? In a follow up patchset, we likely also want to expose the PP ring size. As that could be relevant when assessing inflight number. --Jesper > + - > + name: inflight-mem > + type: uint > + doc: | > + Amount of memory held by inflight pages. > > operations: > list: > @@ -163,6 +174,8 @@ name: netdev > - id > - ifindex > - napi-id > + - inflight > + - inflight-mem > dump: > reply: *pp-reply > config-cond: page-poo
On Wed, 22 Nov 2023 11:16:48 +0100 Eric Dumazet wrote: > > + - > > + name: inflight-mem > > + type: uint > > 4GB limit seems small, should not we make this 64bit right away ? Yes, uint is my magic auto-sized integer which can be either 32b or 64b depending on the value. See commit 374d345d9b5e and 7d4caf54d2e.
On Wed, 22 Nov 2023 16:24:55 +0100 Jesper Dangaard Brouer wrote: > > + - > > + name: inflight > > + type: uint > > + doc: | > > + Number of outstanding references to this page pool (allocated > > + but yet to be freed pages). > > Maybe it is worth explaining in this doc that these inflight references > also cover elements in (ptr) ring (and alloc-cache) ? > > In a follow up patchset, we likely also want to expose the PP ring size. > As that could be relevant when assessing inflight number. Good point, how about: Number of outstanding references to this page pool (allocated but yet to be freed pages). Allocated pages may be held in socket receive queues, driver receive rings, page pool recycling ring, the page pool cache, etc.
On 11/22/23 17:06, Jakub Kicinski wrote: > On Wed, 22 Nov 2023 16:24:55 +0100 Jesper Dangaard Brouer wrote: >>> + - >>> + name: inflight >>> + type: uint >>> + doc: | >>> + Number of outstanding references to this page pool (allocated >>> + but yet to be freed pages). >> >> Maybe it is worth explaining in this doc that these inflight references >> also cover elements in (ptr) ring (and alloc-cache) ? >> >> In a follow up patchset, we likely also want to expose the PP ring size. >> As that could be relevant when assessing inflight number. > > Good point, how about: > > Number of outstanding references to this page pool (allocated > but yet to be freed pages). Allocated pages may be held in > socket receive queues, driver receive rings, page pool recycling > ring, the page pool cache, etc. Sound good to me :-) - ACK --Jesper
On Wed, Nov 22, 2023 at 5:03 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 22 Nov 2023 11:16:48 +0100 Eric Dumazet wrote: > > > + - > > > + name: inflight-mem > > > + type: uint > > > > 4GB limit seems small, should not we make this 64bit right away ? > > Yes, uint is my magic auto-sized integer which can be either 32b or 64b > depending on the value. See commit 374d345d9b5e and 7d4caf54d2e. Ah, nice ;)
diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml index 82fbe81f7a49..85209e19dca9 100644 --- a/Documentation/netlink/specs/netdev.yaml +++ b/Documentation/netlink/specs/netdev.yaml @@ -114,6 +114,17 @@ name: netdev checks: min: 1 max: u32-max + - + name: inflight + type: uint + doc: | + Number of outstanding references to this page pool (allocated + but yet to be freed pages). + - + name: inflight-mem + type: uint + doc: | + Amount of memory held by inflight pages. operations: list: @@ -163,6 +174,8 @@ name: netdev - id - ifindex - napi-id + - inflight + - inflight-mem dump: reply: *pp-reply config-cond: page-pool diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h index beb158872226..26ae5bdd3187 100644 --- a/include/uapi/linux/netdev.h +++ b/include/uapi/linux/netdev.h @@ -68,6 +68,8 @@ enum { NETDEV_A_PAGE_POOL_ID = 1, NETDEV_A_PAGE_POOL_IFINDEX, NETDEV_A_PAGE_POOL_NAPI_ID, + NETDEV_A_PAGE_POOL_INFLIGHT, + NETDEV_A_PAGE_POOL_INFLIGHT_MEM, __NETDEV_A_PAGE_POOL_MAX, NETDEV_A_PAGE_POOL_MAX = (__NETDEV_A_PAGE_POOL_MAX - 1) diff --git a/net/core/page_pool.c b/net/core/page_pool.c index a8d96ea38d18..566390759294 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -529,7 +529,7 @@ EXPORT_SYMBOL(page_pool_alloc_pages); */ #define _distance(a, b) (s32)((a) - (b)) -static s32 page_pool_inflight(struct page_pool *pool) +s32 page_pool_inflight(const struct page_pool *pool, bool strict) { u32 release_cnt = atomic_read(&pool->pages_state_release_cnt); u32 hold_cnt = READ_ONCE(pool->pages_state_hold_cnt); @@ -537,8 +537,13 @@ static s32 page_pool_inflight(struct page_pool *pool) inflight = _distance(hold_cnt, release_cnt); - trace_page_pool_release(pool, inflight, hold_cnt, release_cnt); - WARN(inflight < 0, "Negative(%d) inflight packet-pages", inflight); + if (strict) { + trace_page_pool_release(pool, inflight, hold_cnt, release_cnt); + WARN(inflight < 0, "Negative(%d) inflight packet-pages", + inflight); + } else { + inflight = max(0, inflight); + } return inflight; } @@ -881,7 +886,7 @@ static int page_pool_release(struct page_pool *pool) int inflight; page_pool_scrub(pool); - inflight = page_pool_inflight(pool); + inflight = page_pool_inflight(pool, true); if (!inflight) __page_pool_destroy(pool); diff --git a/net/core/page_pool_priv.h b/net/core/page_pool_priv.h index c17ea092b4ab..72fb21ea1ddc 100644 --- a/net/core/page_pool_priv.h +++ b/net/core/page_pool_priv.h @@ -3,6 +3,8 @@ #ifndef __PAGE_POOL_PRIV_H #define __PAGE_POOL_PRIV_H +s32 page_pool_inflight(const struct page_pool *pool, bool strict); + int page_pool_list(struct page_pool *pool); void page_pool_unlist(struct page_pool *pool); diff --git a/net/core/page_pool_user.c b/net/core/page_pool_user.c index 35c56fb41c46..d889b347f8f4 100644 --- a/net/core/page_pool_user.c +++ b/net/core/page_pool_user.c @@ -110,6 +110,7 @@ static int page_pool_nl_fill(struct sk_buff *rsp, const struct page_pool *pool, const struct genl_info *info) { + size_t inflight, refsz; void *hdr; hdr = genlmsg_iput(rsp, info); @@ -127,6 +128,13 @@ page_pool_nl_fill(struct sk_buff *rsp, const struct page_pool *pool, nla_put_uint(rsp, NETDEV_A_PAGE_POOL_NAPI_ID, pool->user.napi_id)) goto err_cancel; + inflight = page_pool_inflight(pool, false); + refsz = PAGE_SIZE << pool->p.order; + if (nla_put_uint(rsp, NETDEV_A_PAGE_POOL_INFLIGHT, inflight) || + nla_put_uint(rsp, NETDEV_A_PAGE_POOL_INFLIGHT_MEM, + inflight * refsz)) + goto err_cancel; + genlmsg_end(rsp, hdr); return 0;
Advanced deployments need the ability to check memory use of various system components. It makes it possible to make informed decisions about memory allocation and to find regressions and leaks. Report memory use of page pools. Report both number of references and bytes held. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- Documentation/netlink/specs/netdev.yaml | 13 +++++++++++++ include/uapi/linux/netdev.h | 2 ++ net/core/page_pool.c | 13 +++++++++---- net/core/page_pool_priv.h | 2 ++ net/core/page_pool_user.c | 8 ++++++++ 5 files changed, 34 insertions(+), 4 deletions(-)