On 2024/12/19 11:11, Guowei Dang wrote:
> Add page_pool_put_page_nosync() to respond to dma_sync_size being 0.
>
> The purpose of this is to make the semantics more obvious and may
> enable removing some checkings in the future.
It would be good to describe the actual use case of the above API in
the commit log too.
>
> And in the long term, treating the nosync scenario separately provides
> more flexibility for the user and enable removing of the
> PP_FLAG_DMA_SYNC_DEV in the future.
>
> Since we do have a page_pool_put_full_page(), adding a variant for
> the nosync seems reasonable.
>
> Suggested-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
> Acked-by: Furong Xu <0x1207@xxxxxxxxx>
> Signed-off-by: Guowei Dang <guowei.dang@xxxxxxxxxxx>
> ---
> Documentation/networking/page_pool.rst | 5 ++++-
> include/net/page_pool/helpers.h | 17 +++++++++++++++++
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/networking/page_pool.rst b/Documentation/networking/page_pool.rst
> index 9d958128a57c..a83f7c071132 100644
> --- a/Documentation/networking/page_pool.rst
> +++ b/Documentation/networking/page_pool.rst
> @@ -62,7 +62,8 @@ a page will cause no race conditions is enough.
> :identifiers: struct page_pool_params
>
> .. kernel-doc:: include/net/page_pool/helpers.h
> - :identifiers: page_pool_put_page page_pool_put_full_page
> + :identifiers: page_pool_put_page
> + page_pool_put_page_nosync page_pool_put_full_page
> page_pool_recycle_direct page_pool_free_va
> page_pool_dev_alloc_pages page_pool_dev_alloc_frag
> page_pool_dev_alloc page_pool_dev_alloc_va
> @@ -93,6 +94,8 @@ much of the page needs to be synced (starting at ``offset``).
> When directly freeing pages in the driver (page_pool_put_page())
> the ``dma_sync_size`` argument specifies how much of the buffer needs
> to be synced.
> +If the ``dma_sync_size`` argument is 0, page_pool_put_page_nosync() should be
> +used instead of page_pool_put_page().
It would be good to describe when user should call page_pool_put_page_nosync()
and when user should call page_pool_put_page() as the above doesn't really
help user to decide using which API.
As I recall correctly, it seems there is some use case that user is able to
tell that it is ok the skip the dma sync to improve performance by calling
page_pool_put_page_nosync() even when the page_pool is created with
PP_FLAG_DMA_SYNC_DEV flags set.
>
> If in doubt set ``offset`` to 0, ``max_len`` to ``PAGE_SIZE`` and
> pass -1 as ``dma_sync_size``. That combination of arguments is always
> diff --git a/include/net/page_pool/helpers.h b/include/net/page_pool/helpers.h
> index e555921e5233..5cc68d48624a 100644
> --- a/include/net/page_pool/helpers.h
> +++ b/include/net/page_pool/helpers.h
> @@ -340,12 +340,14 @@ static inline void page_pool_put_netmem(struct page_pool *pool,
> * the allocator owns the page and will try to recycle it in one of the pool
> * caches. If PP_FLAG_DMA_SYNC_DEV is set, the page will be synced for_device
> * using dma_sync_single_range_for_device().
> + * page_pool_put_page_nosync() should be used if dma_sync_size is 0.
> */
> static inline void page_pool_put_page(struct page_pool *pool,
> struct page *page,
> unsigned int dma_sync_size,
> bool allow_direct)
> {
> + DEBUG_NET_WARN_ON_ONCE(!dma_sync_size);
> page_pool_put_netmem(pool, page_to_netmem(page), dma_sync_size,
> allow_direct);
> }
> @@ -372,6 +374,21 @@ static inline void page_pool_put_full_page(struct page_pool *pool,
> page_pool_put_netmem(pool, page_to_netmem(page), -1, allow_direct);
> }
>
> +/**
> + * page_pool_put_page_nosync() - release a reference on a page pool page
> + * @pool: pool from which page was allocated
> + * @page: page to release a reference on
> + * @allow_direct: released by the consumer, allow lockless caching
> + *
> + * Similar to page_pool_put_page(), but will not DMA sync the memory area.
> + */
> +static inline void page_pool_put_page_nosync(struct page_pool *pool,
> + struct page *page,
> + bool allow_direct)
> +{
> + page_pool_put_netmem(pool, page_to_netmem(page), 0, allow_direct);
> +}
> +
> /**
> * page_pool_recycle_direct() - release a reference on a page pool page
> * @pool: pool from which page was allocated
