linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] refine storing NULL
@ 2024-10-17 13:46 Wei Yang
  2024-10-17 13:46 ` [PATCH v2 1/5] maple_tree: print empty for an empty tree on mt_dump() Wei Yang
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Wei Yang @ 2024-10-17 13:46 UTC (permalink / raw)
  To: akpm, Liam.Howlett; +Cc: maple-tree, linux-mm, Wei Yang

The original thread[1] thoughts it is a problem in mas_new_root(). But after
discussion, this should be an improvement on storing NULL.

Patch 1/2 preparation for refine.

Patch 3 remove redundant check in mas_new_root().

Patch 4 refine mas_root_expand() to improve memory efficiency and remove
possible consecutive NULL slot.

Patch 5 adds a test for storing NULL.

[1]: https://lkml.kernel.org/r/20241015233909.23592-1-richard.weiyang@gmail.com

Wei Yang (5):
  maple_tree: print empty for an empty tree on mt_dump()
  maple_tree: the return value of mas_root_expand() is not used
  maple_tree: not necessary to check index/last again
  maple_tree: refine mas_root_expand() on storing NULL
  maple_tree: add a test checking storing null

 lib/maple_tree.c                 | 32 +++++++++++---
 tools/testing/radix-tree/maple.c | 73 ++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 6 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/5] maple_tree: print empty for an empty tree on mt_dump()
  2024-10-17 13:46 [PATCH v2 0/5] refine storing NULL Wei Yang
@ 2024-10-17 13:46 ` Wei Yang
  2024-10-17 13:46 ` [PATCH v2 2/5] maple_tree: the return value of mas_root_expand() is not used Wei Yang
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Wei Yang @ 2024-10-17 13:46 UTC (permalink / raw)
  To: akpm, Liam.Howlett
  Cc: maple-tree, linux-mm, Wei Yang, Liam R . Howlett,
	Sidhartha Kumar, Lorenzo Stoakes

Currently for an empty tree, it would print:

  maple_tree(0x7ffcd02c6ee0) flags 1, height 0 root (nil)
  0: (nil)

This is a little misleading.

Let's print (empty) for an empty tree.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
CC: Sidhartha Kumar <sidhartha.kumar@oracle.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 lib/maple_tree.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index db3de0b9aa9a..63a969d581f2 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -7204,10 +7204,12 @@ void mt_dump(const struct maple_tree *mt, enum mt_dump_format format)
 
 	pr_info("maple_tree(%p) flags %X, height %u root %p\n",
 		 mt, mt->ma_flags, mt_height(mt), entry);
-	if (!xa_is_node(entry))
-		mt_dump_entry(entry, 0, 0, 0, format);
-	else if (entry)
+	if (xa_is_node(entry))
 		mt_dump_node(mt, entry, 0, mt_node_max(entry), 0, format);
+	else if (entry)
+		mt_dump_entry(entry, 0, 0, 0, format);
+	else
+		pr_info("(empty)\n");
 }
 EXPORT_SYMBOL_GPL(mt_dump);
 
-- 
2.34.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 2/5] maple_tree: the return value of mas_root_expand() is not used
  2024-10-17 13:46 [PATCH v2 0/5] refine storing NULL Wei Yang
  2024-10-17 13:46 ` [PATCH v2 1/5] maple_tree: print empty for an empty tree on mt_dump() Wei Yang
@ 2024-10-17 13:46 ` Wei Yang
  2024-10-17 13:46 ` [PATCH v2 3/5] maple_tree: not necessary to check index/last again Wei Yang
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Wei Yang @ 2024-10-17 13:46 UTC (permalink / raw)
  To: akpm, Liam.Howlett
  Cc: maple-tree, linux-mm, Wei Yang, Liam R . Howlett,
	Sidhartha Kumar, Lorenzo Stoakes

