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 <[email protected]>
Cc: Ira Weiny <[email protected]>
Cc: Jonathan Cameron <[email protected]>
Cc: Jesse Brandeburg <[email protected]>
Cc: Ilpo Järvinen <[email protected]>
Cc: Lukas Wunner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
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.
Linus, I include you directly since you have expressed some opinions on
how these helpers are used and want to capture that in a central
location.
This patch stops short of updating coding-style or checkpatch, but I
expect that it can later be used as a reference for that work.
Documentation/core-api/cleanup.rst | 8 +++
Documentation/core-api/index.rst | 1
include/linux/cleanup.h | 112 ++++++++++++++++++++++++++++++++++++
3 files changed, 121 insertions(+)
create mode 100644 Documentation/core-api/cleanup.rst
diff --git a/Documentation/core-api/cleanup.rst b/Documentation/core-api/cleanup.rst
new file mode 100644
index 000000000000..527eb2f8ec6e
--- /dev/null
+++ 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
diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
index 7a3a08d81f11..845fbd54948f 100644
--- a/Documentation/core-api/index.rst
+++ b/Documentation/core-api/index.rst
@@ -36,6 +36,7 @@ Library functionality that is used throughout the kernel.
kobject
kref
assoc_array
+ cleanup
xarray
maple_tree
idr
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..4620a475faee 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -4,6 +4,118 @@
#include <linux/compiler.h>
+/**
+ * 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.
+ *
+ * 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.:
+ *
+ * .. 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))
+ *
+ * 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. 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));
+ *
+ * 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:
+ *
+ * .. 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.
+ *
+ * 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.
+ *
+ * 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()
> From: Dan Williams <[email protected]>
> Sent: Thursday, March 21, 2024 6:05 AM
> + *
> + * Note that unwind order is dictated by declaration order. 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));
> + *
> + * 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:
> + *
an example of dependent cleanup helpers might be helpful to
better understand this expectation?
…
> +++ b/include/linux/cleanup.h
> @@ -4,6 +4,118 @@
>
> #include <linux/compiler.h>
>
> +/**
> + * DOC: scope-based cleanup helpers
> + *
> + * The "goto error" pattern is notorious for introducing …
Will any other label become more helpful for this description approach?
> + * this tedium and can aid in maintaining FILO (first in last out)
⬆
Would an other word be more appropriate here?
> + * contraindicates a pattern like the following:
I would prefer an other wording approach.
> + * struct pci_dev *dev __free(pci_dev_put) = NULL;
Programmers got used to null pointer initialisations.
> + * 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.
Can capabilities be clarified better for the applied compilers?
> 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.
I suggest to reconsider such a development concern in more detail.
> + * 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.
* Is there a need to separate the ellipsis from the subsequent word
by a space character?
* You propose a variable definition without specifying extra curly brackets
(for another compound statement / code block).
This can work only if an appropriate pointer is returned by the called function.
* The involved identifiers can occasionally get longer.
Further code layout challenges would need corresponding clarifications.
How will the handling of line length concerns evolve?
* I suggest to take another look also at the transformation pattern
“Reduce Scope of Variable”.
https://refactoring.com/catalog/reduceScopeOfVariable.html
> + * 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.
* How do you think about to specify any more resource cleanup functions
for growing usage of “smart pointers”?
* Would you like to extend the specification of function pairs for
improved applications of guard variants?
Regards,
Markus
On Wed, Mar 20, 2024 at 03:04:41PM -0700, Dan Williams wrote:
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index c2d09bc4f976..4620a475faee 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -4,6 +4,118 @@
>
> #include <linux/compiler.h>
>
> +/**
> + * 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.
> + *
> + * 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.:
> + *
> + * .. code-block:: c
> + *
So I despise all that RST stuff. It makes what should be trivially
readable text into a trainwreck. We're coders, we use text editors to
read comments.
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 <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Cc: Jonathan Cameron <[email protected]>
> Cc: Jesse Brandeburg <[email protected]>
> Cc: Ilpo Järvinen <[email protected]>
> Cc: Lukas Wunner <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> 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()
>
On Fri, Mar 22, 2024 at 02:00:42PM +0100, Markus Elfring wrote:
> …
> > +++ b/include/linux/cleanup.h
> > @@ -4,6 +4,118 @@
> >
> > #include <linux/compiler.h>
> >
> > +/**
> > + * DOC: scope-based cleanup helpers
> > + *
> > + * The "goto error" pattern is notorious for introducing …
>
> Will any other label become more helpful for this description approach?
>
>
> > + * this tedium and can aid in maintaining FILO (first in last out)
> ⬆
> Would an other word be more appropriate here?
>
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list. I strongly suggest that you not do this anymore. Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all. The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback. Please feel free to also ignore emails
from them.
thanks,
greg k-h's patch email bot
On Wed, Mar 20, 2024 at 03:04:41PM -0700, Dan Williams wrote:
> diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
> index 7a3a08d81f11..845fbd54948f 100644
> --- a/Documentation/core-api/index.rst
> +++ b/Documentation/core-api/index.rst
> @@ -36,6 +36,7 @@ Library functionality that is used throughout the kernel.
> kobject
> kref
> assoc_array
> + cleanup
> xarray
> maple_tree
> idr
Maybe move that up by a line? assoc_array, xarray, maple_tree, idr are
all data structures and it looks weird to have cleanup go in the middle
of them.
Peter Zijlstra wrote:
> On Wed, Mar 20, 2024 at 03:04:41PM -0700, Dan Williams wrote:
>
> > diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> > index c2d09bc4f976..4620a475faee 100644
> > --- a/include/linux/cleanup.h
> > +++ b/include/linux/cleanup.h
> > @@ -4,6 +4,118 @@
> >
> > #include <linux/compiler.h>
> >
> > +/**
> > + * 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.
> > + *
> > + * 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.:
> > + *
> > + * .. code-block:: c
> > + *
>
> So I despise all that RST stuff. It makes what should be trivially
> readable text into a trainwreck. We're coders, we use text editors to
> read comments.
Ok, I will rip out the RST stuff and just make this a standalone comment.
Tian, Kevin wrote:
> > From: Dan Williams <[email protected]>
> > Sent: Thursday, March 21, 2024 6:05 AM
> > + *
> > + * Note that unwind order is dictated by declaration order. 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));
> > + *
> > + * 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:
> > + *
>
> an example of dependent cleanup helpers might be helpful to
> better understand this expectation?
The simplest example I can think of to show the danger of the
"__free(...) = NULL" causing cleanup inter-dependency problems is the
following:
---
LIST_HEAD(list);
DEFINE_MUTEX(lock);
struct object {
struct list_head node;
};
static struct object *alloc_add(void)
{
struct object *obj;
lockdep_assert_held(&lock);
obj = kfree(sizeof(*obj), GFP_KERNEL);
if (obj) {
LIST_HEAD_INIT(&obj->node);
list_add(obj->node, &list):
}
return obj;
}
static void remove_free(struct object *obj)
{
lockdep_assert_held(&lock);
list_del(&obj->node);
kfree(obj);
}
DEFINE_FREE(remove_free, struct object *, if (_T) remove_free(_T))
static int init(void)
{
struct object *obj __free(remove_free) = NULL;
int err;
guard(mutex)(lock);
obj = alloc_add();
if (!obj)
return -ENOMEM;
err = other_init(obj);
if (err)
return err; // remove_free() called without the lock!!
no_free_ptr(obj);
return 0;
}
---
The fix for this bug is to replace the "__free(...) = NULL" pattern and
move the assignment to the declaration.
guard(mutex)(lock);
struct object *obj __free(remove_free) = alloc_add();
..so the compiler can observe LIFO order on the unwind. Yes, no one
should write code like this, all of the init should happen before
assigning to a list, but hopefully it illustrates the point.
> DEFINE_FREE(remove_free, struct object *, if (_T) remove_free(_T))
> static int init(void)
> {
> struct object *obj __free(remove_free) = NULL;
> int err;
>
> guard(mutex)(lock);
> obj = alloc_add();
>
> if (!obj)
> return -ENOMEM;
>
> err = other_init(obj);
> if (err)
> return err; // remove_free() called without the lock!!
>
> no_free_ptr(obj);
> return 0;
> }
You demonstrated an improvable lock granularity and a questionable combination
of variable scopes.
> The fix for this bug is to replace the "__free(...) = NULL" pattern and
> move the assignment to the declaration.
>
> guard(mutex)(lock);
> struct object *obj __free(remove_free) = alloc_add();
How do you think about to describe such a source code transformation
as a conversion of a variable assignment to a variable definition
at the place of a resource allocation?
Would you like to increase the collaboration with the macros “DEFINE_CLASS” and “CLASS”?
https://elixir.bootlin.com/linux/v6.8.1/source/include/linux/cleanup.h#L82
Regards,
Markus
On Fri, Mar 22, 2024 at 12:10:38PM -0700, Dan Williams wrote:
> Peter Zijlstra wrote:
> > So I despise all that RST stuff. It makes what should be trivially
> > readable text into a trainwreck. We're coders, we use text editors to
> > read comments.
>
> Ok, I will rip out the RST stuff and just make this a standalone comment.
I would rather you ignored Peter's persistent whining about RST and
kept the formatting.
Matthew Wilcox wrote:
> On Fri, Mar 22, 2024 at 12:10:38PM -0700, Dan Williams wrote:
> > Peter Zijlstra wrote:
> > > So I despise all that RST stuff. It makes what should be trivially
> > > readable text into a trainwreck. We're coders, we use text editors to
> > > read comments.
> >
> > Ok, I will rip out the RST stuff and just make this a standalone comment.
>
> I would rather you ignored Peter's persistent whining about RST and
> kept the formatting.
Hmm, how about split the difference and teach scripts/kernel-doc to treat
Peter's preferred markup for a C code example as a synonym, i.e.
effectively a search and replace of a line with only:
Ex.
..with:
.. code-block:: c
..within a kernel-doc DOC: section?
Might be easier said the done as I stare down a pile of perl. Maybe a
perl wrangler will come along and take pity on this patch.
On Sat, Mar 23, 2024 at 05:57:45PM -0700, Dan Williams wrote:
> Hmm, how about split the difference and teach scripts/kernel-doc to treat
> Peter's preferred markup for a C code example as a synonym, i.e.
> effectively a search and replace of a line with only:
>
> Ex.
>
> ...with:
>
> .. code-block:: c
>
> ...within a kernel-doc DOC: section?
>
> Might be easier said the done as I stare down a pile of perl. Maybe a
> perl wrangler will come along and take pity on this patch.
On line 757, there are two regexes...
#
# Regexes used only here.
#
my $sphinx_literal = '^[^.].*::$';
my $sphinx_cblock = '^\.\.\ +code-block::';
..which are (only) used immediately below in output_highlight_rst().
Amend those regexes to also match "Ex.", e.g.
my $sphinx_cblock = '^\.\.\ +(code-block::|Ex\.)';
Alternatively, add another variable definition and match against it
in output_highlight_rst().
A third alternative is to use the "::" syntax in lieu of
".. code-block:: c" in your C source file, if that causes
less eyesore for Peter. ;)
Thanks,
Lukas
Dan Williams <[email protected]> writes:
> Matthew Wilcox wrote:
>> On Fri, Mar 22, 2024 at 12:10:38PM -0700, Dan Williams wrote:
>> > Peter Zijlstra wrote:
>> > > So I despise all that RST stuff. It makes what should be trivially
>> > > readable text into a trainwreck. We're coders, we use text editors to
>> > > read comments.
>> >
>> > Ok, I will rip out the RST stuff and just make this a standalone comment.
>>
>> I would rather you ignored Peter's persistent whining about RST and
>> kept the formatting.
Dealing with that is definitely the least pleasant part of trying to
maintain docs...
> Hmm, how about split the difference and teach scripts/kernel-doc to treat
> Peter's preferred markup for a C code example as a synonym, i.e.
> effectively a search and replace of a line with only:
>
> Ex.
>
> ...with:
>
> .. code-block:: c
>
> ...within a kernel-doc DOC: section?
I'm not convinced that "Ex." is a clearer or more readable syntax, and
I'd prefer to avoid adding to the regex hell that kernel-doc already is
or adding more special syntax of our own. How about, as Lukas
suggested, just using the "::" notation? You get a nice literal block,
albeit without the syntax highlighting -- a worthwhile tradeoff, IMO.
Thanks,
jon
Jonathan Corbet wrote:
> Dan Williams <[email protected]> writes:
>
> > Matthew Wilcox wrote:
> >> On Fri, Mar 22, 2024 at 12:10:38PM -0700, Dan Williams wrote:
> >> > Peter Zijlstra wrote:
> >> > > So I despise all that RST stuff. It makes what should be trivially
> >> > > readable text into a trainwreck. We're coders, we use text editors to
> >> > > read comments.
> >> >
> >> > Ok, I will rip out the RST stuff and just make this a standalone comment.
> >>
> >> I would rather you ignored Peter's persistent whining about RST and
> >> kept the formatting.
>
> Dealing with that is definitely the least pleasant part of trying to
> maintain docs...
What is Linux development if not a surprising ongoing discovery of one-off
local preferences?
FWIW, I think the ability to embed RST markup directly into source code
documentation is a slick mechanism. It is something I welcome into any
file I maintain. At the same time, for files I do not maintain,
maintainer deference indicates "jettison some markup syntax to move the
bigger picture forward".
> > Hmm, how about split the difference and teach scripts/kernel-doc to treat
> > Peter's preferred markup for a C code example as a synonym, i.e.
> > effectively a search and replace of a line with only:
> >
> > Ex.
> >
> > ...with:
> >
> > .. code-block:: c
> >
> > ...within a kernel-doc DOC: section?
>
> I'm not convinced that "Ex." is a clearer or more readable syntax, and
> I'd prefer to avoid adding to the regex hell that kernel-doc already is
> or adding more special syntax of our own. How about, as Lukas
> suggested, just using the "::" notation? You get a nice literal block,
> albeit without the syntax highlighting -- a worthwhile tradeoff, IMO.
Sounds reasonable to me, will do that for v2.
Lukas, thanks for the help!
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 <[email protected]>
> > Cc: Ira Weiny <[email protected]>
> > Cc: Jonathan Cameron <[email protected]>
> > Cc: Jesse Brandeburg <[email protected]>
> > Cc: Ilpo J?rvinen <[email protected]>
> > Cc: Lukas Wunner <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Jonathan Corbet <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
> > ---
> > 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/[email protected]/
..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.
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 <[email protected]>
Cc: Ira Weiny <[email protected]>
Cc: Jonathan Cameron <[email protected]>
Cc: Jesse Brandeburg <[email protected]>
Cc: Ilpo Järvinen <[email protected]>
Cc: Lukas Wunner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Changes since v1:
* drop RST markup for C-syntax highlighting in examples, use the simpler
"::" annotation (Peter, Lukas, Jon)
* fixup ordering of core-api sections (Matthew)
* describe ordering as LIFO (Bjorn)
* wait to talk about DEFINE_GUARD() until after DEFINE_FREE() section
(Bjorn)
* clarify the "definition order matters" concern (Bjorn)
* drop statistics for goto patterns in source, "TMI" (Bjorn)
* include example of order dependent cleanups helpers (Kevin, Bjorn)
Documentation/core-api/cleanup.rst | 8 ++
Documentation/core-api/index.rst | 1
include/linux/cleanup.h | 151 ++++++++++++++++++++++++++++++++++++
3 files changed, 160 insertions(+)
create mode 100644 Documentation/core-api/cleanup.rst
diff --git a/Documentation/core-api/cleanup.rst b/Documentation/core-api/cleanup.rst
new file mode 100644
index 000000000000..527eb2f8ec6e
--- /dev/null
+++ 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
diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
index 7a3a08d81f11..2d2b3719567e 100644
--- a/Documentation/core-api/index.rst
+++ b/Documentation/core-api/index.rst
@@ -35,6 +35,7 @@ Library functionality that is used throughout the kernel.
kobject
kref
+ cleanup
assoc_array
xarray
maple_tree
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..8ef2d91c2cbf 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -4,6 +4,157 @@
#include <linux/compiler.h>
+/**
+ * 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.
+ *
+ * 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 unwind a device
+ * reference (pci_dev_put()), or unlock the device (pci_dev_unlock())
+ * before returning.
+ *
+ * The DEFINE_FREE() macro can arrange for PCI device references to be
+ * dropped when the associated variable goes out of scope:
+ *
+ * ::
+ *
+ * DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
+ * ...
+ * 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 @dev is non-NULL
+ * when @dev goes out of scope (automatic variable scope). If a function
+ * wants to invoke pci_dev_put() on error, but return @dev (i.e. without
+ * freeing it) on success, it can do:
+ *
+ * ::
+ *
+ * return no_free_ptr(dev);
+ *
+ * ...or:
+ *
+ * ::
+ *
+ * return_ptr(dev);
+ *
+ * The DEFINE_GUARD() macro can arrange for the PCI device lock to be
+ * dropped when the scope where guard() is invoked ends:
+ *
+ * ::
+ *
+ * DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
+ * ...
+ * guard(pci_dev)(dev);
+ *
+ *
+ * The lifetime of the lock obtained by the guard() helper follows the
+ * scope of automatic variable declaration. Take the following example:
+ *
+ * ::
+ *
+ * func(...)
+ * {
+ * if (...) {
+ * ...
+ * guard(pci_dev)(dev); // pci_dev_lock() invoked here
+ * ...
+ * } // <- implied pci_dev_unlock() triggered here
+ * }
+ *
+ * Observe the lock is held for the remainder of the "if ()" block not
+ * the remainder of "func()".
+ *
+ * Now, when a function uses both __free() and guard(), or multiple
+ * instances of __free(), the LIFO order of variable definition order
+ * matters. GCC documentation says:
+ *
+ * "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)."
+ *
+ * When the unwind order matters it requires that variables be defined
+ * mid-function scope rather than at the top of the file. Take the
+ * following example and notice the bug highlighted by "!!":
+ *
+ * ::
+ *
+ * LIST_HEAD(list);
+ * DEFINE_MUTEX(lock);
+ *
+ * struct object {
+ * struct list_head node;
+ * };
+ *
+ * static struct object *alloc_add(void)
+ * {
+ * struct object *obj;
+ *
+ * lockdep_assert_held(&lock);
+ * obj = kfree(sizeof(*obj), GFP_KERNEL);
+ * if (obj) {
+ * LIST_HEAD_INIT(&obj->node);
+ * list_add(obj->node, &list):
+ * }
+ * return obj;
+ * }
+ *
+ * static void remove_free(struct object *obj)
+ * {
+ * lockdep_assert_held(&lock);
+ * list_del(&obj->node);
+ * kfree(obj);
+ * }
+ *
+ * DEFINE_FREE(remove_free, struct object *, if (_T) remove_free(_T))
+ * static int init(void)
+ * {
+ * struct object *obj __free(remove_free) = NULL;
+ * int err;
+ *
+ * guard(mutex)(&lock);
+ * obj = alloc_add();
+ *
+ * if (!obj)
+ * return -ENOMEM;
+ *
+ * err = other_init(obj);
+ * if (err)
+ * return err; // remove_free() called without the lock!!
+ *
+ * no_free_ptr(obj);
+ * return 0;
+ * }
+ *
+ * That bug is fixed by changing init() to call guard() and define +
+ * initialize @obj in this order:
+ *
+ * ::
+ *
+ * guard(mutex)(&lock);
+ * struct object *obj __free(remove_free) = alloc_add();
+ *
+ * Given that the "__free(...) = NULL" pattern for variables defined at
+ * the top of the function poses this potential interdependency problem
+ * the recommendation is to always define and assign variables in one
+ * statement and not group variable definitions at the top of the
+ * function when __free() is used.
+ *
+ * Lastly, given that the benefit of cleanup helpers is removal of
+ * "goto", and that the "goto" statement can jump between scopes, the
+ * expectation is that usage of "goto" and cleanup helpers is never
+ * mixed in the same function. I.e. for a given routine, convert all
+ * resources that need a "goto" cleanup to scope-based cleanup, or
+ * convert none of them.
+ */
+
/*
* DEFINE_FREE(name, type, free):
* simple helper macro that defines the required wrapper for a __free()
…
> +++ b/include/linux/cleanup.h
> @@ -4,6 +4,157 @@
>
> #include <linux/compiler.h>
>
> +/**
> + * DOC: scope-based cleanup helpers
> + *
> + * The "goto error" pattern is notorious for introducing …
Will any other label become more helpful for this description approach?
> + * this tedium …
Would an other wording be more appropriate here?
> + * … maintaining FILO (first in last out)
How does this text fit to your response from yesterday?
https://lore.kernel.org/all/6601c7f7369d4_2690d29490@dwillia2-mobl3.amr.corp.intel.com.notmuch/
> + * … If a function
> + * wants to invoke pci_dev_put() on error, but return @dev (i.e. without
> + * freeing it) on success, it can do:
> + *
> + * ::
> + *
> + * return no_free_ptr(dev);
> + *
> + * ...or:
> + *
> + * ::
> + *
> + * return_ptr(dev);
…
Would this macro call be preferred as a succinct specification
(so that only the shorter one should be mentioned here)?
https://elixir.bootlin.com/linux/v6.8.1/source/include/linux/cleanup.h#L78
> + * Observe the lock is held for the remainder of the "if ()" block not
> + * the remainder of "func()".
I suggest to add a word in this sentence.
* Observe the lock is held for the remainder of the "if ()" block
* (and not the remainder of "func()").
> + * the top of the function poses this potential interdependency problem
I suggest to add a comma at the end of this line.
> + * the recommendation is to always define and assign variables in one
> + * statement and not group variable definitions at the top of the
> + * function when __free() is used.
I became curious how code layout guidance will evolve further also
according to such an advice.
Would you like to increase the collaboration with the macros “DEFINE_CLASS” and “CLASS”?
https://elixir.bootlin.com/linux/v6.8.1/source/include/linux/cleanup.h#L82
Regards,
Markus
One little nit...
Dan Williams <[email protected]> writes:
> + * The DEFINE_FREE() macro can arrange for PCI device references to be
> + * dropped when the associated variable goes out of scope:
> + *
> + * ::
> + *
This can be written a bit more concisely as:
...goes out of scope::
without the separate "::" line, reducing the markup noise a bit more.
Thanks,
jon
Jonathan Corbet wrote:
> One little nit...
>
> Dan Williams <[email protected]> writes:
>
> > + * The DEFINE_FREE() macro can arrange for PCI device references to be
> > + * dropped when the associated variable goes out of scope:
> > + *
> > + * ::
> > + *
>
> This can be written a bit more concisely as:
>
> ...goes out of scope::
>
> without the separate "::" line, reducing the markup noise a bit more.
Oh, nice! Today I learned...
On Mon, 25 Mar 2024 15:57:42 -0700
Dan Williams <[email protected]> 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 <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Cc: Jonathan Cameron <[email protected]>
> Cc: Jesse Brandeburg <[email protected]>
> Cc: Ilpo J?rvinen <[email protected]>
> Cc: Lukas Wunner <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
Other than the formatting improvement Jon suggested, this looks good to me
and corresponds to my understanding of how this stuff should be used.
FWIW
Reviewed-by: Jonathan Cameron <[email protected]>
Dan Williams wrote:
[..]
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index c2d09bc4f976..8ef2d91c2cbf 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -4,6 +4,157 @@
>
> #include <linux/compiler.h>
>
> +/**
> + * 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)
Missed this FILO => LIFO conversion per Bjorn.
Dan Williams wrote:
[..]
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index c2d09bc4f976..8ef2d91c2cbf 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
[..]
> + * When the unwind order matters it requires that variables be defined
> + * mid-function scope rather than at the top of the file. Take the
> + * following example and notice the bug highlighted by "!!":
> + *
> + * ::
> + *
> + * LIST_HEAD(list);
> + * DEFINE_MUTEX(lock);
> + *
> + * struct object {
> + * struct list_head node;
> + * };
> + *
> + * static struct object *alloc_add(void)
> + * {
> + * struct object *obj;
> + *
> + * lockdep_assert_held(&lock);
> + * obj = kfree(sizeof(*obj), GFP_KERNEL);
This should be kzalloc(), and I should note that this example is of the
UNTESTED variety.
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 <[email protected]>
Cc: Ira Weiny <[email protected]>
Cc: Jesse Brandeburg <[email protected]>
Cc: Ilpo Järvinen <[email protected]>
Cc: Lukas Wunner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Changes since v2:
* remove the unnecessary newlines around code examples further reducing
the visual interruption of RST metadata (Jon)
* Fix a missing FILO=>LIFO conversion
* Fix a bug in the example
* Collect Jonathan's reviewed-by
Review has been quiet on this thread for a few days so I think is the
final rev. Let me know if anything else to fix up.
Documentation/core-api/cleanup.rst | 8 ++
Documentation/core-api/index.rst | 1
include/linux/cleanup.h | 136 ++++++++++++++++++++++++++++++++++++
3 files changed, 145 insertions(+)
create mode 100644 Documentation/core-api/cleanup.rst
diff --git a/Documentation/core-api/cleanup.rst b/Documentation/core-api/cleanup.rst
new file mode 100644
index 000000000000..527eb2f8ec6e
--- /dev/null
+++ 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
diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
index 7a3a08d81f11..2d2b3719567e 100644
--- a/Documentation/core-api/index.rst
+++ b/Documentation/core-api/index.rst
@@ -35,6 +35,7 @@ Library functionality that is used throughout the kernel.
kobject
kref
+ cleanup
assoc_array
xarray
maple_tree
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..5ffb2127e3ac 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -4,6 +4,142 @@
#include <linux/compiler.h>
+/**
+ * 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 LIFO (last in first out)
+ * unwind ordering to avoid unintentional leaks.
+ *
+ * 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 unwind a device
+ * reference (pci_dev_put()), or unlock the device (pci_dev_unlock())
+ * before returning.
+ *
+ * The DEFINE_FREE() macro can arrange for PCI device references to be
+ * dropped when the associated variable goes out of scope::
+ *
+ * DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
+ * ...
+ * 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 @dev is non-NULL
+ * when @dev goes out of scope (automatic variable scope). If a function
+ * wants to invoke pci_dev_put() on error, but return @dev (i.e. without
+ * freeing it) on success, it can do::
+ *
+ * return no_free_ptr(dev);
+ *
+ * ...or::
+ *
+ * return_ptr(dev);
+ *
+ * The DEFINE_GUARD() macro can arrange for the PCI device lock to be
+ * dropped when the scope where guard() is invoked ends::
+ *
+ * DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
+ * ...
+ * guard(pci_dev)(dev);
+ *
+ * The lifetime of the lock obtained by the guard() helper follows the
+ * scope of automatic variable declaration. Take the following example::
+ *
+ * func(...)
+ * {
+ * if (...) {
+ * ...
+ * guard(pci_dev)(dev); // pci_dev_lock() invoked here
+ * ...
+ * } // <- implied pci_dev_unlock() triggered here
+ * }
+ *
+ * Observe the lock is held for the remainder of the "if ()" block not
+ * the remainder of "func()".
+ *
+ * Now, when a function uses both __free() and guard(), or multiple
+ * instances of __free(), the LIFO order of variable definition order
+ * matters. GCC documentation says:
+ *
+ * "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)."
+ *
+ * When the unwind order matters it requires that variables be defined
+ * mid-function scope rather than at the top of the file. Take the
+ * following example and notice the bug highlighted by "!!"::
+ *
+ * LIST_HEAD(list);
+ * DEFINE_MUTEX(lock);
+ *
+ * struct object {
+ * struct list_head node;
+ * };
+ *
+ * static struct object *alloc_add(void)
+ * {
+ * struct object *obj;
+ *
+ * lockdep_assert_held(&lock);
+ * obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+ * if (obj) {
+ * LIST_HEAD_INIT(&obj->node);
+ * list_add(obj->node, &list):
+ * }
+ * return obj;
+ * }
+ *
+ * static void remove_free(struct object *obj)
+ * {
+ * lockdep_assert_held(&lock);
+ * list_del(&obj->node);
+ * kfree(obj);
+ * }
+ *
+ * DEFINE_FREE(remove_free, struct object *, if (_T) remove_free(_T))
+ * static int init(void)
+ * {
+ * struct object *obj __free(remove_free) = NULL;
+ * int err;
+ *
+ * guard(mutex)(&lock);
+ * obj = alloc_add();
+ *
+ * if (!obj)
+ * return -ENOMEM;
+ *
+ * err = other_init(obj);
+ * if (err)
+ * return err; // remove_free() called without the lock!!
+ *
+ * no_free_ptr(obj);
+ * return 0;
+ * }
+ *
+ * That bug is fixed by changing init() to call guard() and define +
+ * initialize @obj in this order::
+ *
+ * guard(mutex)(&lock);
+ * struct object *obj __free(remove_free) = alloc_add();
+ *
+ * Given that the "__free(...) = NULL" pattern for variables defined at
+ * the top of the function poses this potential interdependency problem
+ * the recommendation is to always define and assign variables in one
+ * statement and not group variable definitions at the top of the
+ * function when __free() is used.
+ *
+ * Lastly, given that the benefit of cleanup helpers is removal of
+ * "goto", and that the "goto" statement can jump between scopes, the
+ * expectation is that usage of "goto" and cleanup helpers is never
+ * mixed in the same function. I.e. for a given routine, convert all
+ * resources that need a "goto" cleanup to scope-based cleanup, or
+ * convert none of them.
+ */
+
/*
* DEFINE_FREE(name, type, free):
* simple helper macro that defines the required wrapper for a __free()
> Changes since v2:
> * remove the unnecessary newlines around code examples further reducing
> the visual interruption of RST metadata (Jon)
> * Fix a missing FILO=>LIFO conversion
> * Fix a bug in the example
I find such improvements nice.
> * Collect Jonathan's reviewed-by
How good does this action fit to the event that you pointed issues out also yourself?
> Review has been quiet on this thread for a few days so I think is the
> final rev.
I got an other impression.
> Let me know if anything else to fix up.
I would appreciate if further patch review comments can get more attention.
…
> +++ b/include/linux/cleanup.h
> @@ -4,6 +4,142 @@
>
> #include <linux/compiler.h>
>
> +/**
> + * DOC: scope-based cleanup helpers
> + *
> + * The "goto error" pattern is notorious for introducing …
Will any other label become more helpful for this description approach?
> + * this tedium …
Would an other wording be more appropriate here?
> + * … If a function
> + * wants to invoke pci_dev_put() on error, but return @dev (i.e. without
> + * freeing it) on success, it can do:
> + *
> + * ::
> + *
> + * return no_free_ptr(dev);
> + *
> + * ...or:
> + *
> + * ::
> + *
> + * return_ptr(dev);
…
Would this macro call be preferred as a succinct specification
(so that only the shorter one should be mentioned here)?
https://elixir.bootlin.com/linux/v6.8.2/source/include/linux/cleanup.h#L78
> + * Observe the lock is held for the remainder of the "if ()" block not
> + * the remainder of "func()".
I suggest to add a word in this sentence.
* Observe the lock is held for the remainder of the "if ()" block
* (and not the remainder of "func()").
> + * That bug is fixed by changing init() to call guard() and define +
> + * initialize @obj in this order::
> + *
> + * guard(mutex)(&lock);
> + * struct object *obj __free(remove_free) = alloc_add();
It is helpful to point such a design possibility and preference out.
But I imagine that the abstraction level should be raised another bit.
It seems that the mentioned variable definition should be achieved by
calling the macro “CLASS” instead for “an instance of the named class”.
Thus the macro “DEFINE_CLASS” should also be called before accordingly.
https://elixir.bootlin.com/linux/v6.8.2/source/include/linux/cleanup.h#L82
> + * the top of the function poses this potential interdependency problem
I suggest to add a comma at the end of this line.
> + * the recommendation is to always define and assign variables in one
> + * statement and not group variable definitions at the top of the
> + * function when __free() is used.
I became curious how code layout guidance will evolve further also
according to such an advice.
> + * Lastly, given that the benefit of cleanup helpers is removal of
> + * "goto", and that the "goto" statement can jump between scopes, the
> + * expectation is that usage of "goto" and cleanup helpers is never
> + * mixed in the same function. I.e. for a given routine, convert all
> + * resources that need a "goto" cleanup to scope-based cleanup, or
> + * convert none of them.
Can the word wrapping become nicer another bit?
* Lastly, given that the benefit of cleanup helpers is removal of "goto",
* and that the "goto" statement can jump between scopes,
* the expectation is that usage of "goto" and cleanup helpers is never
* mixed in the same function. I.e. for a given routine, convert all
* resources that need a "goto" cleanup to scope-based cleanup,
* or convert none of them.
Regards,
Markus
> From: Dan Williams <[email protected]>
> Sent: Saturday, March 30, 2024 7:49 AM
>
> 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 <[email protected]>
> Cc: Ira Weiny <[email protected]>
> Cc: Jesse Brandeburg <[email protected]>
> Cc: Ilpo Järvinen <[email protected]>
> Cc: Lukas Wunner <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Reviewed-by: Jonathan Cameron <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
> Reviewed-by: Kevin Tian <[email protected]>
Did you take any of my review comments into account for this patch version so far?
Regards,
Markus