Re: [RFC net-next 03/15] net: Add memory provider capabilities

Stanislav Fomichev [off-list ref] writes:

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

quoted

The current memory providers, devmem and io_uring zero-copy RX, give
memory that the CPU cannot read. The core allows them only on queues
with header-data split on, a zero header-split threshold, and no XDP
program. AF_XDP needs a provider that the CPU can read and that
works with XDP.

Add a capability field to the provider operations, with
MP_CAP_READABLE as the first flag. Apply the header-split and XDP
rules only to providers without it. A queue with an AF_XDP buffer
pool still rejects other providers, but it accepts the provider made
from that same pool.

Provider memory can become free while a queue is restarted, and no
device event reports it. After a restart, and after a failed restart
is rolled back, schedule the queue's NAPI if the queue has a
provider, so the driver tries to refill.

Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 Documentation/networking/netmem.rst     |  7 ++-
 include/net/page_pool/memory_provider.h | 11 +++++
 include/net/page_pool/types.h           |  7 +--
 net/core/dev.c                          |  4 +-
 net/core/netdev_rx_queue.c              | 57 ++++++++++++++++++++-----
 net/ethtool/rings.c                     | 18 +++-----
 net/xdp/xsk_buff_pool.c                 |  3 +-
 7 files changed, 78 insertions(+), 29 deletions(-)

diff --git a/Documentation/networking/netmem.rst b/Documentation/networking/netmem.rst
index 217869d1108d..aacd185843ac 100644
--- a/Documentation/networking/netmem.rst
+++ b/Documentation/networking/netmem.rst

@@ -48,8 +48,11 @@ Driver RX Requirements
    - PP_FLAG_DMA_SYNC_DEV: netmem dma addr is not necessarily dma-syncable
      by the driver. The driver must delegate the dma syncing to the page_pool,
      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_ALLOW_UNREADABLE_NETMEM. This opts the page pool into the memory
+     provider configured on its RX queue. For a provider without
+     MP_CAP_READABLE, the driver must specify it only for a payload pool
+     with tcp-data-split enabled. A readable provider may also back a
+     regular or header pool.
 
 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

diff --git a/include/net/page_pool/memory_provider.h b/include/net/page_pool/memory_provider.h
index 255ce4cfd975..d18ec079ffe2 100644
--- a/include/net/page_pool/memory_provider.h
+++ b/include/net/page_pool/memory_provider.h

@@ -9,6 +9,15 @@ struct netdev_rx_queue;
 struct netlink_ext_ack;
 struct sk_buff;
 
