net: page_pool: add netlink-based introspection

struct page_pool is rather performance critical and we use 16B of the first cache line to store 2 pointers used only by test code. Future patches will add more informational (non-fast path) attributes.

It's convenient for the user of the API to not have to worry which fields are fast and which are slow path. Use struct groups to split the params into the two categories internally.

Acked-by Jesper Dangaard Brouer <[email protected]>

Acked-by Ilias Apalodimas <[email protected]>

Reviewed-by Mina Almasry <[email protected]>

Signed-off-by Jakub Kicinski <[email protected]>

---
 include/net/page_pool/types.h | 31 +++++++++++++++++++------------
 net/core/page_pool.c          |  7 ++++---
 2 files changed, 23 insertions(+), 15 deletions(-)

include/net/page_pool/types.h +19 -12

index 6fc5134095ed..23950fcc4eca 100644
@@ -54,18 +54,22 @@ struct pp_alloc_cache {
  * @offset:    DMA sync address offset for PP_FLAG_DMA_SYNC_DEV
  */
 struct page_pool_params {
-       unsigned int    flags;
-       unsigned int    order;
-       unsigned int    pool_size;
-       int             nid;
-       struct device   *dev;
-       struct napi_struct *napi;
-       enum dma_data_direction dma_dir;
-       unsigned int    max_len;
-       unsigned int    offset;
+       struct_group_tagged(page_pool_params_fast, fast,
+               unsigned int    flags;
+               unsigned int    order;
+               unsigned int    pool_size;
+               int             nid;
+               struct device   *dev;
+               struct napi_struct *napi;
+               enum dma_data_direction dma_dir;
+               unsigned int    max_len;
+               unsigned int    offset;
+       );
+       struct_group_tagged(page_pool_params_slow, slow,
 /* private: used by test code only */
-       void (*init_callback)(struct page *page, void *arg);
-       void *init_arg;
+               void (*init_callback)(struct page *page, void *arg);
+               void *init_arg;
+       );
 };
 
 #ifdef CONFIG_PAGE_POOL_STATS

@@ -119,7 +123,7 @@ struct page_pool_stats {
 #endif
 
 struct page_pool {
-       struct page_pool_params p;
+       struct page_pool_params_fast p;
 
        long frag_users;
        struct page *frag_page;

@@ -178,6 +182,9 @@ struct page_pool {
        refcount_t user_cnt;
 
        u64 destroy_cnt;
+
+       /* Slow/Control-path information follows */
+       struct page_pool_params_slow slow;
 };
 
 struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);

net/core/page_pool.c +4 -4

index 5e409b98aba0..5cae413de7cc 100644
@@ -173,7 +173,8 @@ static int page_pool_init(struct page_pool *pool,
 {
        unsigned int ring_qsize = 1024; /* Default */
 
-       memcpy(&pool->p, params, sizeof(pool->p));
+       memcpy(&pool->p, &params->fast, sizeof(pool->p));
+       memcpy(&pool->slow, &params->slow, sizeof(pool->slow));
 
        /* Validate only known flags were used */
        if (pool->p.flags & ~(PP_FLAG_ALL))

@@ -384,8 +385,8 @@ static void page_pool_set_pp_info(struct page_pool *pool,
         * the overhead is negligible.
         */
        page_pool_fragment_page(page, 1);
-       if (pool->p.init_callback)
-               pool->p.init_callback(page, pool->p.init_arg);
+       if (pool->slow.init_callback)
+               pool->slow.init_callback(page, pool->slow.init_arg);
 }
 
 static void page_pool_clear_pp_info(struct page *page)
--

2.41.0

To fully benefit from previous commit add one byte of state in the first cache line recording if we need to look at the slow part.

The packing isn't all that impressive right now, we create a 7B hole. I'm expecting Olek's rework will reshuffle this, anyway.

Acked-by Jesper Dangaard Brouer <[email protected]>

Acked-by Ilias Apalodimas <[email protected]>

Reviewed-by Mina Almasry <[email protected]>

Signed-off-by Jakub Kicinski <[email protected]>

---
 include/net/page_pool/types.h | 2 ++
 net/core/page_pool.c          | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

include/net/page_pool/types.h +2 -0

index 23950fcc4eca..e1bb92c192de 100644
@@ -125,6 +125,8 @@ struct page_pool_stats {
 struct page_pool {
        struct page_pool_params_fast p;
 
+       bool has_init_callback;
+
        long frag_users;
        struct page *frag_page;
        unsigned int frag_offset;

net/core/page_pool.c +3 -2

index 5cae413de7cc..08af9de8e8eb 100644
@@ -212,6 +212,8 @@ static int page_pool_init(struct page_pool *pool,
                 */
        }
 
+       pool->has_init_callback = !!pool->slow.init_callback;
+
 #ifdef CONFIG_PAGE_POOL_STATS
        pool->recycle_stats = alloc_percpu(struct page_pool_recycle_stats);
        if (!pool->recycle_stats)

@@ -385,7 +387,7 @@ static void page_pool_set_pp_info(struct page_pool *pool,
         * the overhead is negligible.
         */
        page_pool_fragment_page(page, 1);
-       if (pool->slow.init_callback)
+       if (pool->has_init_callback)
                pool->slow.init_callback(page, pool->slow.init_arg);
 }
 
--

2.41.0

We'll soon (next change in the series) need a fuller unwind path in page_pool_create() so create the inverse of page_pool_init().

Reviewed-by Ilias Apalodimas <[email protected]>

Signed-off-by Jakub Kicinski <[email protected]>

---
 net/core/page_pool.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

net/core/page_pool.c +13 -9

index 08af9de8e8eb..4aed4809b5b4 100644
@@ -234,6 +234,18 @@ static int page_pool_init(struct page_pool *pool,
        return 0;
 }
 
+static void page_pool_uninit(struct page_pool *pool)
+{
+       ptr_ring_cleanup(&pool->ring, NULL);
+
+       if (pool->p.flags & PP_FLAG_DMA_MAP)
+               put_device(pool->p.dev);
+
+#ifdef CONFIG_PAGE_POOL_STATS
+       free_percpu(pool->recycle_stats);
+#endif
+}
+
 /**
  * page_pool_create() - create a page pool.
  * @params: parameters, see struct page_pool_params

@@ -817,14 +829,7 @@ static void __page_pool_destroy(struct page_pool *pool)
        if (pool->disconnect)
                pool->disconnect(pool);
 
-       ptr_ring_cleanup(&pool->ring, NULL);
-
-       if (pool->p.flags & PP_FLAG_DMA_MAP)
-               put_device(pool->p.dev);
-
-#ifdef CONFIG_PAGE_POOL_STATS
-       free_percpu(pool->recycle_stats);
-#endif
+       page_pool_uninit(pool);
        kfree(pool);
 }
 
--

2.41.0

To give ourselves the flexibility of creating netlink commands and ability to refer to page pool instances in uAPIs create IDs for page pools.

Signed-off-by Jakub Kicinski <[email protected]>

---
 include/net/page_pool/types.h |  4 ++++
 net/core/Makefile             |  2 +-
 net/core/page_pool.c          | 21 +++++++++++++++-----
 net/core/page_pool_priv.h     |  9 +++++++++
 net/core/page_pool_user.c     | 36 +++++++++++++++++++++++++++++++++++
 5 files changed, 66 insertions(+), 6 deletions(-)
 create mode 100644 net/core/page_pool_priv.h
 create mode 100644 net/core/page_pool_user.c

include/net/page_pool/types.h +4 -0

index e1bb92c192de..c19f0df3bf0b 100644
@@ -187,6 +187,10 @@ struct page_pool {
 
        /* Slow/Control-path information follows */
        struct page_pool_params_slow slow;
+       /* User-facing fields, protected by page_pools_lock */
+       struct {
+               u32 id;
+       } user;
 };
 
 struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);

net/core/Makefile +1 -1

index 0cb734cbc24b..821aec06abf1 100644
@@ -18,7 +18,7 @@ obj-y              += dev.o dev_addr_lists.o dst.o netevent.o \
 obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o
 
 obj-y += net-sysfs.o
-obj-$(CONFIG_PAGE_POOL) += page_pool.o
+obj-$(CONFIG_PAGE_POOL) += page_pool.o page_pool_user.o
 obj-$(CONFIG_PROC_FS) += net-procfs.o
 obj-$(CONFIG_NET_PKTGEN) += pktgen.o
 obj-$(CONFIG_NETPOLL) += netpoll.o

net/core/page_pool.c +16 -5

index 4aed4809b5b4..b3b484fbacae 100644
@@ -23,6 +23,8 @@
 
 #include <trace/events/page_pool.h>
 
+#include "page_pool_priv.h"
+
 #define DEFER_TIME (msecs_to_jiffies(1000))
 #define DEFER_WARN_INTERVAL (60 * HZ)
 

@@ -260,13 +262,21 @@ struct page_pool *page_pool_create(const struct page_pool_params *params)
                return ERR_PTR(-ENOMEM);
 
        err = page_pool_init(pool, params);
-       if (err < 0) {
-               pr_warn("%s() gave up with errno %d\n", __func__, err);
-               kfree(pool);
-               return ERR_PTR(err);
-       }
+       if (err < 0)
+               goto err_free;
+
+       err = page_pool_list(pool);
+       if (err)
+               goto err_uninit;
 
        return pool;
+
+err_uninit:
+       page_pool_uninit(pool);
+err_free:
+       pr_warn("%s() gave up with errno %d\n", __func__, err);
+       kfree(pool);
+       return ERR_PTR(err);
 }
 EXPORT_SYMBOL(page_pool_create);
 

@@ -829,6 +839,7 @@ static void __page_pool_destroy(struct page_pool *pool)
        if (pool->disconnect)
                pool->disconnect(pool);
 
+       page_pool_unlist(pool);
        page_pool_uninit(pool);
        kfree(pool);
 }

net/core/page_pool_priv.h new file +9 -0

new file mode 100644
index 000000000000..c17ea092b4ab
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __PAGE_POOL_PRIV_H
+#define __PAGE_POOL_PRIV_H
+
+int page_pool_list(struct page_pool *pool);
+void page_pool_unlist(struct page_pool *pool);
+
+#endif

net/core/page_pool_user.c new file +36 -1

new file mode 100644
index 000000000000..630d1eeecf2a
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/mutex.h>
+#include <linux/xarray.h>
+#include <net/page_pool/types.h>
+
+#include "page_pool_priv.h"
+
+static DEFINE_XARRAY_FLAGS(page_pools, XA_FLAGS_ALLOC1);
+static DEFINE_MUTEX(page_pools_lock);
+
+int page_pool_list(struct page_pool *pool)
+{
+       static u32 id_alloc_next;
+       int err;
+
+       mutex_lock(&page_pools_lock);
+       err = xa_alloc_cyclic(&page_pools, &pool->user.id, pool, xa_limit_32b,
+                             &id_alloc_next, GFP_KERNEL);
+       if (err < 0)
+               goto err_unlock;
+
+       mutex_unlock(&page_pools_lock);
+       return 0;
+
+err_unlock:
+       mutex_unlock(&page_pools_lock);
+       return err;
+}
+
+void page_pool_unlist(struct page_pool *pool)
+{
+       mutex_lock(&page_pools_lock);
+       xa_erase(&page_pools, pool->user.id);
+       mutex_unlock(&page_pools_lock);
+}
--

2.41.0

To give ourselves the flexibility of creating netlink commands and ability to refer to page pool instances in uAPIs create IDs for page pools.

Signed-off-by Jakub Kicinski <[email protected]>

---
 include/net/page_pool/types.h |  4 ++++
 net/core/Makefile             |  2 +-
 net/core/page_pool.c          | 21 +++++++++++++++-----
 net/core/page_pool_priv.h     |  9 +++++++++
 net/core/page_pool_user.c     | 36 +++++++++++++++++++++++++++++++++++
 5 files changed, 66 insertions(+), 6 deletions(-)
 create mode 100644 net/core/page_pool_priv.h
 create mode 100644 net/core/page_pool_user.c

net/core/page_pool_user.c new file +15 -1

+
+       mutex_unlock(&page_pools_lock);
+       return 0;
+
+err_unlock:
+       mutex_unlock(&page_pools_lock);
+       return err;
+}
+
+void page_pool_unlist(struct page_pool *pool)
+{
+       mutex_lock(&page_pools_lock);
+       xa_erase(&page_pools, pool->user.id);
+       mutex_unlock(&page_pools_lock);
+}
--

2.41.0

net/core/page_pool_user.c line 21

Quoted 5 lines

net/core/page_pool_user.c new file +5 -0

+       mutex_lock(&page_pools_lock);
+       err = xa_alloc_cyclic(&page_pools, &pool->user.id, pool, xa_limit_32b,
+                             &id_alloc_next, GFP_KERNEL);
+       if (err < 0)
+               goto err_unlock;  

A nit really, but get rid of the if/goto and just let this return err; ?

Quoted 7 lines · On Thu, 9 Nov 2023 11:21:32 +0200 Ilias Apalodimas wrote:

net/core/page_pool_user.c line 21

Quoted 5 lines

net/core/page_pool_user.c new file +5 -0

+       mutex_lock(&page_pools_lock);
+       err = xa_alloc_cyclic(&page_pools, &pool->user.id, pool, xa_limit_32b,
+                             &id_alloc_next, GFP_KERNEL);
+       if (err < 0)
+               goto err_unlock;

A nit really, but get rid of the if/goto and just let this return err; ?

There's more stuff added here by a subsequent patch. It ends up like this:

int page_pool_list(struct page_pool *pool)

{
        static u32 id_alloc_next;
        int err;

        mutex_lock(&page_pools_lock);
        err = xa_alloc_cyclic(&page_pools, &pool->user.id, pool, xa_limit_32b,
                              &id_alloc_next, GFP_KERNEL);
        if (err < 0)
                goto err_unlock;

        if (pool->slow.netdev) {
                hlist_add_head(&pool->user.list,
                               &pool->slow.netdev->page_pools);
                pool->user.napi_id = pool->p.napi ? pool->p.napi->napi_id : 0;

                netdev_nl_page_pool_event(pool, NETDEV_CMD_PAGE_POOL_ADD_NTF);
        }

        mutex_unlock(&page_pools_lock);
        return 0;

err_unlock:

        mutex_unlock(&page_pools_lock);
        return err;
}

Do you want me to combine the error and non-error paths? I have a weak preference for not mixing, sometimes err gets set to a positive value and that starts to propagate, unlikely to happen here tho.

Link the page pools with netdevs. This needs to be netns compatible so we have two options. Either we record the pools per netns and have to worry about moving them as the netdev gets moved. Or we record them directly on the netdev so they move with the netdev without any extra work.

Implement the latter option. Since pools may outlast netdev we need a place to store orphans. In time honored tradition use loopback for this purpose.

Quoted 6 lines · Jakub Kicinski

Link the page pools with netdevs. This needs to be netns compatible so we have two options. Either we record the pools per netns and have to worry about moving them as the netdev gets moved. Or we record them directly on the netdev so they move with the netdev without any extra work.

Quoted 3 lines

Implement the latter option. Since pools may outlast netdev we need a place to store orphans. In time honored tradition use loopback for this purpose.

blackhole_netdev might be a better choice than loopback

Quoted 7 lines · On Tue, 24 Oct 2023 11:31:46 -0600 David Ahern wrote:

Quoted 6 lines · Jakub Kicinski

Link the page pools with netdevs. This needs to be netns compatible so we have two options. Either we record the pools per netns and have to worry about moving them as the netdev gets moved. Or we record them directly on the netdev so they move with the netdev without any extra work.

Quoted 5 lines

Quoted 3 lines

Implement the latter option. Since pools may outlast netdev we need a place to store orphans. In time honored tradition use loopback for this purpose.

blackhole_netdev might be a better choice than loopback

With loopback we still keep it per netns, and still accessible via netlink.

Quoted 4 lines

Quoted 1 line

blackhole_netdev might be a better choice than loopback

With loopback we still keep it per netns, and still accessible via netlink.

and if the namespace and loopback device get deleted?

Link the page pools with netdevs. This needs to be netns compatible so we have two options. Either we record the pools per netns and have to worry about moving them as the netdev gets moved. Or we record them directly on the netdev so they move with the netdev without any extra work.

Implement the latter option. Since pools may outlast netdev we need a place to store orphans. In time honored tradition use loopback for this purpose.

Signed-off-by Jakub Kicinski <[email protected]>

---
v1: fix race between page pool and netdev disappearing (Simon)

---
 include/linux/list.h          | 20 ++++++++
 include/linux/netdevice.h     |  4 ++
 include/linux/poison.h        |  2 +
 include/net/page_pool/types.h |  4 ++
 net/core/page_pool_user.c     | 90 +++++++++++++++++++++++++++++++++++
 5 files changed, 120 insertions(+)

include/linux/list.h +20 -0

index 164b4d0e9d2a..3d8884472164 100644
@@ -1111,6 +1111,26 @@ static inline void hlist_move_list(struct hlist_head *old,
        old->first = NULL;
 }
 
+/**
+ * hlist_splice_init() - move all entries from one list to another
+ * @from: hlist_head from which entries will be moved
+ * @last: last entry on the @from list
+ * @to:   hlist_head to which entries will be moved
+ *
+ * @to can be empty, @from must contain at least @last.
+ */
+static inline void hlist_splice_init(struct hlist_head *from,
+                                    struct hlist_node *last,
+                                    struct hlist_head *to)
+{
+       if (to->first)
+               to->first->pprev = &last->next;
+       last->next = to->first;
+       to->first = from->first;
+       from->first->pprev = &to->first;
+       from->first = NULL;
+}
+
 #define hlist_entry(ptr, type, member) container_of(ptr,type,member)
 
 #define hlist_for_each(pos, head) \

include/linux/netdevice.h +0 -0

index b8bf669212cc..224ee4680a31 100644

include/linux/netdevice.h +0 -0

 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 

include/linux/poison.h +2 -0

index 851a855d3868..27a7dad17eef 100644
@@ -83,6 +83,8 @@
 
 /********** net/core/skbuff.c **********/
 #define SKB_LIST_POISON_NEXT   ((void *)(0x800 + POISON_POINTER_DELTA))
+/********** net/ **********/
+#define NET_PTR_POISON         ((void *)(0x801 + POISON_POINTER_DELTA))
 
 /********** kernel/bpf/ **********/
 #define BPF_PTR_POISON ((void *)(0xeB9FUL + POISON_POINTER_DELTA))

include/net/page_pool/types.h +1 -0

index c19f0df3bf0b..b258a571201e 100644
@@ -5,6 +5,7 @@
 
 #include <linux/dma-direction.h>
 #include <linux/ptr_ring.h>
+#include <linux/types.h>
 
 #define PP_FLAG_DMA_MAP                BIT(0) /* Should page_pool do the DMA
                                        * map/unmap

include/net/page_pool/types.h +2 -0

  * @napi:      NAPI which is the sole consumer of pages, otherwise NULL
  * @dma_dir:   DMA mapping direction
  * @max_len:   max DMA sync memory size for PP_FLAG_DMA_SYNC_DEV

@@ -66,6 +68,7 @@ struct page_pool_params {
                unsigned int    offset;
        );
        struct_group_tagged(page_pool_params_slow, slow,
+               struct net_device *netdev;
 /* private: used by test code only */
                void (*init_callback)(struct page *page, void *arg);
                void *init_arg;

@@ -189,6 +192,7 @@ struct page_pool {
        struct page_pool_params_slow slow;
        /* User-facing fields, protected by page_pools_lock */
        struct {
+               struct hlist_node list;
                u32 id;
        } user;
 };

net/core/page_pool_user.c +64 -0

index 630d1eeecf2a..7cd6f416b87a 100644
@@ -1,14 +1,31 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/mutex.h>
+#include <linux/netdevice.h>
 #include <linux/xarray.h>
+#include <net/net_debug.h>
 #include <net/page_pool/types.h>
 
 #include "page_pool_priv.h"
 
 static DEFINE_XARRAY_FLAGS(page_pools, XA_FLAGS_ALLOC1);
+/* Protects: page_pools, netdevice->page_pools, pool->slow.netdev, pool->user.
+ * Ordering: inside rtnl_lock
+ */
 static DEFINE_MUTEX(page_pools_lock);
 
+/* Page pools are only reachable from user space (via netlink) if they are
+ * linked to a netdev at creation time. Following page pool "visibility"
+ * states are possible:
+ *  - normal
+ *    - user.list: linked to real netdev, netdev: real netdev
+ *  - orphaned - real netdev has disappeared
+ *    - user.list: linked to lo, netdev: lo
+ *  - invisible - either (a) created without netdev linking, (b) unlisted due
+ *      to error, or (c) the entire namespace which owned this pool disappeared
+ *    - user.list: unhashed, netdev: unknown
+ */
+
 int page_pool_list(struct page_pool *pool)
 {
        static u32 id_alloc_next;

@@ -20,6 +37,10 @@ int page_pool_list(struct page_pool *pool)
        if (err < 0)
                goto err_unlock;
 
+       if (pool->slow.netdev)
+               hlist_add_head(&pool->user.list,
+                              &pool->slow.netdev->page_pools);
+
        mutex_unlock(&page_pools_lock);
        return 0;
 

@@ -32,5 +53,74 @@ void page_pool_unlist(struct page_pool *pool)
 {
        mutex_lock(&page_pools_lock);
        xa_erase(&page_pools, pool->user.id);
+       hlist_del(&pool->user.list);
        mutex_unlock(&page_pools_lock);
 }
+
+static void page_pool_unreg_netdev_wipe(struct net_device *netdev)
+{
+       struct page_pool *pool;
+       struct hlist_node *n;
+
+       mutex_lock(&page_pools_lock);
+       hlist_for_each_entry_safe(pool, n, &netdev->page_pools, user.list) {
+               hlist_del_init(&pool->user.list);
+               pool->slow.netdev = NET_PTR_POISON;
+       }
+       mutex_unlock(&page_pools_lock);
+}
+
+static void page_pool_unreg_netdev(struct net_device *netdev)
+{
+       struct page_pool *pool, *last;
+       struct net_device *lo;
+
+       lo = __dev_get_by_index(dev_net(netdev), 1);
+       if (!lo) {
+               netdev_err_once(netdev,
+                               "can't get lo to store orphan page pools\n");
+               page_pool_unreg_netdev_wipe(netdev);
+               return;
+       }
+
+       mutex_lock(&page_pools_lock);
+       last = NULL;
+       hlist_for_each_entry(pool, &netdev->page_pools, user.list) {
+               pool->slow.netdev = lo;
+               last = pool;
+       }
+       if (last)
+               hlist_splice_init(&netdev->page_pools, &last->user.list,
+                                 &lo->page_pools);
+       mutex_unlock(&page_pools_lock);
+}
+
+static int
+page_pool_netdevice_event(struct notifier_block *nb,
+                         unsigned long event, void *ptr)

net/core/page_pool_user.c +16 -1

+               page_pool_unreg_netdev(netdev);
+       else
+               page_pool_unreg_netdev_wipe(netdev);
+       return NOTIFY_OK;
+}
+
+static struct notifier_block page_pool_netdevice_nb = {
+       .notifier_call = page_pool_netdevice_event,
+};
+
+static int __init page_pool_user_init(void)
+{
+       return register_netdevice_notifier(&page_pool_netdevice_nb);
+}
+
+subsys_initcall(page_pool_user_init);
--

2.41.0

include/linux/netdevice.h struct net_device { · line 2437

Quoted 4 lines

include/linux/netdevice.h +4 -0

+#if IS_ENABLED(CONFIG_PAGE_POOL)
+       /** @page_pools: page pools created for this netdevice */
+       struct hlist_head       page_pools;
+#endif  

I wonder if this per netdev field is really necessary. Is it not possible to do the same simply looping over the (global) page_pools xarray? Or is that too silly of an idea. I guess on some systems you may end up with 100s or 1000s of active or orphaned page pools and then globally iterating over the whole page_pools xarray can be really slow..

include/net/page_pool/types.h struct pp_alloc_cache { · line 52

Quoted 5 lines

include/net/page_pool/types.h +1 -0

@@ -48,6 +49,7 @@ struct pp_alloc_cache {
  * @pool_size: size of the ptr_ring
  * @nid:       NUMA node id to allocate from pages from
  * @dev:       device, for DMA pre-mapping purposes
+ * @netdev:    netdev this pool will serve (leave as NULL if none or multiple)  

Is this an existing use case (page_pools that serve null or multiple netdevs), or a future use case? My understanding is that currently page_pools serve at most 1 rx-queue. Spot checking a few drivers that seems to be true.

I'm guessing 1 is _always_ loopback?

Quoted 11 lines · On Wed, 25 Oct 2023 12:56:44 -0700 Mina Almasry wrote:

include/linux/netdevice.h struct net_device { · line 2437

Quoted 4 lines

include/linux/netdevice.h +4 -0

+#if IS_ENABLED(CONFIG_PAGE_POOL)
+       /** @page_pools: page pools created for this netdevice */
+       struct hlist_head       page_pools;
+#endif

I wonder if this per netdev field is really necessary. Is it not possible to do the same simply looping over the (global) page_pools xarray? Or is that too silly of an idea. I guess on some systems you may end up with 100s or 1000s of active or orphaned page pools and then globally iterating over the whole page_pools xarray can be really slow..

I think we want the per-netdev hlist either way, on netdev unregistration we need to find its pools to clear the pointers. At which point we can as well use that to dump the pools.

I don't see a strong reason to use one approach over the other. Note that other objects like napi and queues (WIP patches) also walk netdevs and dump sub-objects from them.

Quoted 10 lines

include/net/page_pool/types.h struct pp_alloc_cache { · line 52

Quoted 5 lines

include/net/page_pool/types.h +1 -0

@@ -48,6 +49,7 @@ struct pp_alloc_cache {
  * @pool_size: size of the ptr_ring
  * @nid:       NUMA node id to allocate from pages from
  * @dev:       device, for DMA pre-mapping purposes
+ * @netdev:    netdev this pool will serve (leave as NULL if none or multiple)

Is this an existing use case (page_pools that serve null or multiple netdevs), or a future use case? My understanding is that currently page_pools serve at most 1 rx-queue. Spot checking a few drivers that seems to be true.

I think I saw one embedded driver for a switch-like device which has queues servicing all ports, and therefore netdevs.
We'd need some help from people using such devices to figure out what the right way to represent them is, and what extra bits of functionality they need.

Quoted 1 line

I'm guessing 1 is _always_ loopback?

AFAIK, yes. I should probably use LOOPBACK_IFINDEX, to make it clearer.

Link page pool instances to netdev for the drivers which already link to NAPI. Unless the driver is doing something very weird per-NAPI should imply per-netdev.

Signed-off-by Jakub Kicinski <[email protected]>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 1 +
 drivers/net/ethernet/microsoft/mana/mana_en.c     | 1 +
 3 files changed, 3 insertions(+)

Quoted 5 lines

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 1 +
 drivers/net/ethernet/microsoft/mana/mana_en.c     | 1 +
 3 files changed, 3 insertions(+)  

Mind add a line for netsec.c (probably in netsec_setup_rx_dring), since that's the only NIC I currently have around with pp support?

Quoted 6 lines · On Thu, 9 Nov 2023 11:11:04 +0200 Ilias Apalodimas wrote:

Quoted 5 lines

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 1 +
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 1 +
 drivers/net/ethernet/microsoft/mana/mana_en.c     | 1 +
 3 files changed, 3 insertions(+)

Quoted 2 lines

Mind add a line for netsec.c (probably in netsec_setup_rx_dring), since that's the only NIC I currently have around with pp support?

It's a single queue / single napi device? So like this?

drivers/net/ethernet/socionext/netsec.c +0 -0

        };
        int i, err;
 

Report when page pool was destroyed. Together with the inflight / memory use reporting this can serve as a replacement for the warning about leaked page pools we currently print to dmesg.

Example output for a fake leaked page pool using some hacks in netdevsim (one "live" pool, and one "leaked" on the same dev):

$ ./cli.py --no-schema --spec netlink/specs/netdev.yaml \
           --dump page-pool-get
[{'id': 2, 'ifindex': 3},
 {'id': 1, 'ifindex': 3, 'destroyed': 133, 'inflight': 1}]

Signed-off-by Jakub Kicinski <[email protected]>

---
 Documentation/netlink/specs/netdev.yaml |  9 +++++++++ include/net/page_pool/types.h           |  1 + include/uapi/linux/netdev.h             |  1 + net/core/page_pool.c                    |  1 + net/core/page_pool_priv.h               |  1 + net/core/page_pool_user.c               | 12 ++++++++++++

 6 files changed, 25 insertions(+)

Documentation/netlink/specs/netdev.yaml +0 -0

index 8d995760a14a..8be8f249bed3 100644
@@ -125,6 +125,14 @@ name: netdev

         type: uint
         doc: |
           Amount of memory held by inflight pages.
+      -
+        name: destroyed
+        type: uint
+        doc: |
+          Seconds in CLOCK_BOOTTIME of when Page Pool was destroyed. +          Page Pools wait for all the memory allocated from them to be freed +          before truly disappearing.
+          Absent if Page Pool hasn't been destroyed.

 operations:
   list:

Documentation/netlink/specs/netdev.yaml +0 -0

@@ -176,6 +184,7 @@ name: netdev

             - napi-id
             - inflight
             - inflight-mem
+            - destroyed
       dump:
         reply: *pp-reply
     -

include/net/page_pool/types.h +1 -0

index 7e47d7bb2c1e..f0c51ef5e345 100644
@@ -193,6 +193,7 @@ struct page_pool {
         /* User-facing fields, protected by page_pools_lock */
         struct {
                 struct hlist_node list;
+               u64 destroyed;
                 u32 napi_id;
                 u32 id;
         } user;

include/uapi/linux/netdev.h +1 -0

index 26ae5bdd3187..e5bf66d2aa31 100644
@@ -70,6 +70,7 @@ enum {
         NETDEV_A_PAGE_POOL_NAPI_ID,
         NETDEV_A_PAGE_POOL_INFLIGHT,
         NETDEV_A_PAGE_POOL_INFLIGHT_MEM,
+       NETDEV_A_PAGE_POOL_DESTROYED,

        __NETDEV_A_PAGE_POOL_MAX, NETDEV_A_PAGE_POOL_MAX = (__NETDEV_A_PAGE_POOL_MAX - 1)

net/core/page_pool.c +0 -0

index 30c8fc91fa66..57847fbb76a0 100644
@@ -949,6 +949,7 @@ void page_pool_destroy(struct page_pool *pool)
         if (!page_pool_release(pool))
                 return;

+       page_pool_destroyed(pool); pool->defer_start = jiffies; pool->defer_warn  = jiffies + DEFER_WARN_INTERVAL;

net/core/page_pool_priv.h +0 -0

index 72fb21ea1ddc..7fe6f842a270 100644
@@ -6,6 +6,7 @@
  s32 page_pool_inflight(const struct page_pool *pool, bool strict);

 int page_pool_list(struct page_pool *pool); +void page_pool_destroyed(struct page_pool *pool); void page_pool_unlist(struct page_pool *pool);
 #endif

net/core/page_pool_user.c +4 -0

index c971fe9eeb01..1fb5c3cbe412 100644
@@ -134,6 +134,10 @@ page_pool_nl_fill(struct sk_buff *rsp, const struct
 page_pool *pool,
             nla_put_uint(rsp, NETDEV_A_PAGE_POOL_INFLIGHT_MEM,
                          inflight * refsz))
                 goto err_cancel;
+       if (pool->user.destroyed &&
+           nla_put_uint(rsp, NETDEV_A_PAGE_POOL_DESTROYED,
+                        pool->user.destroyed))
+               goto err_cancel;

        genlmsg_end(rsp, hdr);

net/core/page_pool_user.c +0 -0

@@ -219,6 +223,14 @@ int page_pool_list(struct page_pool *pool)
         return err;
  }

net/core/page_pool_user.c +8 -0

+void page_pool_destroyed(struct page_pool *pool)
+{
+       mutex_lock(&page_pools_lock);
+       pool->user.destroyed = ktime_get_boottime_seconds();
+       netdev_nl_page_pool_event(pool, NETDEV_CMD_PAGE_POOL_CHANGE_NTF);
+       mutex_unlock(&page_pools_lock);
+}
+

 void page_pool_unlist(struct page_pool *pool)
 {
        mutex_lock(&page_pools_lock);

This is a new revision of the RFC posted in August:
https://lore.kernel.org/all/[email protected]/
There's been a handful of fixes and tweaks but the overall architecture is unchanged.

As a reminder the RFC was posted as the first step towards an API which could configure the page pools (GET API as a stepping stone for a SET API to come later). I wasn't sure whether we should commit to the GET API before the SET takes shape, hence the large delay between versions.

Unfortunately, real deployment experience made this series much more urgent. We recently started to deploy newer kernels / drivers at Meta, making significant use of page pools for the first time.

We immediately run into page pool leaks both real and false positive warnings. As Eric pointed out/predicted there's no guarantee that applications will read / close their sockets so a page pool page may be stuck in a socket (but not leaked) forever. This happens a lot in our fleet. Most of these are obviously due to application bugs but we should not be printing kernel warnings due to minor application resource leaks.

Conversely the page pool memory may get leaked at runtime, and we have no way to detect / track that, unless someone reconfigures the NIC and destroys the page pools which leaked the pages.

The solution presented here is to expose the memory use of page pools via netlink. This allows for continuous monitoring of memory used by page pools, regardless if they were destroyed or not. Sample in patch 15 can print the memory use and recycling efficiency:

$ ./page-pool
    eth0[2]     page pools: 10 (zombies: 0)
                refs: 41984 bytes: 171966464 (refs: 0 bytes: 0)
                recycling: 90.3% (alloc: 656:397681 recycle: 89652:270201)

The main change compared to the RFC is that the API now exposes outstanding references and byte counts even for "live" page pools. The warning is no longer printed if page pool is accessible via netlink.

Jakub Kicinski (15):
net: page_pool: split the page_pool_params into fast and slow
net: page_pool: avoid touching slow on the fastpath
net: page_pool: factor out uninit
net: page_pool: id the page pools
net: page_pool: record pools per netdev
net: page_pool: stash the NAPI ID for easier access
eth: link netdev to page_pools in drivers
net: page_pool: add nlspec for basic access to page pools
net: page_pool: implement GET in the netlink API
net: page_pool: add netlink notifications for state changes
net: page_pool: report amount of memory held by page pools
net: page_pool: report when page pool was destroyed
net: page_pool: expose page pool stats via netlink
net: page_pool: mute the periodic warning for visible page pools
tools: ynl: add sample for getting page-pool information

 Documentation/netlink/specs/netdev.yaml       | 161 +++++++
 Documentation/networking/page_pool.rst        |  10 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   1 +
 .../net/ethernet/mellanox/mlx5/core/en_main.c |   1 +
 drivers/net/ethernet/microsoft/mana/mana_en.c |   1 +
 include/linux/list.h                          |  20 +
 include/linux/netdevice.h                     |   4 +
 include/linux/poison.h                        |   2 +
 include/net/page_pool/helpers.h               |   8 +-
 include/net/page_pool/types.h                 |  43 +-
 include/uapi/linux/netdev.h                   |  36 ++
 net/core/Makefile                             |   2 +-
 net/core/netdev-genl-gen.c                    |  52 +++
 net/core/netdev-genl-gen.h                    |  11 +
 net/core/page_pool.c                          |  78 ++--
 net/core/page_pool_priv.h                     |  12 +
 net/core/page_pool_user.c                     | 414 +++++++++++++++++
 tools/include/uapi/linux/netdev.h             |  36 ++
 tools/net/ynl/generated/netdev-user.c         | 419 ++++++++++++++++++
 tools/net/ynl/generated/netdev-user.h         | 171 +++++++
 tools/net/ynl/lib/ynl.h                       |   2 +-
 tools/net/ynl/samples/.gitignore              |   1 +
 tools/net/ynl/samples/Makefile                |   2 +-
 tools/net/ynl/samples/page-pool.c             | 147 ++++++
 24 files changed, 1586 insertions(+), 48 deletions(-)
 create mode 100644 net/core/page_pool_priv.h
 create mode 100644 net/core/page_pool_user.c
 create mode 100644 tools/net/ynl/samples/page-pool.c

--
2.41.0

Quoted 7 lines

We immediately run into page pool leaks both real and false positive warnings. As Eric pointed out/predicted there's no guarantee that applications will read / close their sockets so a page pool page may be stuck in a socket (but not leaked) forever. This happens a lot in our fleet. Most of these are obviously due to application bugs but we should not be printing kernel warnings due to minor application resource leaks.

Fair enough, I guess you mean 'continuous warnings'?

Quoted 10 lines

Conversely the page pool memory may get leaked at runtime, and we have no way to detect / track that, unless someone reconfigures the NIC and destroys the page pools which leaked the pages.

The solution presented here is to expose the memory use of page pools via netlink. This allows for continuous monitoring of memory used by page pools, regardless if they were destroyed or not. Sample in patch 15 can print the memory use and recycling efficiency:

Quoted 4 lines

$ ./page-pool
    eth0[2]     page pools: 10 (zombies: 0)
                refs: 41984 bytes: 171966464 (refs: 0 bytes: 0)
                recycling: 90.3% (alloc: 656:397681 recycle: 89652:270201)

That's reasonable, and the recycling rate is pretty impressive.

Any idea how that translated to enhancements overall? mem/cpu pressure etc