No user of the return value now, just remove it.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
CC: Sidhartha Kumar <sidhartha.kumar@oracle.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 lib/maple_tree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index 63a969d581f2..ff9d972dba4d 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -3326,7 +3326,7 @@ static noinline_for_kasan void mas_commit_b_node(struct ma_wr_state *wr_mas,
  * @mas: The maple state
  * @entry: The entry to store into the tree
  */
-static inline int mas_root_expand(struct ma_state *mas, void *entry)
+static inline void mas_root_expand(struct ma_state *mas, void *entry)
 {
 	void *contents = mas_root_locked(mas);
 	enum maple_type type = maple_leaf_64;
@@ -3362,7 +3362,7 @@ static inline int mas_root_expand(struct ma_state *mas, void *entry)
 	ma_set_meta(node, maple_leaf_64, 0, slot);
 	/* swap the new root into the tree */
 	rcu_assign_pointer(mas->tree->ma_root, mte_mk_root(mas->node));
-	return slot;
+	return;
 }
 
 static inline void mas_store_root(struct ma_state *mas, void *entry)
-- 
2.34.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 3/5] maple_tree: not necessary to check index/last again
  2024-10-17 13:46 [PATCH v2 0/5] refine storing NULL Wei Yang
  2024-10-17 13:46 ` [PATCH v2 1/5] maple_tree: print empty for an empty tree on mt_dump() Wei Yang
  2024-10-17 13:46 ` [PATCH v2 2/5] maple_tree: the return value of mas_root_expand() is not used Wei Yang
@ 2024-10-17 13:46 ` Wei Yang
  2024-10-17 13:46 ` [PATCH v2 4/5] maple_tree: refine mas_root_expand() on storing NULL Wei Yang
  2024-10-17 13:46 ` [PATCH v2 5/5] maple_tree: add a test checking storing null Wei Yang
  4 siblings, 0 replies; 10+ messages in thread
From: Wei Yang @ 2024-10-17 13:46 UTC (permalink / raw)
  To: akpm, Liam.Howlett
  Cc: maple-tree, linux-mm, Wei Yang, Liam R . Howlett,
	Sidhartha Kumar, Lorenzo Stoakes

Before calling mas_new_root(), the range has been checked.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
CC: Sidhartha Kumar <sidhartha.kumar@oracle.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 lib/maple_tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index ff9d972dba4d..a90c29156fe2 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -3591,7 +3591,7 @@ static inline void mas_new_root(struct ma_state *mas, void *entry)
 	void __rcu **slots;
 	unsigned long *pivots;
 
-	if (!entry && !mas->index && mas->last == ULONG_MAX) {
+	if (!entry) {
 		mas->depth = 0;
 		mas_set_height(mas);
 		rcu_assign_pointer(mas->tree->ma_root, entry);
-- 
2.34.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 4/5] maple_tree: refine mas_root_expand() on storing NULL
  2024-10-17 13:46 [PATCH v2 0/5] refine storing NULL Wei Yang
                   ` (2 preceding siblings ...)
  2024-10-17 13:46 ` [PATCH v2 3/5] maple_tree: not necessary to check index/last again Wei Yang
@ 2024-10-17 13:46 ` Wei Yang
  2024-10-17 14:10   ` Liam R. Howlett
  2024-10-17 13:46 ` [PATCH v2 5/5] maple_tree: add a test checking storing null Wei Yang
  4 siblings, 1 reply; 10+ messages in thread
From: Wei Yang @ 2024-10-17 13:46 UTC (permalink / raw)
  To: akpm, Liam.Howlett
  Cc: maple-tree, linux-mm, Wei Yang, Liam R . Howlett,
	Sidhartha Kumar, Lorenzo Stoakes

Currently, when storing NULL on mas_root_expand(), the behavior could be
improved.

For example possible cases are:

  * store NULL at any range result a new node
  * store NULL at range [m, n] where m > 0 to a single entry tree result
    a new node with range [m, n] set to NULL
  * store NULL at range [m, n] where m > 0 to an empty tree result
    consecutive NULL slot

This patch tries to improve in:

  * memory efficient by setting to empty tree instead of using a node
  * remove the possibility of consecutive NULL slot which will prohibit
    extended null in later operation

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
CC: Sidhartha Kumar <sidhartha.kumar@oracle.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 lib/maple_tree.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index a90c29156fe2..15d2124acc36 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -3335,6 +3335,24 @@ static inline void mas_root_expand(struct ma_state *mas, void *entry)
 	unsigned long *pivots;
 	int slot = 0;
 
