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

Stanislav Fomichev [off-list ref] writes:

On 10/02, Björn Töpel wrote:

quoted

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@kernel.org>
---
 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 {
 

[..]

quoted

 	/* 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..

Ok!

quoted

+	/* 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?

xp_pp_create() sets vaddr in patch 9. I'll move that closer.

quoted

+	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.

Good! I'll consolidate the binding's shift (and rebase on your
base_virtual cleanup)

quoted

 };
 
 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 ?

UMEM pages don't have to have contiguous DMA addresses, so I'd keep the
index-based CPU offset.


Björn