* RE: [PATCH] cleanup: Add usage and style documentation
2024-03-20 22:04 [PATCH] cleanup: Add usage and style documentation Dan Williams
@ 2024-03-22 5:43 ` Tian, Kevin
2024-03-23 0:17 ` Dan Williams
2024-03-22 9:06 ` Peter Zijlstra
` (4 subsequent siblings)
5 siblings, 1 reply; 27+ messages in thread
From: Tian, Kevin @ 2024-03-22 5:43 UTC (permalink / raw)
To: Williams, Dan J, peterz, torvalds
Cc: Bjorn Helgaas, Weiny, Ira, Jonathan Cameron, Brandeburg, Jesse,
Ilpo Järvinen, Wunner, Lukas, Jonathan Corbet, linux-pci,
linux-kernel, gregkh, linux-doc
> 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?
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH] cleanup: Add usage and style documentation
2024-03-22 5:43 ` Tian, Kevin
@ 2024-03-23 0:17 ` Dan Williams
2024-03-23 18:01 ` Markus Elfring
0 siblings, 1 reply; 27+ messages in thread
From: Dan Williams @ 2024-03-23 0:17 UTC (permalink / raw)
To: Tian, Kevin, Williams, Dan J, peterz, torvalds
Cc: Bjorn Helgaas, Weiny, Ira, Jonathan Cameron, Brandeburg, Jesse,
Ilpo Järvinen, Wunner, Lukas, Jonathan Corbet, linux-pci,
linux-kernel, gregkh, linux-doc
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH] cleanup: Add usage and style documentation
2024-03-23 0:17 ` Dan Williams
@ 2024-03-23 18:01 ` Markus Elfring
0 siblings, 0 replies; 27+ messages in thread
From: Markus Elfring @ 2024-03-23 18:01 UTC (permalink / raw)
To: Dan Williams, Peter Zijlstra, Linus Torvalds, Jonathan Corbet,
linux-doc, linux-kernel, linux-pci, kernel-janitors
Cc: Bjorn Helgaas, Greg Kroah-Hartman, Ilpo Järvinen, Ira Weiny,
Jesse Brandeburg, Jonathan Cameron, Julia Lawall, Kevin Tian,
Lukas Wunner
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-20 22:04 [PATCH] cleanup: Add usage and style documentation Dan Williams
2024-03-22 5:43 ` Tian, Kevin
@ 2024-03-22 9:06 ` Peter Zijlstra
2024-03-22 19:10 ` Dan Williams
2024-03-22 13:00 ` Markus Elfring
` (3 subsequent siblings)
5 siblings, 1 reply; 27+ messages in thread
From: Peter Zijlstra @ 2024-03-22 9:06 UTC (permalink / raw)
To: Dan Williams
Cc: torvalds, Bjorn Helgaas, Ira Weiny, Jonathan Cameron,
Jesse Brandeburg, Ilpo Järvinen, Lukas Wunner,
Jonathan Corbet, linux-pci, linux-kernel, gregkh, linux-doc
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-22 9:06 ` Peter Zijlstra
@ 2024-03-22 19:10 ` Dan Williams
2024-03-23 20:44 ` Matthew Wilcox
0 siblings, 1 reply; 27+ messages in thread
From: Dan Williams @ 2024-03-22 19:10 UTC (permalink / raw)
To: Peter Zijlstra, Dan Williams
Cc: torvalds, Bjorn Helgaas, Ira Weiny, Jonathan Cameron,
Jesse Brandeburg, Ilpo Järvinen, Lukas Wunner,
Jonathan Corbet, linux-pci, linux-kernel, gregkh, linux-doc
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-22 19:10 ` Dan Williams
@ 2024-03-23 20:44 ` Matthew Wilcox
2024-03-24 0:57 ` Dan Williams
0 siblings, 1 reply; 27+ messages in thread
From: Matthew Wilcox @ 2024-03-23 20:44 UTC (permalink / raw)
To: Dan Williams
Cc: Peter Zijlstra, torvalds, Bjorn Helgaas, Ira Weiny,
Jonathan Cameron, Jesse Brandeburg, Ilpo Järvinen,
Lukas Wunner, Jonathan Corbet, linux-pci, linux-kernel, gregkh,
linux-doc
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-23 20:44 ` Matthew Wilcox
@ 2024-03-24 0:57 ` Dan Williams
2024-03-24 6:23 ` Lukas Wunner
2024-03-24 9:08 ` Jonathan Corbet
0 siblings, 2 replies; 27+ messages in thread
From: Dan Williams @ 2024-03-24 0:57 UTC (permalink / raw)
To: Matthew Wilcox, Dan Williams
Cc: Peter Zijlstra, torvalds, Bjorn Helgaas, Ira Weiny,
Jonathan Cameron, Jesse Brandeburg, Ilpo Järvinen,
Lukas Wunner, Jonathan Corbet, linux-pci, linux-kernel, gregkh,
linux-doc
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-24 0:57 ` Dan Williams
@ 2024-03-24 6:23 ` Lukas Wunner
2024-03-24 9:08 ` Jonathan Corbet
1 sibling, 0 replies; 27+ messages in thread
From: Lukas Wunner @ 2024-03-24 6:23 UTC (permalink / raw)
To: Dan Williams
Cc: Matthew Wilcox, Peter Zijlstra, torvalds, Bjorn Helgaas,
Ira Weiny, Jonathan Cameron, Jesse Brandeburg,
Ilpo Järvinen, Jonathan Corbet, linux-pci, linux-kernel,
gregkh, linux-doc
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-24 0:57 ` Dan Williams
2024-03-24 6:23 ` Lukas Wunner
@ 2024-03-24 9:08 ` Jonathan Corbet
2024-03-24 20:37 ` Dan Williams
1 sibling, 1 reply; 27+ messages in thread
From: Jonathan Corbet @ 2024-03-24 9:08 UTC (permalink / raw)
To: Dan Williams, Matthew Wilcox, Dan Williams
Cc: Peter Zijlstra, torvalds, Bjorn Helgaas, Ira Weiny,
Jonathan Cameron, Jesse Brandeburg, Ilpo Järvinen,
Lukas Wunner, linux-pci, linux-kernel, gregkh, linux-doc
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-24 9:08 ` Jonathan Corbet
@ 2024-03-24 20:37 ` Dan Williams
0 siblings, 0 replies; 27+ messages in thread
From: Dan Williams @ 2024-03-24 20:37 UTC (permalink / raw)
To: Jonathan Corbet, Dan Williams, Matthew Wilcox
Cc: Peter Zijlstra, torvalds, Bjorn Helgaas, Ira Weiny,
Jonathan Cameron, Jesse Brandeburg, Ilpo Järvinen,
Lukas Wunner, linux-pci, linux-kernel, gregkh, linux-doc
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!
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-20 22:04 [PATCH] cleanup: Add usage and style documentation Dan Williams
2024-03-22 5:43 ` Tian, Kevin
2024-03-22 9:06 ` Peter Zijlstra
@ 2024-03-22 13:00 ` Markus Elfring
2024-03-22 13:48 ` Greg Kroah-Hartman
2024-03-22 13:34 ` Bjorn Helgaas
` (2 subsequent siblings)
5 siblings, 1 reply; 27+ messages in thread
From: Markus Elfring @ 2024-03-22 13:00 UTC (permalink / raw)
To: Dan Williams, Peter Zijlstra, Linus Torvalds, Jonathan Corbet,
linux-doc, linux-kernel, linux-pci, kernel-janitors
Cc: Bjorn Helgaas, Greg Kroah-Hartman, Ilpo Järvinen, Ira Weiny,
Jesse Brandeburg, Jonathan Cameron, Julia Lawall, Lukas Wunner
…
> +++ 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-22 13:00 ` Markus Elfring
@ 2024-03-22 13:48 ` Greg Kroah-Hartman
0 siblings, 0 replies; 27+ messages in thread
From: Greg Kroah-Hartman @ 2024-03-22 13:48 UTC (permalink / raw)
To: Markus Elfring
Cc: Dan Williams, Peter Zijlstra, Linus Torvalds, Jonathan Corbet,
linux-doc, linux-kernel, linux-pci, kernel-janitors,
Bjorn Helgaas, Ilpo Järvinen, Ira Weiny, Jesse Brandeburg,
Jonathan Cameron, Julia Lawall, Lukas Wunner
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-20 22:04 [PATCH] cleanup: Add usage and style documentation Dan Williams
` (2 preceding siblings ...)
2024-03-22 13:00 ` Markus Elfring
@ 2024-03-22 13:34 ` Bjorn Helgaas
2024-03-25 18:52 ` Dan Williams
2024-03-22 13:45 ` Matthew Wilcox
2024-03-25 22:57 ` [PATCH v2] " Dan Williams
5 siblings, 1 reply; 27+ messages in thread
From: Bjorn Helgaas @ 2024-03-22 13:34 UTC (permalink / raw)
To: Dan Williams
Cc: peterz, torvalds, Bjorn Helgaas, Ira Weiny, Jonathan Cameron,
Jesse Brandeburg, Ilpo Järvinen, Lukas Wunner,
Jonathan Corbet, linux-pci, linux-kernel, gregkh, linux-doc
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()
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-22 13:34 ` Bjorn Helgaas
@ 2024-03-25 18:52 ` Dan Williams
0 siblings, 0 replies; 27+ messages in thread
From: Dan Williams @ 2024-03-25 18:52 UTC (permalink / raw)
To: Bjorn Helgaas, Dan Williams
Cc: peterz, torvalds, Bjorn Helgaas, Ira Weiny, Jonathan Cameron,
Jesse Brandeburg, Ilpo Järvinen, Lukas Wunner,
Jonathan Corbet, linux-pci, linux-kernel, gregkh, linux-doc
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] cleanup: Add usage and style documentation
2024-03-20 22:04 [PATCH] cleanup: Add usage and style documentation Dan Williams
` (3 preceding siblings ...)
2024-03-22 13:34 ` Bjorn Helgaas
@ 2024-03-22 13:45 ` Matthew Wilcox
2024-03-25 22:57 ` [PATCH v2] " Dan Williams
5 siblings, 0 replies; 27+ messages in thread
From: Matthew Wilcox @ 2024-03-22 13:45 UTC (permalink / raw)
To: Dan Williams
Cc: peterz, torvalds, Bjorn Helgaas, Ira Weiny, Jonathan Cameron,
Jesse Brandeburg, Ilpo Järvinen, Lukas Wunner,
Jonathan Corbet, linux-pci, linux-kernel, gregkh, linux-doc
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] cleanup: Add usage and style documentation
2024-03-20 22:04 [PATCH] cleanup: Add usage and style documentation Dan Williams
` (4 preceding siblings ...)
2024-03-22 13:45 ` Matthew Wilcox
@ 2024-03-25 22:57 ` Dan Williams
2024-03-26 12:06 ` Markus Elfring
` (5 more replies)
5 siblings, 6 replies; 27+ messages in thread
From: Dan Williams @ 2024-03-25 22:57 UTC (permalink / raw)
To: peterz, torvalds
Cc: Bjorn Helgaas, Ira Weiny, Jonathan Cameron, Jesse Brandeburg,
Ilpo Järvinen, Lukas Wunner, Jonathan Corbet, linux-pci,
linux-kernel, gregkh, linux-doc
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.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
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()
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cleanup: Add usage and style documentation
2024-03-25 22:57 ` [PATCH v2] " Dan Williams
@ 2024-03-26 12:06 ` Markus Elfring
2024-03-26 15:35 ` Jonathan Corbet
` (4 subsequent siblings)
5 siblings, 0 replies; 27+ messages in thread
From: Markus Elfring @ 2024-03-26 12:06 UTC (permalink / raw)
To: Dan Williams, Peter Zijlstra, Linus Torvalds, Jonathan Corbet,
linux-doc, linux-kernel, linux-pci, kernel-janitors
Cc: Bjorn Helgaas, Greg Kroah-Hartman, Ilpo Järvinen, Ira Weiny,
Jesse Brandeburg, Jonathan Cameron, Julia Lawall, Kevin Tian,
Lukas Wunner, Matthew Wilcox
…
> +++ 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cleanup: Add usage and style documentation
2024-03-25 22:57 ` [PATCH v2] " Dan Williams
2024-03-26 12:06 ` Markus Elfring
@ 2024-03-26 15:35 ` Jonathan Corbet
2024-03-26 16:51 ` Dan Williams
2024-03-26 16:56 ` Jonathan Cameron
` (3 subsequent siblings)
5 siblings, 1 reply; 27+ messages in thread
From: Jonathan Corbet @ 2024-03-26 15:35 UTC (permalink / raw)
To: Dan Williams, peterz, torvalds
Cc: Bjorn Helgaas, Ira Weiny, Jonathan Cameron, Jesse Brandeburg,
Ilpo Järvinen, Lukas Wunner, linux-pci, linux-kernel,
gregkh, linux-doc
One little nit...
Dan Williams <dan.j.williams@intel.com> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cleanup: Add usage and style documentation
2024-03-26 15:35 ` Jonathan Corbet
@ 2024-03-26 16:51 ` Dan Williams
0 siblings, 0 replies; 27+ messages in thread
From: Dan Williams @ 2024-03-26 16:51 UTC (permalink / raw)
To: Jonathan Corbet, Dan Williams, peterz, torvalds
Cc: Bjorn Helgaas, Ira Weiny, Jonathan Cameron, Jesse Brandeburg,
Ilpo Järvinen, Lukas Wunner, linux-pci, linux-kernel,
gregkh, linux-doc
Jonathan Corbet wrote:
> One little nit...
>
> Dan Williams <dan.j.williams@intel.com> 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...
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cleanup: Add usage and style documentation
2024-03-25 22:57 ` [PATCH v2] " Dan Williams
2024-03-26 12:06 ` Markus Elfring
2024-03-26 15:35 ` Jonathan Corbet
@ 2024-03-26 16:56 ` Jonathan Cameron
2024-03-26 17:49 ` Dan Williams
` (2 subsequent siblings)
5 siblings, 0 replies; 27+ messages in thread
From: Jonathan Cameron @ 2024-03-26 16:56 UTC (permalink / raw)
To: Dan Williams
Cc: peterz, torvalds, Bjorn Helgaas, Ira Weiny, Jesse Brandeburg,
Ilpo Järvinen, Lukas Wunner, Jonathan Corbet, linux-pci,
linux-kernel, gregkh, linux-doc
On Mon, 25 Mar 2024 15:57:42 -0700
Dan Williams <dan.j.williams@intel.com> 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.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
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 <Jonathan.Cameron@huawei.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cleanup: Add usage and style documentation
2024-03-25 22:57 ` [PATCH v2] " Dan Williams
` (2 preceding siblings ...)
2024-03-26 16:56 ` Jonathan Cameron
@ 2024-03-26 17:49 ` Dan Williams
2024-03-26 17:53 ` Dan Williams
2024-03-29 23:48 ` [PATCH v3] " Dan Williams
5 siblings, 0 replies; 27+ messages in thread
From: Dan Williams @ 2024-03-26 17:49 UTC (permalink / raw)
To: Dan Williams, peterz, torvalds
Cc: Bjorn Helgaas, Ira Weiny, Jonathan Cameron, Jesse Brandeburg,
Ilpo Järvinen, Lukas Wunner, Jonathan Corbet, linux-pci,
linux-kernel, gregkh, linux-doc
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] cleanup: Add usage and style documentation
2024-03-25 22:57 ` [PATCH v2] " Dan Williams
` (3 preceding siblings ...)
2024-03-26 17:49 ` Dan Williams
@ 2024-03-26 17:53 ` Dan Williams
2024-03-29 23:48 ` [PATCH v3] " Dan Williams
5 siblings, 0 replies; 27+ messages in thread
From: Dan Williams @ 2024-03-26 17:53 UTC (permalink / raw)
To: Dan Williams, peterz, torvalds
Cc: Bjorn Helgaas, Ira Weiny, Jonathan Cameron, Jesse Brandeburg,
Ilpo Järvinen, Lukas Wunner, Jonathan Corbet, linux-pci,
linux-kernel, gregkh, linux-doc
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v3] cleanup: Add usage and style documentation
2024-03-25 22:57 ` [PATCH v2] " Dan Williams
` (4 preceding siblings ...)
2024-03-26 17:53 ` Dan Williams
@ 2024-03-29 23:48 ` Dan Williams
2024-03-30 20:23 ` Markus Elfring
2024-04-01 8:19 ` Tian, Kevin
5 siblings, 2 replies; 27+ messages in thread
From: Dan Williams @ 2024-03-29 23:48 UTC (permalink / raw)
To: peterz, torvalds
Cc: Bjorn Helgaas, Ira Weiny, Jesse Brandeburg, Ilpo Järvinen,
Lukas Wunner, Jonathan Corbet, Jonathan Cameron, gregkh,
linux-kernel, linux-pci
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: Jesse Brandeburg <jesse.brandeburg@intel.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
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()
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v3] cleanup: Add usage and style documentation
2024-03-29 23:48 ` [PATCH v3] " Dan Williams
@ 2024-03-30 20:23 ` Markus Elfring
2024-04-01 8:19 ` Tian, Kevin
1 sibling, 0 replies; 27+ messages in thread
From: Markus Elfring @ 2024-03-30 20:23 UTC (permalink / raw)
To: Dan Williams, Peter Zijlstra, Linus Torvalds, Jonathan Corbet,
linux-doc, linux-kernel, linux-pci, kernel-janitors
Cc: Bjorn Helgaas, Greg Kroah-Hartman, Ilpo Järvinen, Ira Weiny,
Jesse Brandeburg, Jonathan Cameron, Julia Lawall, Kevin Tian,
Lukas Wunner, Matthew Wilcox
> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH v3] cleanup: Add usage and style documentation
2024-03-29 23:48 ` [PATCH v3] " Dan Williams
2024-03-30 20:23 ` Markus Elfring
@ 2024-04-01 8:19 ` Tian, Kevin
2024-04-02 7:15 ` [v3] " Markus Elfring
1 sibling, 1 reply; 27+ messages in thread
From: Tian, Kevin @ 2024-04-01 8:19 UTC (permalink / raw)
To: Williams, Dan J, peterz, torvalds
Cc: Bjorn Helgaas, Weiny, Ira, Brandeburg, Jesse, Ilpo Järvinen,
Lukas Wunner, Jonathan Corbet, Jonathan Cameron, gregkh,
linux-kernel, linux-pci
> From: Dan Williams <dan.j.williams@intel.com>
> 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 <bhelgaas@google.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [v3] cleanup: Add usage and style documentation
2024-04-01 8:19 ` Tian, Kevin
@ 2024-04-02 7:15 ` Markus Elfring
0 siblings, 0 replies; 27+ messages in thread
From: Markus Elfring @ 2024-04-02 7:15 UTC (permalink / raw)
To: Kevin Tian, Dan Williams, linux-doc, linux-pci, kernel-janitors
Cc: Bjorn Helgaas, Greg Kroah-Hartman, Ilpo Järvinen, Ira Weiny,
Jesse Brandeburg, Jonathan Cameron, Julia Lawall, Lukas Wunner,
Matthew Wilcox, Peter Zijlstra, Linus Torvalds, Jonathan Corbet,
LKML
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Did you take any of my review comments into account for this patch version so far?
Regards,
Markus
^ permalink raw reply [flat|nested] 27+ messages in thread