Re: [RFC net-next 05/15] page_pool: Extend memory provider operations

[Stanislav Fomichev]

On 10/02, Björn Töpel wrote:
> The current memory providers give memory that the CPU cannot read,
> and they can always refill. An AF_XDP provider is different. The CPU
> can read its memory, it has only the buffers that userspace puts in
> the FILL ring, and its buffers go back to userspace instead of being
> freed. It needs four things from page_pool:
> 
> - A CPU address for each net_iov. A provider can set the address of
>   the first net_iov in its net_iov_area and the size each net_iov
>   covers. netmem_address() then computes the address without a call
>   into the provider; drivers call it for every packet. A
>   net_iov_area without an address stays unreadable.
>   page_pool_is_unreadable() is false for a readable provider.
> 
> - A way to say that the refill is not done. The new refill_done
>   callback tells the driver whether it may stop refilling. A
>   provider that still has buffers returns false, and the driver
>   keeps NAPI scheduled.
> 
> - A way to give up a buffer without freeing it. Add a batched call
>   that ends page_pool ownership of buffers that leave through the
>   provider, for example to userspace. It clears the page_pool link,
>   so a later page pool can take the buffer. Add the matching batched
>   call that sets the link, and a DMA sync helper for drivers that
>   post a buffer again directly.
> 
> - No buffer splitting, because a split buffer has several owners.
>   Add MP_CAP_FRAG. page_pool refuses fragment allocation from a
>   provider without it.
> 
> devmem and io_uring set MP_CAP_FRAG and keep their fragment
> behaviour. Their refill paths now use the batched call that sets the
> link.
> 
> Signed-off-by: Björn Töpel <bjorn@xxxxxxxxxx>
> ---
>  include/net/netmem.h                    | 31 ++++++++++-
>  include/net/page_pool/helpers.h         | 62 +++++++++++++++++++++-
>  include/net/page_pool/memory_provider.h | 28 +++++-----
>  io_uring/zcrx.c                         |  4 +-
>  net/core/devmem.c                       |  7 ++-
>  net/core/page_pool.c                    | 68 ++++++++++++++++++++-----
>  6 files changed, 165 insertions(+), 35 deletions(-)
> 
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index cc97611632dc..e6dff0b01581 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -105,6 +105,12 @@ struct net_iov_area {
>  

[..]

>  	/* Offset into the dma-buf where this chunk starts.  */
>  	unsigned long base_virtual;

Will send a patch to drop this one, don't think we use it anymore..

[..]

> +	/* CPU address of the first net_iov's memory, or NULL when the CPU
> +	 * cannot read the area. Each net_iov covers 1 << @niov_shift bytes.
> +	 */
> +	void *vaddr;

Who is setting this? Somewhere in another patch? Can we keep it here?

> +	u8 niov_shift;

If we need niov_shift at area level, let's move the one from binding here?
Then the existing callers can do binding->area.niov_shift.

>  };
>  
>  static inline struct net_iov_area *net_iov_owner(const struct net_iov *niov)
> @@ -117,6 +123,23 @@ static inline unsigned int net_iov_idx(const struct net_iov *niov)
>  	return niov - net_iov_owner(niov)->niovs;
>  }
>  
> +static inline bool net_iov_is_readable(const struct net_iov *niov)
> +{
> +	return net_iov_owner(niov)->vaddr;
> +}
> +
> +static inline void *net_iov_address(const struct net_iov *niov)
> +{
> +	const struct net_iov_area *area = net_iov_owner(niov);
> +	unsigned long off;
> +
> +	if (!area->vaddr)
> +		return NULL;
> +
> +	off = (unsigned long)net_iov_idx(niov) << area->niov_shift;
> +	return area->vaddr + off;

Or, alternatively, store dma_base in area and do:

return niov->dma_addr - area.dma_base + vadd ?