Re: [PATCH net-next v6 2/2] page_pool: Track DMA-mapped pages and unmap them when destroying the pool

[Pavel Begunkov <asml.silence@gmail.com>]

On 4/1/25 10:27, Toke Høiland-Jørgensen wrote:
...
> Reported-by: Yonglong Liu <liuyonglong@huawei.com>
> Closes: https://lore.kernel.org/r/8743264a-9700-4227-a556-5f931c720211@huawei.com
> Fixes: ff7d6b27f894 ("page_pool: refurbish version of page_pool code")
> Suggested-by: Mina Almasry <almasrymina@google.com>
> Reviewed-by: Mina Almasry <almasrymina@google.com>
> Reviewed-by: Jesper Dangaard Brouer <hawk@kernel.org>
> Tested-by: Jesper Dangaard Brouer <hawk@kernel.org>
> Tested-by: Qiuling Ren <qren@redhat.com>
> Tested-by: Yuying Ma <yuma@redhat.com>
> Tested-by: Yonglong Liu <liuyonglong@huawei.com>
> Acked-by: Jesper Dangaard Brouer <hawk@kernel.org>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

I haven't looked into the bit carving, but the rest looks
good to me. A few nits below,

...
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 7745ad924ae2d801580a6760eba9393e1cf67b01..52b5ddab7ecb405066fd55b8d61abfd4186b9dcf 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -227,6 +227,8 @@ static int page_pool_init(struct page_pool *pool,
>   			return -EINVAL;
>   
>   		pool->dma_map = true;
> +
> +		xa_init_flags(&pool->dma_mapped, XA_FLAGS_ALLOC1);

nit: might be better to init/destroy unconditionally, it doesn't
allocate any memory.

>   	}
>   
>   	if (pool->slow.flags & PP_FLAG_DMA_SYNC_DEV) {
> @@ -276,9 +278,6 @@ static int page_pool_init(struct page_pool *pool,
>   	/* Driver calling page_pool_create() also call page_pool_destroy() */
>   	refcount_set(&pool->user_cnt, 1);
>   
> -	if (pool->dma_map)
> -		get_device(pool->p.dev);
> -
>   	if (pool->slow.flags & PP_FLAG_ALLOW_UNREADABLE_NETMEM) {
>   		netdev_assert_locked(pool->slow.netdev);
>   		rxq = __netif_get_rx_queue(pool->slow.netdev,
> @@ -322,7 +321,7 @@ static void page_pool_uninit(struct page_pool *pool)
>   	ptr_ring_cleanup(&pool->ring, NULL);
>   
>   	if (pool->dma_map)
> -		put_device(pool->p.dev);
> +		xa_destroy(&pool->dma_mapped);
>   
>   #ifdef CONFIG_PAGE_POOL_STATS
>   	if (!pool->system)
> @@ -463,13 +462,21 @@ page_pool_dma_sync_for_device(const struct page_pool *pool,
>   			      netmem_ref netmem,
>   			      u32 dma_sync_size)
>   {
> -	if (pool->dma_sync && dma_dev_need_sync(pool->p.dev))
> -		__page_pool_dma_sync_for_device(pool, netmem, dma_sync_size);
> +	if (READ_ONCE(pool->dma_sync) && dma_dev_need_sync(pool->p.dev)) {
> +		rcu_read_lock();
> +		/* re-check under rcu_read_lock() to sync with page_pool_scrub() */
> +		if (READ_ONCE(pool->dma_sync))
> +			__page_pool_dma_sync_for_device(pool, netmem,
> +							dma_sync_size);
> +		rcu_read_unlock();
> +	}
>   }
>   
> -static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem)
> +static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem, gfp_t gfp)
>   {
>   	dma_addr_t dma;
> +	int err;
> +	u32 id;
>   
>   	/* Setup DMA mapping: use 'struct page' area for storing DMA-addr
>   	 * since dma_addr_t can be either 32 or 64 bits and does not always fit
> @@ -483,15 +490,28 @@ static bool page_pool_dma_map(struct page_pool *pool, netmem_ref netmem)
>   	if (dma_mapping_error(pool->p.dev, dma))
>   		return false;
>   
> -	if (page_pool_set_dma_addr_netmem(netmem, dma))
> +	if (in_softirq())
> +		err = xa_alloc(&pool->dma_mapped, &id, netmem_to_page(netmem),
> +			       PP_DMA_INDEX_LIMIT, gfp);
> +	else
> +		err = xa_alloc_bh(&pool->dma_mapped, &id, netmem_to_page(netmem),
> +				  PP_DMA_INDEX_LIMIT, gfp);

Is it an optimisation? bh disable should be reentrable and could
just be xa_alloc_bh(). KERN_{NOTICE,INFO} Maybe?


> +	if (err) {
> +		WARN_ONCE(1, "couldn't track DMA mapping, please report to netdev@");

That can happen with enough memory pressure, I don't think
it should be a warning. Maybe some pr_info?

>   		goto unmap_failed;
> +	}
>   
> +	if (page_pool_set_dma_addr_netmem(netmem, dma)) {
> +		WARN_ONCE(1, "unexpected DMA address, please report to netdev@");
> +		goto unmap_failed;
> +	}
> +
> +	netmem_set_dma_index(netmem, id);
>   	page_pool_dma_sync_for_device(pool, netmem, pool->p.max_len);
>   
>   	return true;


-- 
Pavel Begunkov