+	if (!entry) {
+		/*
+		 * We come here in two cases:
+		 * 1. This is an empty tree
+		 * 2. This is a single entry tree with range [0, 0]
+		 *
+		 * If this is an empty tree, the result should still be an
+		 * empty tree no matter what the range is.
+		 *
+		 * If this is a single entry tree, we should set it to an
+		 * empty tree if the range cover [0, 0]. Otherwise, we don't
+		 * need to change it.
+		 */
+		if (!mas->index && contents)
+			rcu_assign_pointer(mas->tree->ma_root, NULL);
+		return;
+	}
+
 	node = mas_pop_node(mas);
 	pivots = ma_pivots(node, type);
 	slots = ma_slots(node, type);
-- 
2.34.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 5/5] maple_tree: add a test checking storing null
  2024-10-17 13:46 [PATCH v2 0/5] refine storing NULL Wei Yang
                   ` (3 preceding siblings ...)
  2024-10-17 13:46 ` [PATCH v2 4/5] maple_tree: refine mas_root_expand() on storing NULL Wei Yang
@ 2024-10-17 13:46 ` Wei Yang
  2024-10-17 14:12   ` Liam R. Howlett
  4 siblings, 1 reply; 10+ messages in thread
From: Wei Yang @ 2024-10-17 13:46 UTC (permalink / raw)
  To: akpm, Liam.Howlett
  Cc: maple-tree, linux-mm, Wei Yang, Liam R . Howlett,
	Sidhartha Kumar, Lorenzo Stoakes

Add a test to assert that, when storing null to am empty tree or a
single entry tree it will not result into:

  * a root node with range [0, ULONG_MAX] set to NULL
  * a root node with consecutive slot set to NULL

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
CC: Sidhartha Kumar <sidhartha.kumar@oracle.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 tools/testing/radix-tree/maple.c | 73 ++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/tools/testing/radix-tree/maple.c b/tools/testing/radix-tree/maple.c
index 5fde09999be4..fbb08f151b01 100644
--- a/tools/testing/radix-tree/maple.c
+++ b/tools/testing/radix-tree/maple.c
@@ -35867,6 +35867,75 @@ static noinline void __init check_locky(struct maple_tree *mt)
 	mt_clear_in_rcu(mt);
 }
 
+static noinline void __init check_store_null(struct maple_tree *mt)
+{
+	MA_STATE(ms, mt, 0, ULONG_MAX);
+
+	mt_set_non_kernel(10);
+
+	/*
+	 * Store NULL at range [0, ULONG_MAX] to an empty tree should result
+	 * in an empty tree
+	 */
+	mas_store(&ms, NULL);
+	MT_BUG_ON(mt, !mtree_empty(mt));
+	mtree_destroy(mt);
+
+	/*
+	 * Store NULL at any range to an empty tree should result in an empty
+	 * tree
+	 */
+	mas_set_range(&ms, 3, 10);
+	mas_store(&ms, NULL);
+	MT_BUG_ON(mt, !mtree_empty(mt));
+	mtree_destroy(mt);
+
+	/*
+	 * Store NULL at range [0, ULONG_MAX] to a single entry tree should
+	 * result in an empty tree
+	 */
+	mas_set(&ms, 0);
+	mas_store(&ms, &ms);
+	mas_set_range(&ms, 0, ULONG_MAX);
+	mas_store(&ms, NULL);
+	MT_BUG_ON(mt, !mtree_empty(mt));
+	mtree_destroy(mt);
+
+	/*
+	 * Store NULL at range [0, n] to a single entry tree should
+	 * result in an empty tree
+	 */
+	mas_set(&ms, 0);
+	mas_store(&ms, &ms);
+	mas_set_range(&ms, 0, 5);
+	mas_store(&ms, NULL);
+	MT_BUG_ON(mt, !mtree_empty(mt));
+	mtree_destroy(mt);
+
+	/*
+	 * Store NULL at range [m, n] where m > 0 to a single entry tree
+	 * should still be a single entry tree
+	 */
+	mas_set(&ms, 0);
+	mas_store(&ms, &ms);
+	mas_set_range(&ms, 2, 5);
+	mas_store(&ms, NULL);
+	MT_BUG_ON(mt, mtree_empty(mt));
+	MT_BUG_ON(mt, xa_is_node(mt->ma_root));
+	mtree_destroy(mt);
+
+	/*
+	 * Store NULL at range [0, ULONG_MAX] to a tree with node should
+	 * result in an empty tree
+	 */
+	mas_set_range(&ms, 1, 3);
+	mas_store(&ms, &ms);
+	mas_set_range(&ms, 0, ULONG_MAX);
+	mas_store(&ms, NULL);
+	MT_BUG_ON(mt, !mtree_empty(mt));
+	mtree_destroy(mt);
+}
+
 /*
  * Compares two nodes except for the addresses stored in the nodes.
  * Returns zero if they are the same, otherwise returns non-zero.
@@ -36344,6 +36413,10 @@ void farmer_tests(void)
 	node->parent = ma_parent_ptr(node);
 	ma_free_rcu(node);
 
+	mt_init_flags(&tree, MT_FLAGS_ALLOC_RANGE);
+	check_store_null(&tree);
+	mtree_destroy(&tree);
+
 	/* Check things that will make lockdep angry */
 	mt_init_flags(&tree, MT_FLAGS_ALLOC_RANGE);
 	check_locky(&tree);
