Re: [PATCH net] docs: net: page_pool: document PP_FLAG_DMA_SYNC_DEV parameters

[Randy Dunlap <rdunlap@infradead.org>]

A few nits:

On 8/1/23 13:31, Jakub Kicinski wrote:
> Using PP_FLAG_DMA_SYNC_DEV is a bit confusing. It was perhaps
> more obvious when it was introduced but the page pool use
> has grown beyond XDP and beyond packet-per-page so now
> making the heads and tails out of this feature is not
> trivial.
> 
> Obviously making the API more user friendly would be
> a better fix, but until someone steps up to do that
> let's at least document what the parameters are.
> 
> Relevant discussion in the first Link.
> 
> Link: https://lore.kernel.org/all/20230731114427.0da1f73b@kernel.org/
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: hawk@kernel.org
> CC: ilias.apalodimas@linaro.org
> CC: corbet@lwn.net
> CC: linux-doc@vger.kernel.org
> CC: Michael Chan <michael.chan@broadcom.com>
> CC: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  Documentation/networking/page_pool.rst | 34 ++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/Documentation/networking/page_pool.rst b/Documentation/networking/page_pool.rst
> index 0aa850cf4447..7064813b3b58 100644
> --- a/Documentation/networking/page_pool.rst
> +++ b/Documentation/networking/page_pool.rst
> @@ -109,6 +109,40 @@ a page will cause no race conditions is enough.
>    caller can then report those stats to the user (perhaps via ethtool,
>    debugfs, etc.). See below for an example usage of this API.
>  
> +DMA sync
> +--------
> +Driver is always responsible for sync'ing the pages for the CPU.

                                    syncing [as on the next line]

> +Drivers may choose to take care of syncing for the device as well

  or                                  sync'ing
since you use "sync'ed" 2 lines below.

> +or set the ``PP_FLAG_DMA_SYNC_DEV`` flag to request that pages
> +allocated from the page pool are already sync'ed for the device.
> +
> +If ``PP_FLAG_DMA_SYNC_DEV`` is set, the driver must inform the core what portion
> +of the buffer has to be synced. This allows the core to avoid syncing the entire

  or                       sync'ed.
Just be consistent.

> +page when the drivers knows that the device only accessed a portion of the page.
> +
> +Most drivers will reserve a headroom in front of the frame,

                     reserve headroom in front of the frame.
or                   reserve some headroom in front of the frame.

> +this part of the buffer is not touched by the device, so to avoid syncing

   This                                                              ^^^ [be consistent]

> +it drivers can set the ``offset`` field in struct page_pool_params
> +appropriately.
> +
> +For pages recycled on the XDP xmit and skb paths the page pool will
> +use the ``max_len`` member of struct page_pool_params to decide how
> +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 in doubt set ``offset`` to 0, ``max_len`` to ``PAGE_SIZE`` and
> +pass -1 as ``dma_sync_size``. That combination of arguments is always
> +correct.

   at the expense of more overhead?

> +
> +Note that the sync'ing parameters are for the entire page.
> +This is important to remember when using fragments (``PP_FLAG_PAGE_FRAG``),
> +where allocated buffers may be smaller than a full page.
> +Unless the driver author really understands page pool internals
> +it's recommended to always use ``offset = 0``, ``max_len = PAGE_SIZE``
> +with fragmented page pools.
> +
>  Stats API and structures
>  ------------------------
>  If the kernel is configured with ``CONFIG_PAGE_POOL_STATS=y``, the API

-- 
~Randy