Re: [PATCH net-next v11 0/6] introduce page_pool_alloc() related API
From: Yunsheng Lin <hidden>
Date: 2023-10-19 13:22:26
Also in:
bpf, linux-arm-kernel, linux-mediatek, lkml
Subsystem:
networking [general], page pool, the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jesper Dangaard Brouer, Ilias Apalodimas, Linus Torvalds
On 2023/10/18 23:35, Jakub Kicinski wrote:
On Wed, 18 Oct 2023 19:47:16 +0800 Yunsheng Lin wrote:quoted
quoted
mention it in the documentation. Plus the kdoc of the function should say that this is just a thin wrapper around other page pool APIs, and it's safe to mix it with other page pool APIs?I am not sure I understand what do 'safe' and 'mix' mean here. For 'safe' part, I suppose you mean if there is a va accociated with a 'struct page' without calling some API like kmap()? For that, I suppose it is safe when the driver is calling page_pool API without the __GFP_HIGHMEM flag. Maybe we should mention that in the kdoc and give a warning if page_pool_*alloc_va() is called with the __GFP_HIGHMEM flag?Sounds good. Warning wrapped in #if CONFIG_DEBUG_NET perhaps?
How about something like __get_free_pages() does with gfp flags? https://elixir.free-electrons.com/linux/v6.4-rc6/source/mm/page_alloc.c#L4818 how about something like below on top of this patchset:
diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h
index 7550beeacf3d..61cee55606c0 100644
--- a/include/net/page_pool/helpers.h
+++ b/include/net/page_pool/helpers.h@@ -167,13 +167,13 @@ static inline struct page *page_pool_dev_alloc(struct page_pool *pool, return page_pool_alloc(pool, offset, size, gfp); } -static inline void *page_pool_cache_alloc(struct page_pool *pool, - unsigned int *size, gfp_t gfp) +static inline void *page_pool_alloc_va(struct page_pool *pool, + unsigned int *size, gfp_t gfp) { unsigned int offset; struct page *page; - page = page_pool_alloc(pool, &offset, size, gfp); + page = page_pool_alloc(pool, &offset, size, gfp & ~__GFP_HIGHMEM); if (unlikely(!page)) return NULL;
@@ -181,21 +181,22 @@ static inline void *page_pool_cache_alloc(struct page_pool *pool, } /** - * page_pool_dev_cache_alloc() - allocate a cache. + * page_pool_dev_alloc_va() - allocate a page or a page fragment. * @pool: pool from which to allocate * @size: in as the requested size, out as the allocated size * - * Get a cache from the page allocator or page_pool caches. + * This is just a thin wrapper around the page_pool_alloc() API, and + * it returns va of the allocated page or page fragment. * * Return: - * Return the addr for the allocated cache, otherwise return NULL. + * Return the va for the allocated page or page fragment, otherwise return NULL. */ -static inline void *page_pool_dev_cache_alloc(struct page_pool *pool, - unsigned int *size) +static inline void *page_pool_dev_alloc_va(struct page_pool *pool, + unsigned int *size) { gfp_t gfp = (GFP_ATOMIC | __GFP_NOWARN); - return page_pool_cache_alloc(pool, size, gfp); + return page_pool_alloc_va(pool, size, gfp); } /**
@@ -338,17 +339,17 @@ static inline void page_pool_recycle_direct(struct page_pool *pool, (sizeof(dma_addr_t) > sizeof(unsigned long)) /** - * page_pool_cache_free() - free a cache into the page_pool - * @pool: pool from which cache was allocated - * @data: addr of cache to be free + * page_pool_free_va() - free a va into the page_pool + * @pool: pool from which va was allocated + * @va: va to be free * @allow_direct: freed by the consumer, allow lockless caching * * Free a cache allocated from page_pool_dev_cache_alloc(). */ -static inline void page_pool_cache_free(struct page_pool *pool, void *data, - bool allow_direct) +static inline void page_pool_free_va(struct page_pool *pool, void *va, + bool allow_direct) { - page_pool_put_page(pool, virt_to_head_page(data), -1, allow_direct); + page_pool_put_page(pool, virt_to_head_page(va), -1, allow_direct); }