Re: I

[Leon Romanovsky]

On Mon, Aug 24, 2026 at 02:29:34PM -0600, Logan Gunthorpe wrote:
> 
> 
> On 2026-08-21 13:38, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxx>
> > 
> > pdev->p2pdma has two lifetime models. Provider-based entry points are
> > quiesced by their driver before remove completes. pci_p2pmem_find_many()
> > and the p2pmem sysfs attributes can race with unbind and therefore rely
> > on the teardown grace period.
> > 
> > Document publication, teardown, and how the grace period protects both
> > the struct pci_p2pdma object and its optional gen_pool.
> > 
> > Tested-by: Tushar Dave <tdave@xxxxxxxxxx>
> > Cc: Alex Williamson <alex@xxxxxxxxxxx>
> > Cc: Matt Evans <matt@xxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>
> > ---
> >  drivers/pci/p2pdma.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 53 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> > index a77ef9deb3c6..49bc8cf06240 100644
> > --- a/drivers/pci/p2pdma.c
> > +++ b/drivers/pci/p2pdma.c
> > @@ -21,6 +21,39 @@
> >  #include <linux/seq_buf.h>
> >  #include <linux/xarray.h>
> >  
> > +/*
> > + * Lifetime and RCU usage
> > + *
> > + * Within one driver bind, pdev->p2pdma is published exactly once, by
> 
> Is published the right verb here? Seems like we use published for
> different purposes in p2pdma and calling pcim_p2pdma_init() publishing
> reads strangely.

It should probably say "pdev->p2pdma is set exactly once".

> 
> > + * pcim_p2pdma_init(), and cleared exactly once, by the pci_p2pdma_release()
> > + * devres action that the same function installs. It is never re-pointed at a
> 
> "It is never re-pointed at" is some strange wording. I had to read it a
> few times to understand what it is saying.

Sorry about that. AI + non-english speaker = "re-pointed".

> 
> > + * second struct pci_p2pdma, so a reader that observes a non-NULL pointer
> > + * always observes the same, fully initialised object. That object is devres
> > + * memory allocated before the action is installed, so devres frees it only
> > + * after pci_p2pdma_release() has returned.
> 
> I don't quite follow the point of this paragraph. It's like it's
> building to some kind of gotcha, but all it seems to be saying is is the
> life cycle of pdev->p2pdma is the same as the lifecycle of pdev.

Yes

> 
> > + * Most exported entry points reach pdev->p2pdma through a struct pci_dev or a
> > + * struct p2pdma_provider owned by the provider driver, and
> > + * pcim_p2pdma_provider() requires callers to drop those references before the
> > + * driver's remove() completes. Those cannot run concurrently with
> > + * pci_p2pdma_release(), and their rcu_dereference() calls are simply how an
> > + * __rcu pointer is read.
> > + *
> > + * pci_p2pmem_find_many() and the p2pmem sysfs attributes are the exceptions.
> > + * The first walks every PCI device, so it can reach a provider whose driver is
> > + * unbinding: pci_get_device() pins the struct pci_dev, not the driver. The
> > + * second is reachable from userspace until sysfs_remove_group() runs at the end
> > + * of the release. pci_has_p2pmem() must dereference the object to determine
> > + * whether it owns a gen_pool, so even a poolless object must remain alive until
> 
> Maybe a hyphen with pool-less or maybe better to use plain language: "so
> even a device without a pool must remain alive..."

I will try to simplify the language.

> 
> > + * that RCU reader exits. The sysfs group is created with the pool.
> > + *
> > + * The grace period in pci_p2pdma_release() first protects the struct
> > + * pci_p2pdma itself from being freed while pci_has_p2pmem() is using it. For a
> > + * pool-backed provider it also fences the gen_pool: gen_pool_alloc_owner()
> > + * walks pool->chunks under RCU and gen_pool_destroy() frees those chunks
> > + * without waiting for a grace period of its own, so pci_alloc_p2pmem() and
> > + * p2pmem_alloc_mmap() hold rcu_read_lock() across the allocation.
> > + */
> >  struct pci_p2pdma {
> >  	struct gen_pool *pool;
> >  	bool p2pmem_published;
> > @@ -235,9 +268,19 @@ static void pci_p2pdma_release(void *data)
> >  	if (!p2pdma)
> >  		return;
> >  
> > -	/* Flush and disable pci_alloc_p2p_mem() */
> > +	/*
> > +	 * Stop new RCU readers and wait for readers that observed p2pdma before
> > +	 * allowing devres to free it. This is required even without a pool,
> > +	 * because pci_has_p2pmem() dereferences every non-NULL p2pdma it finds.
> > +	 * For a pool-backed provider this also fences gen_pool_destroy().
> > +	 */
> >  	RCU_INIT_POINTER(pdev->p2pdma, NULL);
> >  	synchronize_rcu();
> > +
> > +	/*
> > +	 * The grace period also ensures no RCU reader can still be accessing
> > +	 * map_types here.
> > +	 */
> >  	xa_destroy(&p2pdma->map_types);
> >  
> >  	if (!p2pdma->pool)
> > @@ -255,6 +298,9 @@ static void pci_p2pdma_release(void *data)
> >   * for a PCI device. It allocates and sets up the necessary data
> >   * structures to support P2PDMA operations, including mapping type
> >   * tracking.
> > + *
> > + * The state is published once per driver bind and torn down by a devres
> 
> I still find the use of "published" here a bit odd and I'm not sure what
> "state" it is referring to.

published == assigned.

> 
> > + * action on unbind. Repeated calls for the same device are a no-op.
> >   */
> >  int pcim_p2pdma_init(struct pci_dev *pdev)
> >  {
> > @@ -786,6 +832,12 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
> >  		map_type = PCI_P2PDMA_MAP_NOT_SUPPORTED;
> >  	}
> >  done:
> > +	/*
> > +	 * pci_p2pmem_find_many() reaches this with a provider whose driver may
> > +	 * be unbinding, so the store runs under RCU: pci_p2pdma_release()
> > +	 * clears the pointer and waits for readers before destroying
> > +	 * map_types. See "Lifetime and RCU usage" above.
> > +	 */
> 
> I don't know, but this seems like we're just describing basic RCU usage
> here. I'm not sure I personally find much value in the comment.

It was mainly intended to help AI review P2P patches according to the
lifetime model.

> 
> >  	rcu_read_lock();
> >  	p2pdma = rcu_dereference(provider->p2pdma);
> >  	if (p2pdma)
> > 
> 
> Thanks,
> 
> Logan