On 2023/10/18 23:35, Jakub Kicinski wrote: > On Wed, 18 Oct 2023 19:47:16 +0800 Yunsheng Lin wrote: >>> 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); }
