Message-ID: <6601c7f7369d4_2690d29490@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date: Mon, 25 Mar 2024 11:52:39 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Bjorn Helgaas <helgaas@...nel.org>, Dan Williams
	<dan.j.williams@...el.com>
CC: <peterz@...radead.org>, <torvalds@...ux-foundation.org>, Bjorn Helgaas
	<bhelgaas@...gle.com>, Ira Weiny <ira.weiny@...el.com>, Jonathan Cameron
	<jonathan.cameron@...wei.com>, Jesse Brandeburg <jesse.brandeburg@...el.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, "Lukas
 Wunner" <lukas.wunner@...el.com>, Jonathan Corbet <corbet@....net>,
	<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<gregkh@...uxfoundation.org>, <linux-doc@...r.kernel.org>
Subject: Re: [PATCH] cleanup: Add usage and style documentation

Bjorn Helgaas wrote:
> On Wed, Mar 20, 2024 at 03:04:41PM -0700, Dan Williams wrote:
> > When proposing that PCI grow some new cleanup helpers for pci_dev_put()
> > and pci_dev_{lock,unlock} [1], Bjorn had some fundamental questions
> > about expectations and best practices. Upon reviewing an updated
> > changelog with those details he recommended adding them to documentation
> > in the header file itself.
> > 
> > Add that documentation and link it into the rendering for
> > Documentation/core-api/.
> > 
> > Link: http://lore.kernel.org/r/20240104183218.GA1820872@bhelgaas [1]
> > Cc: Bjorn Helgaas <bhelgaas@...gle.com>
> > Cc: Ira Weiny <ira.weiny@...el.com>
> > Cc: Jonathan Cameron <jonathan.cameron@...wei.com>
> > Cc: Jesse Brandeburg <jesse.brandeburg@...el.com>
> > Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > Cc: Lukas Wunner <lukas.wunner@...el.com>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Jonathan Corbet <corbet@....net>
> > Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> > ---
> > Peter, Linus,
> > 
> > I am starting to see more usage of the cleanup helpers and some
> > style confusion or misunderstanding on best practices on how to use
> > them. As I mention above, Bjorn found the writeup I did for justifying
> > __free(pci_dev_put) and guard(pci_dev) useful, so here is an attempt to
> > uplevel and centralize those notes.
> 
> Thanks for doing this; I appreciate it!
> 
> > +++ b/Documentation/core-api/cleanup.rst
> > @@ -0,0 +1,8 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +===========================
> > +Scope-based Cleanup Helpers
> > +===========================
> > +
> > +.. kernel-doc:: include/linux/cleanup.h
> > +   :doc: scope-based cleanup helpers
> 
> Neat, I didn't know about this way of referencing doc in the source
> file, although I see the markup isn't universally loved in source.
> Either in cleanup.h or under Documentation/ is fine with me; grep will
> find it either place.

While grep will find it there are benefits to keeping the documentation
closer to the source. So I will keep this "header" markup, but switch to
lighter weight "::" instead of ".. code-block:: c" to minimize speed
bumps reading the examples.

