Re: [PATCH net-next v6 1/6] page_pool: frag API support for 32-bit arch with 64-bit DMA

From: Ilias Apalodimas
Date: Wed Aug 16 2023 - 07:27:57 EST

Next message: Muhammad Usama Anjum: "Re: [PATCH v31 2/6] fs/proc/task_mmu: Implement IOCTL to get and optionally clear info about PTEs"
Previous message: Tomi Valkeinen: "[PATCH v2 12/12] drm/bridge: tc358768: Default to positive h/v syncs"
Next in thread: Yunsheng Lin: "Re: [PATCH net-next v6 1/6] page_pool: frag API support for 32-bit arch with 64-bit DMA"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Yunsheng

On Mon, 14 Aug 2023 at 15:59, Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> Currently page_pool_alloc_frag() is not supported in 32-bit
> arch with 64-bit DMA because of the overlap issue between
> pp_frag_count and dma_addr_upper in 'struct page' for those
> arches, which seems to be quite common, see [1], which means
> driver may need to handle it when using frag API.

That wasn't so common. IIRC it was a single TI platform that was breaking?

>
> In order to simplify the driver's work when using frag API
> this patch allows page_pool_alloc_frag() to call
> page_pool_alloc_pages() to return pages for those arches.

Do we have any use cases of people needing this? Those architectures
should be long dead and although we have to support them in the
kernel, I don't personally see the advantage of adjusting the API to
do that. Right now we have a very clear separation between allocating
pages or fragments. Why should we hide a page allocation under a
frag allocation? A driver writer can simply allocate pages for those
boards. Am I the only one not seeing a clean win here?

Thanks
/Ilias

