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

On Tue, Jul 28, 2026 at 04:33:43PM -0600, Alex Williamson wrote:
> Hi Leon,
> 
> Please see below...
> 
> On Wed, 15 Jul 2026 18:47:24 +0100
> Matt Evans <[email protected]> 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 <[email protected]>
> > Acked-by: Bjorn Helgaas <[email protected]>
> > Reviewed-by: Logan Gunthorpe <[email protected]>
> > ---
> >  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?

I remember this concern when I wrote the patches and wanted to remove
RCU entirely. I revisited the Sashiko report, but reached the same
conclusion again.

The "bug" reported by Sashiko does not exist. The two-layer split
ensures that p2p is bound to the driver's lifecycle. As a result,
pdev->p2pdma is assigned and cleared only once during the lifetime of
pdev.

In this case, the RCU primitives are effectively NOPs, since nothing
will ever update that pointer.

I still needed to keep rcu_dereference() in
pci_p2pdma_map_type() to satisfy static analyzers, which would
otherwise complain about accessing an RCU-protected pointer without the
proper annotations.

RCU is used only in the sysfs flow.

Thanks

> 
> 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/[email protected]/
> 
> > +	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();
>