Re: [PATCH v7 03/15] net: page_pool: create hooks for custom page providers

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: David Wei <dw@davidwei.uk>
Date: Tue, 29 Oct 2024 16:05:06 -0700

> From: Jakub Kicinski <kuba@kernel.org>
> 
> The page providers which try to reuse the same pages will
> need to hold onto the ref, even if page gets released from
> the pool - as in releasing the page from the pp just transfers
> the "ownership" reference from pp to the provider, and provider
> will wait for other references to be gone before feeding this
> page back into the pool.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> [Pavel] Rebased, renamed callback, +converted devmem
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
>  include/net/page_pool/types.h |  9 +++++++++
>  net/core/devmem.c             | 14 +++++++++++++-
>  net/core/page_pool.c          | 17 +++++++++--------
>  3 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
> index c022c410abe3..8a35fe474adb 100644
> --- a/include/net/page_pool/types.h
> +++ b/include/net/page_pool/types.h
> @@ -152,8 +152,16 @@ struct page_pool_stats {
>   */
>  #define PAGE_POOL_FRAG_GROUP_ALIGN	(4 * sizeof(long))
>  
> +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);
> +	int (*init)(struct page_pool *pool);
> +	void (*destroy)(struct page_pool *pool);
> +};
> +
>  struct pp_memory_provider_params {
>  	void *mp_priv;
> +	const struct memory_provider_ops *mp_ops;
>  };
>  
>  struct page_pool {
> @@ -215,6 +223,7 @@ struct page_pool {
>  	struct ptr_ring ring;
>  
>  	void *mp_priv;
> +	const struct memory_provider_ops *mp_ops;
>  
>  #ifdef CONFIG_PAGE_POOL_STATS
>  	/* recycle stats are per-cpu to avoid locking */
> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 5c10cf0e2a18..01738029e35c 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -26,6 +26,8 @@
>  /* Protected by rtnl_lock() */
>  static DEFINE_XARRAY_FLAGS(net_devmem_dmabuf_bindings, XA_FLAGS_ALLOC1);
>  
> +static const struct memory_provider_ops dmabuf_devmem_ops;
> +
>  static void net_devmem_dmabuf_free_chunk_owner(struct gen_pool *genpool,
>  					       struct gen_pool_chunk *chunk,
>  					       void *not_used)
> @@ -117,6 +119,7 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
>  		WARN_ON(rxq->mp_params.mp_priv != binding);
>  
>  		rxq->mp_params.mp_priv = NULL;
> +		rxq->mp_params.mp_ops = NULL;
>  
>  		rxq_idx = get_netdev_rx_queue_index(rxq);
>  
> @@ -142,7 +145,7 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
>  	}
>  
>  	rxq = __netif_get_rx_queue(dev, rxq_idx);
> -	if (rxq->mp_params.mp_priv) {
> +	if (rxq->mp_params.mp_ops) {
>  		NL_SET_ERR_MSG(extack, "designated queue already memory provider bound");
>  		return -EEXIST;
>  	}
> @@ -160,6 +163,7 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
>  		return err;
>  
>  	rxq->mp_params.mp_priv = binding;
> +	rxq->mp_params.mp_ops = &dmabuf_devmem_ops;
>  
>  	err = netdev_rx_queue_restart(dev, rxq_idx);
>  	if (err)
> @@ -169,6 +173,7 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
>  
>  err_xa_erase:
>  	rxq->mp_params.mp_priv = NULL;
> +	rxq->mp_params.mp_ops = NULL;
>  	xa_erase(&binding->bound_rxqs, xa_idx);
>  
>  	return err;
> @@ -388,3 +393,10 @@ bool mp_dmabuf_devmem_release_page(struct page_pool *pool, netmem_ref netmem)
>  	/* We don't want the page pool put_page()ing our net_iovs. */
>  	return false;
>  }
> +
> +static const struct memory_provider_ops dmabuf_devmem_ops = {
> +	.init			= mp_dmabuf_devmem_init,
> +	.destroy		= mp_dmabuf_devmem_destroy,
> +	.alloc_netmems		= mp_dmabuf_devmem_alloc_netmems,
> +	.release_netmem		= mp_dmabuf_devmem_release_page,
> +};
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index a813d30d2135..c21c5b9edc68 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -284,10 +284,11 @@ static int page_pool_init(struct page_pool *pool,
>  		rxq = __netif_get_rx_queue(pool->slow.netdev,
>  					   pool->slow.queue_idx);
>  		pool->mp_priv = rxq->mp_params.mp_priv;
> +		pool->mp_ops = rxq->mp_params.mp_ops;
>  	}
>  
> -	if (pool->mp_priv) {
> -		err = mp_dmabuf_devmem_init(pool);
> +	if (pool->mp_ops) {
> +		err = pool->mp_ops->init(pool);

Can't we just switch-case instead of indirect calls?
IO_URING is bool, it can't be a module, which means its functions will
be available here when it's enabled. These ops are easy to predict (no
ops, dmabuf, io_uring), so this really looks like an enum with 3 entries
+ switch-case ("regular" path is out if this switch-case under likely etc).

>  		if (err) {
>  			pr_warn("%s() mem-provider init failed %d\n", __func__,
>  				err);
> @@ -584,8 +585,8 @@ netmem_ref page_pool_alloc_netmem(struct page_pool *pool, gfp_t gfp)
>  		return netmem;

Thanks,
Olek