-- 
2.34.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/5] maple_tree: refine mas_root_expand() on storing NULL
  2024-10-17 13:46 ` [PATCH v2 4/5] maple_tree: refine mas_root_expand() on storing NULL Wei Yang
@ 2024-10-17 14:10   ` Liam R. Howlett
  2024-10-17 22:21     ` Wei Yang
  0 siblings, 1 reply; 10+ messages in thread
From: Liam R. Howlett @ 2024-10-17 14:10 UTC (permalink / raw)
  To: Wei Yang; +Cc: akpm, maple-tree, linux-mm, Sidhartha Kumar, Lorenzo Stoakes

* Wei Yang <richard.weiyang@gmail.com> [241017 09:46]:
> Currently, when storing NULL on mas_root_expand(), the behavior could be
> improved.
> 
> For example possible cases are:
> 
>   * store NULL at any range result a new node
>   * store NULL at range [m, n] where m > 0 to a single entry tree result
>     a new node with range [m, n] set to NULL
>   * store NULL at range [m, n] where m > 0 to an empty tree result
>     consecutive NULL slot
> 
> This patch tries to improve in:
> 
>   * memory efficient by setting to empty tree instead of using a node
>   * remove the possibility of consecutive NULL slot which will prohibit
>     extended null in later operation
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
> CC: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  lib/maple_tree.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> index a90c29156fe2..15d2124acc36 100644
> --- a/lib/maple_tree.c
> +++ b/lib/maple_tree.c
> @@ -3335,6 +3335,24 @@ static inline void mas_root_expand(struct ma_state *mas, void *entry)
>  	unsigned long *pivots;
>  	int slot = 0;
>  
> +	if (!entry) {
> +		/*
> +		 * We come here in two cases:
> +		 * 1. This is an empty tree
> +		 * 2. This is a single entry tree with range [0, 0]
> +		 *
> +		 * If this is an empty tree, the result should still be an
> +		 * empty tree no matter what the range is.
> +		 *
> +		 * If this is a single entry tree, we should set it to an
> +		 * empty tree if the range cover [0, 0]. Otherwise, we don't
> +		 * need to change it.
> +		 */
> +		if (!mas->index && contents)
> +			rcu_assign_pointer(mas->tree->ma_root, NULL);
> +		return;
> +	}
> +

This fix should be done in mas_store_root(), which will probably reduce
or remove your lengthy comment.

mas_root_expand() should... expand the root, and this isn't what is
happening now.

>  	node = mas_pop_node(mas);
>  	pivots = ma_pivots(node, type);
>  	slots = ma_slots(node, type);
> -- 
> 2.34.1
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 5/5] maple_tree: add a test checking storing null
  2024-10-17 13:46 ` [PATCH v2 5/5] maple_tree: add a test checking storing null Wei Yang
@ 2024-10-17 14:12   ` Liam R. Howlett
  2024-10-17 22:24     ` Wei Yang
  0 siblings, 1 reply; 10+ messages in thread
From: Liam R. Howlett @ 2024-10-17 14:12 UTC (permalink / raw)
  To: Wei Yang; +Cc: akpm, maple-tree, linux-mm, Sidhartha Kumar, Lorenzo Stoakes

* Wei Yang <richard.weiyang@gmail.com> [241017 09:46]:
> Add a test to assert that, when storing null to am empty tree or a
> single entry tree it will not result into:
> 
>   * a root node with range [0, ULONG_MAX] set to NULL
>   * a root node with consecutive slot set to NULL

