[PATCH 1/2] maple_tree: remove maple_big_node.parent

[PATCH 1/2] maple_tree: remove maple_big_node.parent

[Wei Yang]

The member parent of maple_big_node is never used.

Let's remove it.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
---
 lib/maple_tree.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index 994dd4c8c051..d8f10773e451 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -120,7 +120,6 @@ static const unsigned char mt_min_slots[] = {
 #define MAPLE_BIG_NODE_GAPS	(MAPLE_ARANGE64_SLOTS * 2 + 1)
 
 struct maple_big_node {
-	struct maple_pnode *parent;
 	unsigned long pivot[MAPLE_BIG_NODE_SLOTS - 1];
 	union {
 		struct maple_enode *slot[MAPLE_BIG_NODE_SLOTS];
-- 
2.34.1

[PATCH 2/2] maple_tree: memset maple_big_node as a whole

[Wei Yang]

In maple_big_node, we define slot and padding/gap in a union. And based
on current definition of MAPLE_BIG_NODE_SLOTS/GAPS, padding is always
less then slot and part of the gap is overlapped by slot.

For example on 64bit system:

  MAPLE_BIG_NODE_SLOT is 34
  MAPLE_BIG_NODE_GAP  is 21

With this knowledge, current code actually clear the whole
maple_big_node and even clear some space twice.

Instead of clear maple_big_node each field separately, let's clear it in
one memset.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Liam R. Howlett <Liam.Howlett@Oracle.com>

---
Liam:

This looks obvious, so I just run the ./maple test to see it doesn't
break anything.

Do you think I need to add a benchmark and run a perf for this kind of
change?
---
 lib/maple_tree.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index d8f10773e451..911c5e04e634 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -3134,10 +3134,7 @@ static inline void mast_fill_bnode(struct maple_subtree_state *mast,
 	bool cp = true;
 	unsigned char split;
 
-	memset(mast->bn->gap, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->gap));
-	memset(mast->bn->slot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->slot));
-	memset(mast->bn->pivot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->pivot));
-	mast->bn->b_end = 0;
+	memset(mast->bn, 0, sizeof(struct maple_big_node));
 
 	if (mte_is_root(mas->node)) {
 		cp = false;
-- 
2.34.1

Re: [PATCH 2/2] maple_tree: memset maple_big_node as a whole

[Liam R. Howlett]

* Wei Yang <richard.weiyang@gmail.com> [240907 04:50]:
> In maple_big_node, we define slot and padding/gap in a union. And based
> on current definition of MAPLE_BIG_NODE_SLOTS/GAPS, padding is always
> less then slot and part of the gap is overlapped by slot.
       ^^^^- than

> 
> For example on 64bit system:
> 
>   MAPLE_BIG_NODE_SLOT is 34
>   MAPLE_BIG_NODE_GAP  is 21
> 
> With this knowledge, current code actually clear the whole
> maple_big_node and even clear some space twice.
> 
> Instead of clear maple_big_node each field separately, let's clear it in
> one memset.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
> 
> ---
> Liam:
> 
> This looks obvious, so I just run the ./maple test to see it doesn't
> break anything.
> 

The big node also includes the type, which isn't cleared.  However it is
unconditionally set below, so your change log is not correct in the
statement that it is fully cleared, but the code will work and it is
worth fixing what you have found.

If you are sending more than one patch, it is better to make a
cover letter to explain your series.

It is probably worth re-spinning the series to fix the comment, but
these changes look good.  I'll have a closer look later.

> Do you think I need to add a benchmark and run a perf for this kind of
> change?

No, you are doing less work so this should be better as long as it is
correct.  Zeroing can be expensive on some archs so this is good.

> ---
>  lib/maple_tree.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> index d8f10773e451..911c5e04e634 100644
> --- a/lib/maple_tree.c
> +++ b/lib/maple_tree.c
> @@ -3134,10 +3134,7 @@ static inline void mast_fill_bnode(struct maple_subtree_state *mast,
>  	bool cp = true;
>  	unsigned char split;
>  
> -	memset(mast->bn->gap, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->gap));
> -	memset(mast->bn->slot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->slot));
> -	memset(mast->bn->pivot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->pivot));
> -	mast->bn->b_end = 0;
> +	memset(mast->bn, 0, sizeof(struct maple_big_node));
>  
>  	if (mte_is_root(mas->node)) {
>  		cp = false;
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] maple_tree: memset maple_big_node as a whole

[Wei Yang]

On Sat, Sep 07, 2024 at 09:29:01AM -0400, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [240907 04:50]:
>> In maple_big_node, we define slot and padding/gap in a union. And based
>> on current definition of MAPLE_BIG_NODE_SLOTS/GAPS, padding is always
>> less then slot and part of the gap is overlapped by slot.
>       ^^^^- than
>
>> 
>> For example on 64bit system:
>> 
>>   MAPLE_BIG_NODE_SLOT is 34
>>   MAPLE_BIG_NODE_GAP  is 21
>> 
>> With this knowledge, current code actually clear the whole
>> maple_big_node and even clear some space twice.
>> 
>> Instead of clear maple_big_node each field separately, let's clear it in
>> one memset.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
>> 
>> ---
>> Liam:
>> 
>> This looks obvious, so I just run the ./maple test to see it doesn't
>> break anything.
>> 
>
>The big node also includes the type, which isn't cleared.  However it is
>unconditionally set below, so your change log is not correct in the
>statement that it is fully cleared, but the code will work and it is
>worth fixing what you have found.
>

I will mention this next time.

>If you are sending more than one patch, it is better to make a
>cover letter to explain your series.
>

Ok, I will add a cover letter next time.

>It is probably worth re-spinning the series to fix the comment, but
>these changes look good.  I'll have a closer look later.
>

Thank,

I will re-arrange the change log and add a cover letter for v2.

>> Do you think I need to add a benchmark and run a perf for this kind of
>> change?
>
>No, you are doing less work so this should be better as long as it is
>correct.  Zeroing can be expensive on some archs so this is good.
>
>> ---
>>  lib/maple_tree.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>> 
>> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
>> index d8f10773e451..911c5e04e634 100644
>> --- a/lib/maple_tree.c
>> +++ b/lib/maple_tree.c
>> @@ -3134,10 +3134,7 @@ static inline void mast_fill_bnode(struct maple_subtree_state *mast,
>>  	bool cp = true;
>>  	unsigned char split;
>>  
>> -	memset(mast->bn->gap, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->gap));
>> -	memset(mast->bn->slot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->slot));
>> -	memset(mast->bn->pivot, 0, sizeof(unsigned long) * ARRAY_SIZE(mast->bn->pivot));
>> -	mast->bn->b_end = 0;
>> +	memset(mast->bn, 0, sizeof(struct maple_big_node));
>>  
>>  	if (mte_is_root(mas->node)) {
>>  		cp = false;
>> -- 
>> 2.34.1
>> 

-- 
Wei Yang
Help you, Help me