> > +/**
> > + * DOC: scope-based cleanup helpers
> > + *
> > + * The "goto error" pattern is notorious for introducing subtle resource
> > + * leaks. It is tedious and error prone to add new resource acquisition
> > + * constraints into code paths that already have several unwind
> > + * conditions. The "cleanup" helpers enable the compiler to help with
> > + * this tedium and can aid in maintaining FILO (first in last out)
> > + * unwind ordering to avoid unintentional leaks.
> 
> I'm not a data structures person, but I don't remember seeing "FILO"
> before.  IIUC, FILO == LIFO.  "FILO" appears about five times in the
> tree; "LIFO" about 25.

LIFO it is.

> 
> > + * As drivers make up the majority of the kernel code base lets describe
> > + * the Theory of Operation, Coding Style implications, and motivation
> > + * for using these helpers through the example of cleaning up PCI
> > + * drivers with DEFINE_FREE() and DEFINE_GUARD(), e.g.:
> 
> Maybe:
> 
>   As drivers make up the majority of the kernel code base, here is an
>   example of using these helpers to clean up PCI drivers with
>   DEFINE_FREE() and DEFINE_GUARD, e.g.,:
> 
> Or just s/lets/let's/, although to my ear "let's" is a suggestion and
> doesn't sound quite right in documentation.

Ok will just simplify to the following which also removes the need to
talk about the statistical motivations that you mention are awkward to
land in static documentation:

    As drivers make up the majority of the kernel code base, here is an
    example of using these helpers to clean up PCI drivers. The target of
    the cleanups are occasions where a goto is used to to unwind a device
    reference with pci_dev_put() or unlock the device with pci_dev_unlock().

> 
> > + * .. code-block:: c
> > + *
> > + *	DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
> > + *	DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
> 
> I think DEFINE_FREE() and DEFINE_GUARD() are separable concepts, so
> possibly move DEFINE_GUARD to that discussion.

Ok, will make it a pci_dev_put() section and pci_dev_unlock() section.

> > + * The DEFINE_FREE(pci_dev_put, ...) definition allows for declaring
> > + * variables like this:
> > + *
> > + * .. code-block:: c
> > + *
> > + *	struct pci_dev *dev __free(pci_dev_put) =
> > + *		pci_get_slot(parent, PCI_DEVFN(0, 0));
> > + *
> > + * The above 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
> > + * (i.e. without freeing it) on success, it can do:
> > + *
> > + * .. code-block:: c
> > + *
> > + *	return no_free_ptr(pdev);
> > + *
> > + * ...or:
> > + *
> > + * .. code-block:: c
> > + *
> > + *	return_ptr(pdev);
> > + *
> > + * Note that unwind order is dictated by declaration order.
> 
> Not only dictated, but it's strictly first-declared, last-unwound;
> i.e., unwind order is the reverse of the declaration order, right?

Yes, perhaps I will just quote the gcc documentation here:

"When multiple variables in the same scope have cleanup attributes, at
exit from the scope their associated cleanup functions are run in
reverse order of definition (last defined, first cleanup)."

> 
> > + * That
> > + * contraindicates a pattern like the following:
> > + *
> > + * .. code-block:: c
> > + *
> > + *	int num, ret = 0;
> > + *	struct pci_dev *bridge = ctrl->pcie->port;
> > + *	struct pci_bus *parent = bridge->subordinate;
> > + *	struct pci_dev *dev __free(pci_dev_put) = NULL;
> > + *
> > + *	pci_lock_rescan_remove();
> > + *
> > + *	dev = pci_get_slot(parent, PCI_DEVFN(0, 0));
> 
> As-is, I don't see the problem with this ordering.  I also don't see
> why num, ret, bridge, and parent are relevant.  I think maybe this
> just needs to be fleshed out a little more with a second cleanup to
> fully illustrate the problem.

Hmm, ok. I think this wants an explicit example of the trouble that can
happen when grouping variable definition at the start of a routine for
legacy code-prettiness concerns like x-mas tree declaration style rather
than observing that definition order has functional meaning.

> > + * In this case @dev is declared in x-mas tree style in a preamble
> > + * declaration block. That is problematic because it destroys the
> > + * compiler's ability to infer proper unwind order. If other cleanup
> > + * helpers appeared in such a function that depended on @dev being live
> > + * to complete their unwind then using the "struct obj_type *obj
> > + * __free(...) = NULL" style is an anti-pattern that potentially causes
> > + * a use-after-free bug. Instead, the expectation is this conversion:
> 
> I don't think "xmas-tree style" is relevant to the argument here.  The
> point is ordering with respect to other cleanup helpers.  I think it
> would be good to include another such helper directly in the example.

Will do.

> > + * .. code-block:: c
> > + *
> > + *	int num, ret = 0;
> > + *	struct pci_dev *bridge = ctrl->pcie->port;
> > + *	struct pci_bus *parent = bridge->subordinate;
> > + *
> > + *	pci_lock_rescan_remove();
> > + *
> > + *	struct pci_dev *dev __free(pci_dev_put) =
> > + *		pci_get_slot(parent, PCI_DEVFN(0, 0));
> > + *
> > + * ...which implies that declaring variables in mid-function scope is
> > + * not only allowed, but expected.
> 
> A declaration mid-function may be required in some cases, but it's not
> required here.  I'm not sure if adding an example to include a case
> where it is required would be useful or overkill.

So I was reacting to sentiment like this:

https://lore.kernel.org/netdev/6266c75a-c02a-431f-a4f2-43b51586ffb4@intel.com/

..where concern about cosmetics underestimate or misunderstand the
collision with functional behavior.

> > + * The motivation for deploying DEFINE_FREE(pci_dev_put, ...) is that at
> > + * the time of writing of this documentation there are ~590 instances of
> > + * pci_dev_put(), ~70 of them with 10 lines of a goto implying that a
> > + * significant number of gotos might be cleaned up for incremental
> > + * maintenance burden relief.
> 
> Good motivation for a commit log, but maybe a bit TMI for long-lived
> documentation.

Reduced it to the mention that "goto removal" is a virtue of these
helpers.

Powered by blists - more mailing lists