Re: [PATCH net-next v1 04/11] xsk: Allocate UMEM chunks through page_pool

[netdev-bot+sashiko]

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] The patch adds NET_IOV_XSK, a net_iov type the CPU can read:
  net_iov_address() returns area->vaddr + (idx << niov_shift), and…
- [Low] The kernel-doc of __netmem_address() (include/net/netmem.h) still
  describes header buffers as the standard case of always-page-backed…

--

xsk: Allocate UMEM chunks through page_pool

This patch adds an AF_XDP memory provider for page_pool. Each aligned
UMEM chunk becomes a CPU-readable net_iov of the new type NET_IOV_XSK,
and netmem_address() can now return an address for it. The provider
tracks which owner holds each chunk, and keeps chunks released by a page
pool on an internal reuse stack.

> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index b986ad616334b..a3b67ef32b218 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -69,6 +69,7 @@ DECLARE_STATIC_KEY_FALSE(page_pool_restricted);
>  enum net_iov_type {
>  	NET_IOV_DMABUF,
>  	NET_IOV_IOURING,
> +	NET_IOV_XSK,
>  };
>  
>  /* A memory descriptor representing abstract networking I/O vectors.

[Severity: Low]
The struct net_iov kernel-doc just below this enum still says:

 * Examples include imported dmabuf memory and imported io_uring memory. See
 * net_iov_type for all the supported types. While current net_iov types are
 * unreadable by the CPU, net_iov has no inherent restrictions and may be

NET_IOV_XSK is readable by the CPU. xsk_pp_create() sets
pp->area.vaddr = pool->addrs, so net_iov_address() returns
area->vaddr + (idx << niov_shift), and netmem_address() is no longer
NULL for every in-tree net_iov.

Documentation/networking/netmem.rst has the same problem. Its "Decouple
net_iov from unreadability" section still says that in-tree net_iov
implementations are unreadable by the CPU and that netmem_address()
returns NULL for them.

Should both texts be updated to list XSK UMEM as a readable net_iov
type? Neither has changed by the last commit of the series, "eth: fbnic:
Support AF_XDP page pool zero copy".

[ ... ]

> @@ -345,10 +362,16 @@ static inline void *__netmem_address(netmem_ref netmem)
>  	return page_address(__netmem_to_page(netmem));
>  }
>  

[Severity: Low]
The kernel-doc for __netmem_address() above still uses a header buffer
as its example of netmem that is always page-backed:

 * Unsafe version of netmem_address(). When @netmem is always page-backed,
 * e.g. when it's a header buffer, performs faster and generates smaller
 * object code (no check for the LSB). When @netmem points to IOV, provokes
 * undefined behaviour.

memory_provider.h says MP_CAP_READABLE providers "may back header page
pools". xsk_pp is the first provider that is both net_iov based and
MP_CAP_READABLE.

xsk_pp_init() does not reject header pools, and xsk_pp_alloc_netmems()
only hands out NET_IOV_XSK net_iovs. Later in the series, "xsk: Install
UMEM page pool providers in the core" requires the readable provider to
back the header pool.

A driver that follows this doc could call __netmem_address() on a header
buffer of an xsk_pp backed queue. Would it get a garbage pointer back?

No in-tree caller does this today; fbnic uses netmem_address(). Should
the "e.g. when it's a header buffer" wording be updated, though?

> +/**
> + * netmem_address - get the CPU address of the memory behind @netmem
> + * @netmem: netmem to get the address of
> + *
> + * Return: the address, or NULL if the CPU cannot read @netmem.
> + */
>  static inline void *netmem_address(netmem_ref netmem)
>  {
>  	if (netmem_is_net_iov(netmem))
> -		return NULL;
> +		return net_iov_address(netmem_to_net_iov(netmem));
>  
>  	return __netmem_address(netmem);
>  }

[ ... ]

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