Message-ID: <20230524130319.mc7yl47465yi343n@revolver>
Date:   Wed, 24 May 2023 09:03:19 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Matthew Wilcox <willy@...radead.org>, linux-mm@...ck.org,
        Shanker Donthineni <sdonthineni@...dia.com>
Subject: Re: [PATCH v2] maple_tree: Fix a few documentation issues

* Thomas Gleixner <tglx@...utronix.de> [230523 16:51]:
> The documentation of mt_next() claims that it starts the search at the
> provided index. That's incorrect as it starts the search after the provided
> index.
> 
> The documentation of mt_find() is slightly confusing. "Handles locking" is
> not really helpful as it does not explain how the "locking" works. Also the
> documentation of index talks about a range, while in reality the index
> is updated on a succesful search to the index of the found entry plus one.
> 
> Fix similar issues for mt_find_after() and mt_prev().
> 
> Reword the confusing "Note: Will not return the zero entry." comment on
> mt_for_each() and document @__index correctly.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>

Reviewed-by: Liam R. Howlett <Liam.Howlett@...cle.com>

> ---
> V2: Address review feedback. Add pointer to documentation, reword the
>     zero entry and the index explanations. - Liam
> ---
>  include/linux/maple_tree.h |    5 +++--
>  lib/maple_tree.c           |   26 +++++++++++++++++++++-----
>  2 files changed, 24 insertions(+), 7 deletions(-)
> 
> --- a/include/linux/maple_tree.h
> +++ b/include/linux/maple_tree.h
> @@ -659,10 +659,11 @@ void *mt_next(struct maple_tree *mt, uns
>   * mt_for_each - Iterate over each entry starting at index until max.
>   * @__tree: The Maple Tree
>   * @__entry: The current entry
> - * @__index: The index to update to track the location in the tree
> + * @__index: The index to start the search from. Subsequently used as iterator.
>   * @__max: The maximum limit for @index
>   *
> - * Note: Will not return the zero entry.
> + * This iterator skips all entries, which resolve to a NULL pointer,
> + * e.g. entries which has been reserved with XA_ZERO_ENTRY.
>   */
>  #define mt_for_each(__tree, __entry, __index, __max) \
>  	for (__entry = mt_find(__tree, &(__index), __max); \
> --- a/lib/maple_tree.c
> +++ b/lib/maple_tree.c
> @@ -5947,7 +5947,11 @@ EXPORT_SYMBOL_GPL(mas_next);
>   * @index: The start index
>   * @max: The maximum index to check
>   *
> - * Return: The entry at @index or higher, or %NULL if nothing is found.
> + * Takes RCU read lock internally to protect the search, which does not
> + * protect the returned pointer after dropping RCU read lock.
> + * See also: Documentation/core-api/maple_tree.rst
> + *
> + * Return: The entry higher than @index or %NULL if nothing is found.
>   */
>  void *mt_next(struct maple_tree *mt, unsigned long index, unsigned long max)
>  {
> @@ -6012,7 +6016,11 @@ EXPORT_SYMBOL_GPL(mas_prev);
>   * @index: The start index
>   * @min: The minimum index to check
>   *
> - * Return: The entry at @index or lower, or %NULL if nothing is found.
> + * Takes RCU read lock internally to protect the search, which does not
> + * protect the returned pointer after dropping RCU read lock.
> + * See also: Documentation/core-api/maple_tree.rst
> + *
> + * Return: The entry before @index or %NULL if nothing is found.
>   */
>  void *mt_prev(struct maple_tree *mt, unsigned long index, unsigned long min)
>  {
> @@ -6487,9 +6495,15 @@ EXPORT_SYMBOL(mtree_destroy);
>   * mt_find() - Search from the start up until an entry is found.
>   * @mt: The maple tree
>   * @index: Pointer which contains the start location of the search
> - * @max: The maximum value to check
> + * @max: The maximum value of the search range
>   *
> - * Handles locking.  @index will be incremented to one beyond the range.
> + * Takes RCU read lock internally to protect the search, which does not
> + * protect the returned pointer after dropping RCU read lock.
> + * See also: Documentation/core-api/maple_tree.rst
> + *
> + * In case that an entry is found @index is updated to point to the next
> + * possible entry independent whether the found entry is occupying a
> + * single index or a range if indices.
>   *
>   * Return: The entry at or after the @index or %NULL
>   */
> @@ -6548,7 +6562,9 @@ EXPORT_SYMBOL(mt_find);
>   * @index: Pointer which contains the start location of the search
>   * @max: The maximum value to check
>   *
> - * Handles locking, detects wrapping on index == 0
> + * Same as mt_find() except that it checks @index for 0 before
> + * searching. If @index == 0, the search is aborted. This covers a wrap
> + * around of @index to 0 in an iterator loop.
>   *
>   * Return: The entry at or after the @index or %NULL
>   */

Powered by blists - more mailing lists