Message-ID: <65970003ee9ef_8dc6829412@dwillia2-xfh.jf.intel.com.notmuch>
Date: Thu, 4 Jan 2024 10:59:16 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Bjorn Helgaas <helgaas@...nel.org>, Dan Williams
	<dan.j.williams@...el.com>
CC: Bjorn Helgaas <bhelgaas@...gle.com>, Ira Weiny <ira.weiny@...el.com>,
	Jonathan Cameron <jonathan.cameron@...wei.com>, Smita Koralahalli
	<Smita.KoralahalliChannabasappa@....com>, Shiju Jose <shiju.jose@...wei.com>,
	Yazen Ghannam <yazen.ghannam@....com>, Davidlohr Bueso <dave@...olabs.net>,
	Dave Jiang <dave.jiang@...el.com>, Alison Schofield
	<alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, "Ard
 Biesheuvel" <ardb@...nel.org>, <linux-efi@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
	<linux-pci@...r.kernel.org>
Subject: Re: [PATCH v5 8/9] PCI: Define scoped based management functions

Bjorn Helgaas wrote:
[..]
> > ---
> > 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.

I tend to agree, but it does lead to some long lines, for example:

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 4fd1f207c84e..549ba4b8294e 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type,
 				struct cxl_cper_event_rec *rec)
 {
 	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
-	struct pci_dev *pdev __free(pci_dev_put) = NULL;
 	enum cxl_event_log_type log_type;
 	struct cxl_dev_state *cxlds;
 	unsigned int devfn;
 	u32 hdr_flags;
 
 	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
-	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
-					   device_id->bus_num, devfn);
+	struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
+		device_id->segment_num, device_id->bus_num, devfn);
 	if (!pdev)
 		return;
 
...so I think people are choosing the "... __free(x) = NULL;" style for
code density readability.

> 
> > ...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.

Ok, let me ponder an update here.

> 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()?)

The __free() callbacks are invoked in reverse declaration (FILO) order.
However, as I say that another reviewer recommendation falls out. Be
careful about the variable declaration order diverging from the init
order. 

I.e. save the reader from needing to wonder if there are intra variable
init order dependencies by making it clear that init order ==
declaration order.

> 
> 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.

Sure I can trim that out and just say that the CXL driver is one such
occasion where a new goto for pci_dev_put() and pci_dev_unlock() was
about to be introduced.

> > 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@...gle.com>

Thanks, Bjorn!

Powered by blists - more mailing lists