>
> Add a PP_FLAG_PAGE_SPLIT_IN_DRIVER flag in order to fail the
> page_pool creation for 32-bit arch with 64-bit DMA when driver
> tries to do the page splitting itself.
>
> Note that it may aggravate truesize underestimate problem for
> skb as there is no page splitting for those pages, if driver
> need a accurate truesize, it may calculate that according to
> frag size, page order and PAGE_POOL_DMA_USE_PP_FRAG_COUNT
> being true or not. And we may provide a helper for that if it
> turns out to be helpful.
>
> 1. https://lore.kernel.org/all/20211117075652.58299-1-linyunsheng@xxxxxxxxxx/
>
> Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
> CC: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> CC: Alexander Duyck <alexander.duyck@xxxxxxxxx>
> CC: Liang Chen <liangchen.linux@xxxxxxxxx>
> CC: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_main.c | 3 +-
> include/net/page_pool/helpers.h | 38 +++++++++++++++--
> include/net/page_pool/types.h | 42 ++++++++++++-------
> net/core/page_pool.c | 15 +++----
> 4 files changed, 68 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index bc9d5a5bea01..ec9c5a8cbda6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -834,7 +834,8 @@ static int mlx5e_alloc_rq(struct mlx5e_params *params,
> struct page_pool_params pp_params = { 0 };
>
> pp_params.order = 0;
> - pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV | PP_FLAG_PAGE_FRAG;
> + pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV |
> + PP_FLAG_PAGE_SPLIT_IN_DRIVER;
> pp_params.pool_size = pool_size;
> pp_params.nid = node;
> pp_params.dev = rq->pdev;
> diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h
> index 94231533a369..cb18de55f239 100644
> --- a/include/net/page_pool/helpers.h
> +++ b/include/net/page_pool/helpers.h
> @@ -29,8 +29,12 @@
> #ifndef _NET_PAGE_POOL_HELPERS_H
> #define _NET_PAGE_POOL_HELPERS_H
>
> +#include <linux/dma-mapping.h>
> #include <net/page_pool/types.h>
>
> +#define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \
> + (sizeof(dma_addr_t) > sizeof(unsigned long))
> +
> #ifdef CONFIG_PAGE_POOL_STATS
> int page_pool_ethtool_stats_get_count(void);
> u8 *page_pool_ethtool_stats_get_strings(u8 *data);
> @@ -73,6 +77,29 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
> return page_pool_alloc_pages(pool, gfp);
> }
>
> +static inline struct page *page_pool_alloc_frag(struct page_pool *pool,
> + unsigned int *offset,
> + unsigned int size, gfp_t gfp)
> +{
> + unsigned int max_size = PAGE_SIZE << pool->p.order;
> +
> + size = ALIGN(size, dma_get_cache_alignment());
> +
> + if (WARN_ON(size > max_size))
> + return NULL;
> +
> + /* Don't allow page splitting and allocate one big frag
> + * for 32-bit arch with 64-bit DMA, corresponding to
> + * the checking in page_pool_is_last_frag().
> + */
> + if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
> + *offset = 0;
> + return page_pool_alloc_pages(pool, gfp);
> + }
> +
> + return __page_pool_alloc_frag(pool, offset, size, gfp);
> +}
> +
> static inline struct page *page_pool_dev_alloc_frag(struct page_pool *pool,
> unsigned int *offset,
> unsigned int size)
> @@ -134,8 +161,14 @@ static inline long page_pool_defrag_page(struct page *page, long nr)
> static inline bool page_pool_is_last_frag(struct page_pool *pool,
> struct page *page)
> {
> - /* If fragments aren't enabled or count is 0 we were the last user */
> + /* We assume we are the last frag user that is still holding
> + * on to the page if:
> + * 1. Fragments aren't enabled.
> + * 2. We are running in 32-bit arch with 64-bit DMA.
> + * 3. page_pool_defrag_page() indicate we are the last user.
> + */
> return !(pool->p.flags & PP_FLAG_PAGE_FRAG) ||
> + PAGE_POOL_DMA_USE_PP_FRAG_COUNT ||
> (page_pool_defrag_page(page, 1) == 0);
> }
>
> @@ -197,9 +230,6 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
> page_pool_put_full_page(pool, page, true);
> }
>
> -#define PAGE_POOL_DMA_USE_PP_FRAG_COUNT \
> - (sizeof(dma_addr_t) > sizeof(unsigned long))
> -
> /**
> * page_pool_get_dma_addr() - Retrieve the stored DMA address.
> * @page: page allocated from a page pool
> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
> index 887e7946a597..079337c42aa6 100644
> --- a/include/net/page_pool/types.h
> +++ b/include/net/page_pool/types.h
> @@ -6,21 +6,29 @@
> #include <linux/dma-direction.h>
> #include <linux/ptr_ring.h>
>
> -#define PP_FLAG_DMA_MAP BIT(0) /* Should page_pool do the DMA
> - * map/unmap
> - */
> -#define PP_FLAG_DMA_SYNC_DEV BIT(1) /* If set all pages that the driver gets
> - * from page_pool will be
> - * DMA-synced-for-device according to
> - * the length provided by the device
> - * driver.
> - * Please note DMA-sync-for-CPU is still
> - * device driver responsibility
> - */
> -#define PP_FLAG_PAGE_FRAG BIT(2) /* for page frag feature */
> +/* Should page_pool do the DMA map/unmap */
> +#define PP_FLAG_DMA_MAP BIT(0)
> +
> +/* If set all pages that the driver gets from page_pool will be
> + * DMA-synced-for-device according to the length provided by the device driver.
> + * Please note DMA-sync-for-CPU is still device driver responsibility
> + */
> +#define PP_FLAG_DMA_SYNC_DEV BIT(1)
> +
> +/* for page frag feature */
> +#define PP_FLAG_PAGE_FRAG BIT(2)
> +
> +/* If set driver will do the page splitting itself. This is used to fail the
> + * page_pool creation because there is overlap issue between pp_frag_count and
> + * dma_addr_upper in 'struct page' for some arches with
> + * PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true.
> + */
> +#define PP_FLAG_PAGE_SPLIT_IN_DRIVER BIT(3)
> +
> #define PP_FLAG_ALL (PP_FLAG_DMA_MAP |\
> PP_FLAG_DMA_SYNC_DEV |\
> - PP_FLAG_PAGE_FRAG)
> + PP_FLAG_PAGE_FRAG |\
> + PP_FLAG_PAGE_SPLIT_IN_DRIVER)
>
> /*
> * Fast allocation side cache array/stack
> @@ -45,7 +53,8 @@ struct pp_alloc_cache {
>
> /**
> * struct page_pool_params - page pool parameters
> - * @flags: PP_FLAG_DMA_MAP, PP_FLAG_DMA_SYNC_DEV, PP_FLAG_PAGE_FRAG
> + * @flags: PP_FLAG_DMA_MAP, PP_FLAG_DMA_SYNC_DEV, PP_FLAG_PAGE_FRAG,
> + * PP_FLAG_PAGE_SPLIT_IN_DRIVER
> * @order: 2^order pages on allocation
> * @pool_size: size of the ptr_ring
> * @nid: NUMA node id to allocate from pages from
> @@ -183,8 +192,9 @@ struct page_pool {
> };
>
> struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
> -struct page *page_pool_alloc_frag(struct page_pool *pool, unsigned int *offset,
> - unsigned int size, gfp_t gfp);
> +struct page *__page_pool_alloc_frag(struct page_pool *pool,
> + unsigned int *offset, unsigned int size,
> + gfp_t gfp);
> struct page_pool *page_pool_create(const struct page_pool_params *params);
>
> struct xdp_mem_info;
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 77cb75e63aca..d62c11aaea9a 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -14,7 +14,6 @@
> #include <net/xdp.h>
>
> #include <linux/dma-direction.h>
> -#include <linux/dma-mapping.h>
> #include <linux/page-flags.h>
> #include <linux/mm.h> /* for put_page() */
> #include <linux/poison.h>
> @@ -212,7 +211,7 @@ static int page_pool_init(struct page_pool *pool,
> }
>
> if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
> - pool->p.flags & PP_FLAG_PAGE_FRAG)
> + pool->p.flags & PP_FLAG_PAGE_SPLIT_IN_DRIVER)
> return -EINVAL;
>
> #ifdef CONFIG_PAGE_POOL_STATS
> @@ -737,18 +736,16 @@ static void page_pool_free_frag(struct page_pool *pool)
> page_pool_return_page(pool, page);
> }
>
> -struct page *page_pool_alloc_frag(struct page_pool *pool,
> - unsigned int *offset,
> - unsigned int size, gfp_t gfp)
> +struct page *__page_pool_alloc_frag(struct page_pool *pool,
> + unsigned int *offset,
> + unsigned int size, gfp_t gfp)
> {
> unsigned int max_size = PAGE_SIZE << pool->p.order;
> struct page *page = pool->frag_page;
>
> - if (WARN_ON(!(pool->p.flags & PP_FLAG_PAGE_FRAG) ||
> - size > max_size))
> + if (WARN_ON(!(pool->p.flags & PP_FLAG_PAGE_FRAG))
> return NULL;
>
> - size = ALIGN(size, dma_get_cache_alignment());
> *offset = pool->frag_offset;
>
> if (page && *offset + size > max_size) {
> @@ -781,7 +778,7 @@ struct page *page_pool_alloc_frag(struct page_pool *pool,
> alloc_stat_inc(pool, fast);
> return page;
> }
> -EXPORT_SYMBOL(page_pool_alloc_frag);
> +EXPORT_SYMBOL(__page_pool_alloc_frag);
>
> static void page_pool_empty_ring(struct page_pool *pool)
> {
> --
> 2.33.0
>

Next message: Muhammad Usama Anjum: "Re: [PATCH v31 2/6] fs/proc/task_mmu: Implement IOCTL to get and optionally clear info about PTEs"
Previous message: Tomi Valkeinen: "[PATCH v2 12/12] drm/bridge: tc358768: Default to positive h/v syncs"
Next in thread: Yunsheng Lin: "Re: [PATCH net-next v6 1/6] page_pool: frag API support for 32-bit arch with 64-bit DMA"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]