Re: [PATCH net-next v11 0/6] introduce page_pool_alloc() related API

[Yunsheng Lin]

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);
 }