Re: [PATCH v5 1/9] PCI/P2PDMA: Split pool-related cleanup out of pci_p2pdma_release()

[Alex Williamson <alex@shazbot.org>]

Hi Leon,

Please see below...

On Wed, 15 Jul 2026 18:47:24 +0100
Matt Evans <matt@ozlabs.org> wrote:

> Preparing for a refactor in a subsequent patch, split the pool-related
> release code into a new pci_p2pdma_release_pool() function.
> 
> This allows future compile-time selection of a null implementation for
> pci_p2pdma_release_pool(), when p2pdma.c is refactored into core- and
> P2P-related files.
> 
> Signed-off-by: Matt Evans <matt@ozlabs.org>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/pci/p2pdma.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index b2d5266f8653..498bca257419 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -226,6 +226,17 @@ static const struct dev_pagemap_ops p2pdma_pgmap_ops = {
>  	.folio_free = p2pdma_folio_free,
>  };
>  
> +static void pci_p2pdma_release_pool(struct pci_dev *pdev,
> +				    struct pci_p2pdma *p2pdma)
> +{
> +	if (!p2pdma->pool)
> +		return;
> +
> +	synchronize_rcu();

Sashiko notes[1] a high, preexisting issue here that looks like it was
introduced via 372d6d1b8ae3 ("PCI/P2PDMA: Refactor to separate core P2P
functionality from memory allocation").  That commit creates a two-tier
scheme where the optional memory allocation capabilities, such as the
pool, live above a core layer.  Prior to that commit, this synchronize
RCU call was unconditional.  Making it conditional on the pool suggests
it was only considered relevant to the optional layer.

However, map_types, which remains in the core layer, is RCU referenced.
Was the synchronize_rcu() call here miscategorized into the optional
tier?  Should it instead have remained unconditional?

We might need a precursor Fixes: patch to this series that makes it
unconditional in pci_p2pdma_release(), so that it remains there with
this refactor rather than becoming part of this new pool release
function.  Thanks,

Alex

[1]https://lore.kernel.org/all/20260715180521.3CDD61F00A3A@smtp.kernel.org/

> +	gen_pool_destroy(p2pdma->pool);
> +	sysfs_remove_group(&pdev->dev.kobj, &p2pmem_group);
> +}
> +
>  static void pci_p2pdma_release(void *data)
>  {
>  	struct pci_dev *pdev = data;
> @@ -237,15 +248,8 @@ static void pci_p2pdma_release(void *data)
>  
>  	/* Flush and disable pci_alloc_p2p_mem() */
>  	pdev->p2pdma = NULL;
> -	if (p2pdma->pool)
> -		synchronize_rcu();
> +	pci_p2pdma_release_pool(pdev, p2pdma);
>  	xa_destroy(&p2pdma->map_types);
> -
> -	if (!p2pdma->pool)
> -		return;
> -
> -	gen_pool_destroy(p2pdma->pool);
> -	sysfs_remove_group(&pdev->dev.kobj, &p2pmem_group);
>  }
>  
>  /**
> @@ -946,8 +950,8 @@ void *pci_alloc_p2pmem(struct pci_dev *pdev, size_t size)
>  	struct pci_p2pdma *p2pdma;
>  
>  	/*
> -	 * Pairs with synchronize_rcu() in pci_p2pdma_release() to
> -	 * ensure pdev->p2pdma is non-NULL for the duration of the
> +	 * Pairs with synchronize_rcu() in pci_p2pdma_release_pool()
> +	 * to ensure pdev->p2pdma is non-NULL for the duration of the
>  	 * read-lock.
>  	 */
>  	rcu_read_lock();