netdev - Re: [PATCH net] docs: net: page_pool: document PP_FLAG_DMA_SYNC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <23be0fd9-9177-a8bd-e436-07f52e40e79b@infradead.org>
Date: Tue, 1 Aug 2023 14:58:48 -0700
From: Randy Dunlap <rdunlap@...radead.org>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
 hawk@...nel.org, ilias.apalodimas@...aro.org, corbet@....net,
 linux-doc@...r.kernel.org, Michael Chan <michael.chan@...adcom.com>,
 Lorenzo Bianconi <lorenzo@...nel.org>
Subject: Re: [PATCH net] docs: net: page_pool: document PP_FLAG_DMA_SYNC_DEV
 parameters

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@...nel.org>
> ---
> CC: hawk@...nel.org
> CC: ilias.apalodimas@...aro.org
> CC: corbet@....net
> CC: linux-doc@...r.kernel.org
> CC: Michael Chan <michael.chan@...adcom.com>
> CC: Lorenzo Bianconi <lorenzo@...nel.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