Is there any reason this isn't in lib/test_maple_tree.c ?

Does it need internal interfaces at all?

> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
> CC: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  tools/testing/radix-tree/maple.c | 73 ++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/tools/testing/radix-tree/maple.c b/tools/testing/radix-tree/maple.c
> index 5fde09999be4..fbb08f151b01 100644
> --- a/tools/testing/radix-tree/maple.c
> +++ b/tools/testing/radix-tree/maple.c
> @@ -35867,6 +35867,75 @@ static noinline void __init check_locky(struct maple_tree *mt)
>  	mt_clear_in_rcu(mt);
>  }
>  
> +static noinline void __init check_store_null(struct maple_tree *mt)
> +{
> +	MA_STATE(ms, mt, 0, ULONG_MAX);
> +
> +	mt_set_non_kernel(10);
> +
> +	/*
> +	 * Store NULL at range [0, ULONG_MAX] to an empty tree should result
> +	 * in an empty tree
> +	 */
> +	mas_store(&ms, NULL);
> +	MT_BUG_ON(mt, !mtree_empty(mt));
> +	mtree_destroy(mt);
> +
> +	/*
> +	 * Store NULL at any range to an empty tree should result in an empty
> +	 * tree
> +	 */
> +	mas_set_range(&ms, 3, 10);
> +	mas_store(&ms, NULL);
> +	MT_BUG_ON(mt, !mtree_empty(mt));
> +	mtree_destroy(mt);
> +
> +	/*
> +	 * Store NULL at range [0, ULONG_MAX] to a single entry tree should
> +	 * result in an empty tree
> +	 */
> +	mas_set(&ms, 0);
> +	mas_store(&ms, &ms);
> +	mas_set_range(&ms, 0, ULONG_MAX);
> +	mas_store(&ms, NULL);
> +	MT_BUG_ON(mt, !mtree_empty(mt));
> +	mtree_destroy(mt);
> +
> +	/*
> +	 * Store NULL at range [0, n] to a single entry tree should
> +	 * result in an empty tree
> +	 */
> +	mas_set(&ms, 0);
> +	mas_store(&ms, &ms);
> +	mas_set_range(&ms, 0, 5);
> +	mas_store(&ms, NULL);
> +	MT_BUG_ON(mt, !mtree_empty(mt));
> +	mtree_destroy(mt);
> +
> +	/*
> +	 * Store NULL at range [m, n] where m > 0 to a single entry tree
> +	 * should still be a single entry tree
> +	 */
> +	mas_set(&ms, 0);
> +	mas_store(&ms, &ms);
> +	mas_set_range(&ms, 2, 5);
> +	mas_store(&ms, NULL);
> +	MT_BUG_ON(mt, mtree_empty(mt));
> +	MT_BUG_ON(mt, xa_is_node(mt->ma_root));
> +	mtree_destroy(mt);
> +
> +	/*
> +	 * Store NULL at range [0, ULONG_MAX] to a tree with node should
> +	 * result in an empty tree
> +	 */
> +	mas_set_range(&ms, 1, 3);
> +	mas_store(&ms, &ms);
> +	mas_set_range(&ms, 0, ULONG_MAX);
> +	mas_store(&ms, NULL);
> +	MT_BUG_ON(mt, !mtree_empty(mt));
> +	mtree_destroy(mt);
> +}
> +
>  /*
>   * Compares two nodes except for the addresses stored in the nodes.
>   * Returns zero if they are the same, otherwise returns non-zero.
> @@ -36344,6 +36413,10 @@ void farmer_tests(void)
>  	node->parent = ma_parent_ptr(node);
>  	ma_free_rcu(node);
>  
> +	mt_init_flags(&tree, MT_FLAGS_ALLOC_RANGE);
> +	check_store_null(&tree);
> +	mtree_destroy(&tree);
> +
>  	/* Check things that will make lockdep angry */
>  	mt_init_flags(&tree, MT_FLAGS_ALLOC_RANGE);
>  	check_locky(&tree);
> -- 
> 2.34.1
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 4/5] maple_tree: refine mas_root_expand() on storing NULL
  2024-10-17 14:10   ` Liam R. Howlett
@ 2024-10-17 22:21     ` Wei Yang
  0 siblings, 0 replies; 10+ messages in thread
