From: David Wei <dw@xxxxxxxxxxx>
Date: Tue, 29 Oct 2024 16:05:06 -0700
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> [Pavel] Rebased, renamed callback, +converted devmem
> Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx>
> Signed-off-by: David Wei <dw@xxxxxxxxxxx>
> ---
> 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
