| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 7 Jul 2023 12:50:51 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, hawk@...nel.org, ilias.apalodimas@...aro.org,
edumazet@...gle.com, dsahern@...il.com, michael.chan@...adcom.com,
willemb@...gle.com
Subject: Re: [RFC 06/12] net: page_pool: create hooks for custom page providers
On Fri, Jul 7, 2023 at 11:39 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> The page providers which try to reuse the same pages will
> need to hold onto the ref, even if page gets released from
> the pool - as in releasing the page from the pp just transfers
> the "ownership" reference from pp to the provider, and provider
> will wait for other references to be gone before feeding this
> page back into the pool.
>
> The rest if pretty obvious.
>
> Add a test provider which should behave identically to
> a normal page pool.
>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> include/net/page_pool.h | 20 +++++++++++
> net/core/page_pool.c | 80 +++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 97 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index b082c9118f05..5859ab838ed2 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -77,6 +77,7 @@ struct page_pool_params {
> int nid; /* Numa node id to allocate from pages from */
> struct device *dev; /* device, for DMA pre-mapping purposes */
> struct napi_struct *napi; /* Sole consumer of pages, otherwise NULL */
> + u8 memory_provider; /* haaacks! should be user-facing */
> enum dma_data_direction dma_dir; /* DMA mapping direction */
> unsigned int max_len; /* max DMA sync memory size */
> unsigned int offset; /* DMA addr offset */
> @@ -147,6 +148,22 @@ static inline u64 *page_pool_ethtool_stats_get(u64 *data, void *stats)
>
> #endif
>
> +struct mem_provider;
> +
> +enum pp_memory_provider_type {
> + __PP_MP_NONE, /* Use system allocator directly */
> + PP_MP_BASIC, /* Test purposes only, Hacky McHackface */
> +};
> +
> +struct pp_memory_provider_ops {
> + int (*init)(struct page_pool *pool);
> + void (*destroy)(struct page_pool *pool);
> + struct page *(*alloc_pages)(struct page_pool *pool, gfp_t gfp);
> + bool (*release_page)(struct page_pool *pool, struct page *page);
> +};
> +
> +extern const struct pp_memory_provider_ops basic_ops;
> +
> struct page_pool {
> struct page_pool_params p;
>
> @@ -194,6 +211,9 @@ struct page_pool {
> */
> struct ptr_ring ring;
>
> + const struct pp_memory_provider_ops *mp_ops;
> + void *mp_priv;
> +
> #ifdef CONFIG_PAGE_POOL_STATS
> /* recycle stats are per-cpu to avoid locking */
> struct page_pool_recycle_stats __percpu *recycle_stats;
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 09f8c34ad4a7..e886a439f9bb 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -23,6 +23,8 @@
>
> #include <trace/events/page_pool.h>
>
> +static DEFINE_STATIC_KEY_FALSE(page_pool_mem_providers);
> +
> #define DEFER_TIME (msecs_to_jiffies(1000))
> #define DEFER_WARN_INTERVAL (60 * HZ)
>
> @@ -161,6 +163,7 @@ static int page_pool_init(struct page_pool *pool,
> const struct page_pool_params *params)
> {
> unsigned int ring_qsize = 1024; /* Default */
> + int err;
>
> memcpy(&pool->p, params, sizeof(pool->p));
>
> @@ -218,10 +221,36 @@ static int page_pool_init(struct page_pool *pool,
> /* Driver calling page_pool_create() also call page_pool_destroy() */
> refcount_set(&pool->user_cnt, 1);
>
> + switch (pool->p.memory_provider) {
> + case __PP_MP_NONE:
> + break;
> + case PP_MP_BASIC:
> + pool->mp_ops = &basic_ops;
> + break;
> + default:
> + err = -EINVAL;
> + goto free_ptr_ring;
> + }
> +
> + if (pool->mp_ops) {
> + err = pool->mp_ops->init(pool);
> + if (err) {
> + pr_warn("%s() mem-provider init failed %d\n",
> + __func__, err);
> + goto free_ptr_ring;
> + }
> +
> + static_branch_inc(&page_pool_mem_providers);
> + }
> +
> if (pool->p.flags & PP_FLAG_DMA_MAP)
> get_device(pool->p.dev);
>
> return 0;
> +
> +free_ptr_ring:
> + ptr_ring_cleanup(&pool->ring, NULL);
> + return err;
> }
>
> struct page_pool *page_pool_create(const struct page_pool_params *params)
> @@ -463,7 +492,10 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp)
> return page;
>
> /* Slow-path: cache empty, do real allocation */
> - page = __page_pool_alloc_pages_slow(pool, gfp);
> + if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops)
> + page = pool->mp_ops->alloc_pages(pool, gfp);
> + else
> + page = __page_pool_alloc_pages_slow(pool, gfp);
> return page;
> }
> EXPORT_SYMBOL(page_pool_alloc_pages);
> @@ -515,8 +547,13 @@ void __page_pool_release_page_dma(struct page_pool *pool, struct page *page)
> void page_pool_return_page(struct page_pool *pool, struct page *page)
> {
> int count;
> + bool put;
>
> - __page_pool_release_page_dma(pool, page);
> + put = true;
> + if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops)
> + put = pool->mp_ops->release_page(pool, page);
> + else
> + __page_pool_release_page_dma(pool, page);
>
> page_pool_clear_pp_info(page);
>
> @@ -526,7 +563,8 @@ void page_pool_return_page(struct page_pool *pool, struct page *page)
> count = atomic_inc_return_relaxed(&pool->pages_state_release_cnt);
> trace_page_pool_state_release(pool, page, count);
>
> - put_page(page);
> + if (put)
> + put_page(page);
+1 to giving memory providers the option to replace put_page() with a
custom release function. In your original proposal, the put_page() was
intact, and I thought it was some requirement from you that pages must
be freed with put_page(). I made my code with/around that, but I think
it's nice to give future memory providers the option to replace this.
> /* An optimization would be to call __free_pages(page, pool->p.order)
> * knowing page is not part of page-cache (thus avoiding a
> * __page_cache_release() call).
> @@ -779,6 +817,11 @@ static void page_pool_free(struct page_pool *pool)
> if (pool->disconnect)
> pool->disconnect(pool);
>
> + if (pool->mp_ops) {
> + pool->mp_ops->destroy(pool);
> + static_branch_dec(&page_pool_mem_providers);
> + }
> +
> ptr_ring_cleanup(&pool->ring, NULL);
>
> if (pool->p.flags & PP_FLAG_DMA_MAP)
> @@ -952,3 +995,34 @@ bool page_pool_return_skb_page(struct page *page, bool napi_safe)
> return true;
> }
> EXPORT_SYMBOL(page_pool_return_skb_page);
> +
> +/***********************
> + * Mem provider hack *
> + ***********************/
> +
> +static int mp_basic_init(struct page_pool *pool)
> +{
> + return 0;
> +}
> +
> +static void mp_basic_destroy(struct page_pool *pool)
> +{
> +}
> +
> +static struct page *mp_basic_alloc_pages(struct page_pool *pool, gfp_t gfp)
> +{
> + return __page_pool_alloc_pages_slow(pool, gfp);
> +}
> +
> +static bool mp_basic_release(struct page_pool *pool, struct page *page)
> +{
> + __page_pool_release_page_dma(pool, page);
> + return true;
> +}
> +
> +const struct pp_memory_provider_ops basic_ops = {
> + .init = mp_basic_init,
> + .destroy = mp_basic_destroy,
> + .alloc_pages = mp_basic_alloc_pages,
> + .release_page = mp_basic_release,
> +};
> --
> 2.41.0
>
--
Thanks,
Mina
Powered by blists - more mailing lists