Message-ID: <20240322133434.GA1340126@bhelgaas>
Date: Fri, 22 Mar 2024 08:34:34 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: 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

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.

> +/**
> + * 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.

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

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

> + * 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?

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

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

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

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

> + * The guard() helper holds the associated lock for the remainder of the
> + * current scope in which it was invoked. So, for example:
> + *
> + * .. code-block:: c
> + *
> + *	func(...)
> + *	{
> + *		if (...) {
> + *			...
> + *			guard(pci_dev); // pci_dev_lock() invoked here
> + *			...
> + *		} // <- implied pci_dev_unlock() triggered here
> + *	}
> + *
> + * ...in other words, the lock is held for the remainder of the current
> + * scope not the remainder of "func()".
> + *
> + * At the time of writing there are 15 invocations of pci_dev_unlock() in
> + * the kernel with 5 within 10 lines of a goto.
> + *
> + * Conversions of existing code to use cleanup helpers should convert
> + * all resources so that no "goto" unwind statements remain. If not all
> + * resources are amenable to cleanup then additional refactoring is
> + * needed to build helper functions, or the function is simply not a
> + * good candidate for conversion.
> + */
> +
>  /*
>   * DEFINE_FREE(name, type, free):
>   *	simple helper macro that defines the required wrapper for a __free()
> 

Powered by blists - more mailing lists