From: Wei Yang @ 2024-10-17 22:21 UTC (permalink / raw)
  To: Liam R. Howlett, Wei Yang, akpm, maple-tree, linux-mm,
	Sidhartha Kumar, Lorenzo Stoakes

On Thu, Oct 17, 2024 at 10:10:20AM -0400, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [241017 09:46]:
>> Currently, when storing NULL on mas_root_expand(), the behavior could be
>> improved.
>> 
>> For example possible cases are:
>> 
>>   * store NULL at any range result a new node
>>   * store NULL at range [m, n] where m > 0 to a single entry tree result
>>     a new node with range [m, n] set to NULL
>>   * store NULL at range [m, n] where m > 0 to an empty tree result
>>     consecutive NULL slot
>> 
>> This patch tries to improve in:
>> 
>>   * memory efficient by setting to empty tree instead of using a node
>>   * remove the possibility of consecutive NULL slot which will prohibit
>>     extended null in later operation
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
>> CC: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> ---
>>  lib/maple_tree.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>> 
>> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
>> index a90c29156fe2..15d2124acc36 100644
>> --- a/lib/maple_tree.c
>> +++ b/lib/maple_tree.c
>> @@ -3335,6 +3335,24 @@ static inline void mas_root_expand(struct ma_state *mas, void *entry)
>>  	unsigned long *pivots;
>>  	int slot = 0;
>>  
>> +	if (!entry) {
>> +		/*
>> +		 * We come here in two cases:
>> +		 * 1. This is an empty tree
>> +		 * 2. This is a single entry tree with range [0, 0]
>> +		 *
>> +		 * If this is an empty tree, the result should still be an
>> +		 * empty tree no matter what the range is.
>> +		 *
>> +		 * If this is a single entry tree, we should set it to an
>> +		 * empty tree if the range cover [0, 0]. Otherwise, we don't
>> +		 * need to change it.
>> +		 */
>> +		if (!mas->index && contents)
>> +			rcu_assign_pointer(mas->tree->ma_root, NULL);
>> +		return;
>> +	}
>> +
>
>This fix should be done in mas_store_root(), which will probably reduce
>or remove your lengthy comment.
>
>mas_root_expand() should... expand the root, and this isn't what is
>happening now.
>

Right, will move it.

Thanks

>>  	node = mas_pop_node(mas);
>>  	pivots = ma_pivots(node, type);
>>  	slots = ma_slots(node, type);
>> -- 
>> 2.34.1
>> 

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 5/5] maple_tree: add a test checking storing null
  2024-10-17 14:12   ` Liam R. Howlett
@ 2024-10-17 22:24     ` Wei Yang
  0 siblings, 0 replies; 10+ messages in thread
From: Wei Yang @ 2024-10-17 22:24 UTC (permalink / raw)
  To: Liam R. Howlett, Wei Yang, akpm, maple-tree, linux-mm,
	Sidhartha Kumar, Lorenzo Stoakes

On Thu, Oct 17, 2024 at 10:12:22AM -0400, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [241017 09:46]:
>> Add a test to assert that, when storing null to am empty tree or a
>> single entry tree it will not result into:
>> 
>>   * a root node with range [0, ULONG_MAX] set to NULL
>>   * a root node with consecutive slot set to NULL
>
>Is there any reason this isn't in lib/test_maple_tree.c ?
>
>Does it need internal interfaces at all?

Sorry, I didn't notice this requirement.

Will move into test_maple_tree.c