+/**
+ * enum mp_caps - memory provider capabilities
+ * @MP_CAP_READABLE: The CPU can access provider buffers. They may back
+ *	header and regular page pools, and XDP programs may run on them.
+ */
+enum mp_caps {
+	MP_CAP_READABLE		= BIT(0),
+};
+
 struct memory_provider_ops {
 	netmem_ref (*alloc_netmems)(struct page_pool *pool, gfp_t gfp);
 	bool (*release_netmem)(struct page_pool *pool, netmem_ref netmem);

@@ -17,11 +26,13 @@ struct memory_provider_ops {
 	int (*nl_fill)(void *mp_priv, struct sk_buff *rsp,
 		       struct netdev_rx_queue *rxq);
 	void (*uninstall)(void *mp_priv, struct netdev_rx_queue *rxq);
+	u32 caps;
 };
 
 bool net_mp_niov_set_dma_addr(struct net_iov *niov, dma_addr_t addr);
 void net_mp_niov_set_page_pool(struct page_pool *pool, struct net_iov *niov);
 void net_mp_niov_clear_page_pool(struct net_iov *niov);
+bool netif_mp_lacks_cap(struct net_device *dev, u32 cap);
 
 int netif_mp_open_rxq(struct net_device *dev, unsigned int rxq_idx,
 		      const struct pp_memory_provider_params *p,

diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index 03da138722f5..a96376613dda 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h

@@ -22,9 +22,10 @@
 					*/
 #define PP_FLAG_SYSTEM_POOL	BIT(2) /* Global system page_pool */
 
-/* Allow unreadable (net_iov backed) netmem in this page_pool. Drivers setting
- * this must be able to support unreadable netmem, where netmem_address() would
- * return NULL. This flag should not be set for header page_pools.
+/* Allow memory-provider (net_iov backed) netmem in this page_pool. Drivers
+ * setting this must honor the provider's capabilities. Unless the provider has
+ * MP_CAP_READABLE, netmem_address() returns NULL, and the flag must not be set
+ * for a header page_pool.
  *
  * If the driver sets PP_FLAG_ALLOW_UNREADABLE_NETMEM, it should also set
  * page_pool_params.slow.queue_idx.

diff --git a/net/core/dev.c b/net/core/dev.c
index 5ac08da8b9d7..12b532c09a9c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -10423,7 +10423,7 @@ int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
 		return -EBUSY;
 	}
 
-	if (dev_get_min_mp_channel_count(dev)) {
+	if (netif_mp_lacks_cap(dev, MP_CAP_READABLE)) {
 		NL_SET_ERR_MSG(bpf->extack, "unable to propagate XDP to device using memory provider");
 		return -EBUSY;
 	}

@@ -10499,7 +10499,7 @@ static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
 		return -EBUSY;
 	}
 
-	if (dev_get_min_mp_channel_count(dev)) {
+	if (netif_mp_lacks_cap(dev, MP_CAP_READABLE)) {
 		NL_SET_ERR_MSG(extack, "unable to install XDP to device using memory provider");
 		return -EBUSY;
 	}

diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
index 00a7011eb4d5..610e31d1a717 100644
--- a/net/core/netdev_rx_queue.c
+++ b/net/core/netdev_rx_queue.c

@@ -82,8 +82,12 @@ __netif_get_rx_queue_lease(struct net_device **dev, unsigned int *rxq_idx,
 /* See also page_pool_is_unreadable() */
 bool netif_rxq_has_unreadable_mp(struct net_device *dev, unsigned int rxq_idx)
 {
-	if (rxq_idx < dev->real_num_rx_queues)
-		return __netif_get_rx_queue(dev, rxq_idx)->mp_params.mp_ops;
+	const struct memory_provider_ops *ops;
+
+	if (rxq_idx < dev->real_num_rx_queues) {
+		ops = __netif_get_rx_queue(dev, rxq_idx)->mp_params.mp_ops;
+		return ops && !(ops->caps & MP_CAP_READABLE);
+	}
 	return false;
 }
 EXPORT_SYMBOL(netif_rxq_has_unreadable_mp);

@@ -95,6 +99,32 @@ bool netif_rxq_has_mp(struct net_device *dev, unsigned int rxq_idx)
 	return false;
 }
 
+/* Return true if an installed memory provider lacks @cap. */
+bool netif_mp_lacks_cap(struct net_device *dev, u32 cap)
+{
+	const struct memory_provider_ops *ops;
+	int i;
+
+	netdev_assert_locked_ops_compat(dev);
+
+	for (i = dev->real_num_rx_queues - 1; i >= 0; i--) {
+		ops = dev->_rx[i].mp_params.mp_ops;
+		if (ops && (ops->caps & cap) != cap)
+			return true;
+	}
+
+	return false;
+}
+
+/* Provider memory may become available while the queue is replaced,
+ * without a device event to report it.
+ */
+static void netdev_rx_queue_kick(struct netdev_rx_queue *rxq)
+{
+	if (rxq->mp_params.mp_ops && rxq->napi)
+		napi_schedule(rxq->napi);
+}
+
 static int netdev_rx_queue_reconfig(struct net_device *dev,
 				    unsigned int rxq_idx,
 				    struct netdev_queue_config *qcfg_old,

[..]

quoted

@@ -142,6 +172,8 @@ static int netdev_rx_queue_reconfig(struct net_device *dev,
 	}
 
 	qops->ndo_queue_mem_free(dev, old_mem);
+	if (netif_running(dev))
+		netdev_rx_queue_kick(rxq);
 
 	kvfree(old_mem);
 	kvfree(new_mem);

@@ -165,6 +197,11 @@ static int netdev_rx_queue_reconfig(struct net_device *dev,
 
 err_free_new_queue_mem:
 	qops->ndo_queue_mem_free(dev, new_mem);
+	/* Freeing the replacement can hand provider memory back to the old
+	 * queue, which still runs unless the rollback failed.
+	 */
+	if (err != -ENETDOWN && netif_running(dev))
+		netdev_rx_queue_kick(rxq);
 
 err_free_old_mem:
 	kvfree(old_mem);

These are separate fixes?

Thanks for taking a look!

The old pool can hold buffers the new pool needs. We need another refill
after those buffers return. I'll split that from the capabilities patch.


Björn