[v3] cleanup: Add usage and style documentation

Commit Message

Dan Williams March 29, 2024, 11:48 p.m. UTC

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 <[e-mail]>
Cc: Ira Weiny <[e-mail]>
Cc: Jesse Brandeburg <[e-mail]>
Cc: Ilpo Järvinen <[e-mail]>
Cc: Lukas Wunner <[e-mail]>
Cc: Peter Zijlstra <[e-mail]>
Cc: Jonathan Corbet <[e-mail]>
Reviewed-by: Jonathan Cameron <[e-mail]>
Signed-off-by: Dan Williams <[e-mail]>
---
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

Comments

Markus Elfring March 30, 2024, 8:23 p.m. UTC | #1

> 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

Tian, Kevin April 1, 2024, 8:19 a.m. UTC | #2

> From: Dan Williams <[e-mail]>
> 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 <[e-mail]>
> Cc: Ira Weiny <[e-mail]>
> Cc: Jesse Brandeburg <[e-mail]>
> Cc: Ilpo Järvinen <[e-mail]>
> Cc: Lukas Wunner <[e-mail]>
> Cc: Peter Zijlstra <[e-mail]>
> Cc: Jonathan Corbet <[e-mail]>
> Reviewed-by: Jonathan Cameron <[e-mail]>
> Signed-off-by: Dan Williams <[e-mail]>

Reviewed-by: Kevin Tian <[e-mail]>

Markus Elfring April 2, 2024, 7:15 a.m. UTC | #3

> Reviewed-by: Kevin Tian <[e-mail]>

Did you take any of my review comments into account for this patch version so far?

Regards,
Markus

Jonathan Cameron Aug. 2, 2024, 1:58 p.m. UTC | #4

On Fri, 29 Mar 2024 16:48:48 -0700
Dan Williams <[e-mail]> 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 <[e-mail]>
> Cc: Ira Weiny <[e-mail]>
> Cc: Jesse Brandeburg <[e-mail]>
> Cc: Ilpo Järvinen <[e-mail]>
> Cc: Lukas Wunner <[e-mail]>
> Cc: Peter Zijlstra <[e-mail]>
> Cc: Jonathan Corbet <[e-mail]>
> Reviewed-by: Jonathan Cameron <[e-mail]>
> Signed-off-by: Dan Williams <[e-mail]>
> ---
> 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.

Would be good to either get more review, or this applied.

Currently I'm pointing people at the email. Would much rather
point them at the upstream docs!

Jon, would you consider picking this 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()
>

Peter Zijlstra Aug. 2, 2024, 3:07 p.m. UTC | #5

On Fri, Aug 02, 2024 at 02:58:56PM +0100, Jonathan Cameron wrote:
> On Fri, 29 Mar 2024 16:48:48 -0700
> Dan Williams <[e-mail]> 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 <[e-mail]>
> > Cc: Ira Weiny <[e-mail]>
> > Cc: Jesse Brandeburg <[e-mail]>
> > Cc: Ilpo Järvinen <[e-mail]>
> > Cc: Lukas Wunner <[e-mail]>
> > Cc: Peter Zijlstra <[e-mail]>
> > Cc: Jonathan Corbet <[e-mail]>
> > Reviewed-by: Jonathan Cameron <[e-mail]>
> > Signed-off-by: Dan Williams <[e-mail]>
> > ---
> > 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.
> 
> Would be good to either get more review, or this applied.
> 
> Currently I'm pointing people at the email. Would much rather
> point them at the upstream docs!
> 
> Jon, would you consider picking this up?

I suppose I can stick it in tip/locking/core, I think that's how we all
ended up with this nonsense anyway :-)

Patch

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()