>
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
>> CC: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> ---
>>  tools/testing/radix-tree/maple.c | 73 ++++++++++++++++++++++++++++++++
>>  1 file changed, 73 insertions(+)
>> 
>> diff --git a/tools/testing/radix-tree/maple.c b/tools/testing/radix-tree/maple.c
>> index 5fde09999be4..fbb08f151b01 100644
>> --- a/tools/testing/radix-tree/maple.c
>> +++ b/tools/testing/radix-tree/maple.c
>> @@ -35867,6 +35867,75 @@ static noinline void __init check_locky(struct maple_tree *mt)
>>  	mt_clear_in_rcu(mt);
>>  }
>>  
>> +static noinline void __init check_store_null(struct maple_tree *mt)
>> +{
>> +	MA_STATE(ms, mt, 0, ULONG_MAX);
>> +
>> +	mt_set_non_kernel(10);
>> +
>> +	/*
>> +	 * Store NULL at range [0, ULONG_MAX] to an empty tree should result
>> +	 * in an empty tree
>> +	 */
>> +	mas_store(&ms, NULL);
>> +	MT_BUG_ON(mt, !mtree_empty(mt));
>> +	mtree_destroy(mt);
>> +
>> +	/*
>> +	 * Store NULL at any range to an empty tree should result in an empty
>> +	 * tree
>> +	 */
>> +	mas_set_range(&ms, 3, 10);
>> +	mas_store(&ms, NULL);
>> +	MT_BUG_ON(mt, !mtree_empty(mt));
>> +	mtree_destroy(mt);
>> +
>> +	/*
>> +	 * Store NULL at range [0, ULONG_MAX] to a single entry tree should
>> +	 * result in an empty tree
>> +	 */
>> +	mas_set(&ms, 0);
>> +	mas_store(&ms, &ms);
>> +	mas_set_range(&ms, 0, ULONG_MAX);
>> +	mas_store(&ms, NULL);
>> +	MT_BUG_ON(mt, !mtree_empty(mt));
>> +	mtree_destroy(mt);
>> +
>> +	/*
>> +	 * Store NULL at range [0, n] to a single entry tree should
>> +	 * result in an empty tree
>> +	 */
>> +	mas_set(&ms, 0);
>> +	mas_store(&ms, &ms);
>> +	mas_set_range(&ms, 0, 5);
>> +	mas_store(&ms, NULL);
>> +	MT_BUG_ON(mt, !mtree_empty(mt));
>> +	mtree_destroy(mt);
>> +
>> +	/*
>> +	 * Store NULL at range [m, n] where m > 0 to a single entry tree
>> +	 * should still be a single entry tree
>> +	 */
>> +	mas_set(&ms, 0);
>> +	mas_store(&ms, &ms);
>> +	mas_set_range(&ms, 2, 5);
>> +	mas_store(&ms, NULL);
>> +	MT_BUG_ON(mt, mtree_empty(mt));
>> +	MT_BUG_ON(mt, xa_is_node(mt->ma_root));
>> +	mtree_destroy(mt);
>> +
>> +	/*
>> +	 * Store NULL at range [0, ULONG_MAX] to a tree with node should
>> +	 * result in an empty tree
>> +	 */
>> +	mas_set_range(&ms, 1, 3);
>> +	mas_store(&ms, &ms);
>> +	mas_set_range(&ms, 0, ULONG_MAX);
>> +	mas_store(&ms, NULL);
>> +	MT_BUG_ON(mt, !mtree_empty(mt));
>> +	mtree_destroy(mt);
>> +}
>> +
>>  /*
>>   * Compares two nodes except for the addresses stored in the nodes.
>>   * Returns zero if they are the same, otherwise returns non-zero.
>> @@ -36344,6 +36413,10 @@ void farmer_tests(void)
>>  	node->parent = ma_parent_ptr(node);
>>  	ma_free_rcu(node);
>>  
>> +	mt_init_flags(&tree, MT_FLAGS_ALLOC_RANGE);
>> +	check_store_null(&tree);
>> +	mtree_destroy(&tree);
>> +
>>  	/* Check things that will make lockdep angry */
>>  	mt_init_flags(&tree, MT_FLAGS_ALLOC_RANGE);
>>  	check_locky(&tree);
>> -- 
>> 2.34.1
>> 

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-10-17 22:24 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-17 13:46 [PATCH v2 0/5] refine storing NULL Wei Yang
2024-10-17 13:46 ` [PATCH v2 1/5] maple_tree: print empty for an empty tree on mt_dump() Wei Yang
2024-10-17 13:46 ` [PATCH v2 2/5] maple_tree: the return value of mas_root_expand() is not used Wei Yang
2024-10-17 13:46 ` [PATCH v2 3/5] maple_tree: not necessary to check index/last again Wei Yang
2024-10-17 13:46 ` [PATCH v2 4/5] maple_tree: refine mas_root_expand() on storing NULL Wei Yang
2024-10-17 14:10   ` Liam R. Howlett
2024-10-17 22:21     ` Wei Yang
2024-10-17 13:46 ` [PATCH v2 5/5] maple_tree: add a test checking storing null Wei Yang
2024-10-17 14:12   ` Liam R. Howlett
2024-10-17 22:24     ` Wei Yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox