Re: [PATCH v5 8/9] PCI: Define scoped based management functions

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jan 03, 2024 at 04:21:02PM -0800, Dan Williams wrote:
> Bjorn Helgaas wrote:
> > On Wed, Jan 03, 2024 at 02:38:57PM -0800, Dan Williams wrote:
> > > Ira Weiny wrote:
> > > > Users of pci_dev_get() can benefit from a scoped based put.  Also,
> > > > locking a PCI device is often done within a single scope.
> > > > 
> > > > Define a pci_dev_put() free function and a PCI device lock guard.  These
> > > > will initially be used in new CXL event processing code but is defined
> > > > in a separate patch for others to pickup and use/backport easier.
> > > 
> > > Any heartburn if I take this through cxl.git with the rest in this
> > > series? Patch 9 has a dependency on this one.
> > 
> > No real heartburn.  I was trying to figure out what this does
> > since I'm not familiar with the "scoped based put" idea and
> > 'git grep -i "scope.*base"' wasn't much help.
> > 
> > I would kind of like the commit log to say a little more about what
> > the "scoped based put" (does that have too many past tenses in it?)
> > means and how users of pci_dev_get() will benefit.
> 
> That is completely fair, and I should have checked to make sure that the
> changelog clarified the impact of the change.

I see "scoped based put" follows a similar use in cleanup.h, but I
think "scope-based X" reads better.

> > I don't know what "locking a PCI device is often done within a single
> > scope" is trying to tell me either.  What if it's *not* done within a
> > single scope?
> > 
> > Does this change anything for callers of pci_dev_get() and
> > pci_dev_put()?
> > 
> > Does this avoid a common programming error?  I just don't know what
> > the benefit of this is yet.
> > 
> > I'm sure this is really cool stuff, but there's little documentation,
> > few existing users, and I don't know what I need to look for when
> > reviewing things.
> 
> Here a is a re-write of the changelog:
> 
> ---
> PCI: Introduce cleanup helpers for device reference counts and locks
> 
> The "goto error" pattern is notorious for introducing subtle resource
> leaks. Use the new cleanup.h helpers for PCI device reference counts and
> locks.
> 
> Similar to the new put_device() and device_lock() cleanup helpers,
> __free(put_device) and guard(device), define the same for PCI devices,
> __free(pci_dev_put) and guard(pci_dev).  These helpers eliminate the
> need for "goto free;" and "goto unlock;" patterns. For example, A
> 'struct pci_dev *' instance declared as:
> 
> 	struct pci_dev *pdev __free(pci_dev_put) = NULL;

I see several similar __free() uses with NULL initializations in gpio,
but I think this idiom would be slightly improved if the __free()
function were more closely associated with the actual pci_dev_get():

  struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...);

Not always possible, I know, but easier to analyze when it is.

> ...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev
> goes out of scope (automatic variable scope). If a function wants to
> invoke pci_dev_put() on error, but return @pdev on success, it can do:
> 
> 	return no_free_ptr(pdev);
> 
> ...or:
> 
> 	return_ptr(pdev);
> 
> For potential cleanup opportunity there are 587 open-coded calls to
> pci_dev_put() in the kernel with 65 instances within 10 lines of a goto
> statement with the CXL driver threatening to add another one.
> 
> The guard() helper holds the associated lock for the remainder of the
> current scope in which it was invoked. So, for example:
> 
> 	func(...)
> 	{
> 		if (...) {
> 			...
> 			guard(pci_dev); /* pci_dev_lock() invoked here */
> 			...
> 		} /* <- implied pci_dev_unlock() triggered here */
> 	}

Thanks for this!  I had skimmed cleanup.h previously, but it makes a
lot more sense after your description here.  

I think a little introduction along these lines would be even more
useful in cleanup.h since the concept is general and not PCI-specific.
E.g., the motivation (avoid resource leaks with "goto error" pattern),
a definition of "__free() based cleanup function" (IIUC, a function to
be run when a variable goes out of scope), maybe something about
ordering (it's important in the "goto error" pattern that the cleanups
are done in a specific order; how does that translate to __free()?)

But the commit log above is fine with me.  (It does contain tabs,
which get slightly mangled when "git log" indents it.)

> There are 15 invocations of pci_dev_unlock() in the kernel with 5
> instances within 10 lines of a goto statement. Again, the CXL driver is
> threatening to add another.
> 
> Introduce these helpers to preclude the addition of new more error prone
> goto put; / goto unlock; sequences. For now, these helpers are used in
> drivers/cxl/pci.c to allow ACPI error reports to be fed back into the
> CXL driver associated with the PCI device identified in the report.

This part is also fine but doesn't seem strictly necessary to me.  I
think the part about error reports being fed back needs a lot more
background to understand the connection, and probably only makes sense
in the context of that patch.

> As for reviewing conversions that use these new helpers, one of the
> gotchas I have found is that it is easy to make a mistake if a goto
> still exists in the function after the conversion. So unless and until
> all of the resources a function acquires, that currently need a goto to
> unwind them on error, can be converted to cleanup.h based helpers it is
> best not to mix styles.
> 
> I think the function documentation in include/linux/cleanup.h does a
> decent job of explaining how to use the helpers. However, I am happy to
> suggest some updates if you think it would help.

Thanks, Dan!

Acked-by: Bjorn Helgaas <bhelgaas@google.com>