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

["Björn Töpel" <bjorn@kernel.org>]

Stanislav Fomichev <sdf.kernel@gmail.com> writes:

> On 10/02, Björn Töpel wrote:
>> 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,
>
> [..]
>
>> @@ -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