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@xxxxxxxxxx/ > Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx> > --- > CC: hawk@xxxxxxxxxx > CC: ilias.apalodimas@xxxxxxxxxx > CC: corbet@xxxxxxx > CC: linux-doc@xxxxxxxxxxxxxxx > CC: Michael Chan <michael.chan@xxxxxxxxxxxx> > CC: Lorenzo Bianconi <lorenzo@xxxxxxxxxx> > --- > 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
