| Message ID | 171097196970.1011049.9726486429680041876.stgit@dwillia2-xfh.jf.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cleanup: Add usage and style documentation | expand |
> From: Dan Williams <dan.j.williams@intel.com> > 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?
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.
… > +++ 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: > 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@google.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> > Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Cc: Lukas Wunner <lukas.wunner@intel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Jonathan Corbet <corbet@lwn.net> > Signed-off-by: Dan Williams <dan.j.williams@intel.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() >
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.
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
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 <dan.j.williams@intel.com> > > 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 <dan.j.williams@intel.com> 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 <dan.j.williams@intel.com> 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 <bhelgaas@google.com> > > Cc: Ira Weiny <ira.weiny@intel.com> > > Cc: Jonathan Cameron <jonathan.cameron@huawei.com> > > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> > > Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Cc: Lukas Wunner <lukas.wunner@intel.com> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Jonathan Corbet <corbet@lwn.net> > > Signed-off-by: Dan Williams <dan.j.williams@intel.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.
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()
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@google.com> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Jonathan Cameron <jonathan.cameron@huawei.com> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Cc: Lukas Wunner <lukas.wunner@intel.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Jonathan Corbet <corbet@lwn.net> Signed-off-by: Dan Williams <dan.j.williams@intel.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. 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