Re: [PATCH net-next v1 02/11] net: Add memory provider capabilities

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Documentation/networking/netmem.rst, 'Driver RX Requirements',
  disagrees with the readable-provider semantics this patch adds.

--

net: Add memory provider capabilities

This adds MP_CAP_READABLE and MP_CAP_FRAG memory provider capabilities.
The header split and XDP restrictions now apply only to unreadable
providers. A new PP_FLAG_MEM_PROVIDER flag lets a page pool take a
readable provider from its RX queue. devmem and io_uring are marked
MP_CAP_FRAG and keep their current behavior.

> diff --git a/Documentation/networking/netmem.rst b/Documentation/networking/netmem.rst
> index 6871dcdd700cc..39cb75339704d 100644
> --- a/Documentation/networking/netmem.rst
> +++ b/Documentation/networking/netmem.rst
> @@ -103,6 +103,11 @@ Driver RX Requirements
>       which knows when dma-syncing is (or is not) appropriate.
>     - PP_FLAG_ALLOW_UNREADABLE_NETMEM. The driver must specify this flag iff
>       tcp-data-split is enabled.
> +   - PP_FLAG_MEM_PROVIDER. The page pool takes memory from the memory
> +     provider of its RX queue, readable or not. Page pool creation fails if
> +     the provider is unreadable and PP_FLAG_ALLOW_UNREADABLE_NETMEM is not
> +     set, so do not set it on header page pools of such a queue. Without it,
> +     PP_FLAG_ALLOW_UNREADABLE_NETMEM selects unreadable providers only.

[Severity: Low]
This isn't a bug, but does this entry belong under item 4's heading, "The
driver must use the following PP_FLAGS:"?

PP_FLAG_MEM_PROVIDER is opt-in. The comment in
include/net/page_pool/types.h says "Take memory from the memory provider
of the RX queue, if one is installed".

devmem and io_uring pools still get their unreadable provider with only
PP_FLAG_ALLOW_UNREADABLE_NETMEM set, because page_pool_select_mp() assigns
unreadable providers without the new flag. The entry itself also says not
to set the flag on header page pools of a queue with an unreadable
provider.

Item 2 above is still an unconditional requirement:

    2. The driver must support the tcp-data-split ethtool option.

However, __netif_mp_open_rxq() now checks hds_config and hds_thresh only
when the provider lacks MP_CAP_READABLE:

    if (unreadable &&
        dev->cfg->hds_config != ETHTOOL_TCP_DATA_SPLIT_ENABLED) {

The MP_CAP_READABLE kernel-doc also says such a provider "may back header
page pools".

Could items 2 and 4 be reworded to cover the readable provider case? As
far as I can tell, this text is still unchanged at the end of the series.
That leaves af_xdp.rst as the only place that describes the
readable-provider driver contract.

>  
>  5. The driver must not assume the netmem is readable and/or backed by pages.
>     The netmem returned by the page_pool may be unreadable, in which case

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110102039.1002605-1-bjorn%40kernel.org