[PATCH 0/7] spanning write related cleanup

[PATCH 0/7] spanning write related cleanup

[Wei Yang]

Here is some cleanup related to spanning write.

Wei Yang (7):
  maple_tree: not necessary to check ahead if !content
  maple_tree: validate we won't split on NULL
  maple_tree: check mid_split only may have
  maple_tree: the return value of mast_spanning_rebalance() is not used
  maple_tree: the type of left subtree is already saved in bnode->type
  maple_tree: always need to update max of new left node
  maple_tree: only ascend left subtree to get the old node for
    replacement

 lib/maple_tree.c | 56 +++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

-- 
2.34.1

[PATCH 1/7] maple_tree: not necessary to check ahead if !content

[Wei Yang]

Just like mas_wr_extend_null(), only when content is not NULL, we may
have a NULL slot ahead.

So we can wrap it in an else.

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 | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index 7efb1520f9bd..98692704d773 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -3568,18 +3568,20 @@ static inline void mas_extend_spanning_null(struct ma_wr_state *l_wr_mas,
 	unsigned char l_slot;
 
 	l_slot = l_mas->offset;
-	if (!l_wr_mas->content)
+	if (!l_wr_mas->content) {
+		/* If this one is null, the next and prev are not */
 		l_mas->index = l_wr_mas->r_min;
+	} else {
+		if ((l_mas->index == l_wr_mas->r_min) &&
+			 (l_slot &&
+			  !mas_slot_locked(l_mas, l_wr_mas->slots, l_slot - 1))) {
+			if (l_slot > 1)
+				l_mas->index = l_wr_mas->pivots[l_slot - 2] + 1;
+			else
+				l_mas->index = l_mas->min;
 
-	if ((l_mas->index == l_wr_mas->r_min) &&
-		 (l_slot &&
-		  !mas_slot_locked(l_mas, l_wr_mas->slots, l_slot - 1))) {
-		if (l_slot > 1)
-			l_mas->index = l_wr_mas->pivots[l_slot - 2] + 1;
-		else
-			l_mas->index = l_mas->min;
-
-		l_mas->offset = l_slot - 1;
+			l_mas->offset = l_slot - 1;
+		}
 	}
 
 	if (!r_wr_mas->content) {
-- 
2.34.1

[PATCH 2/7] maple_tree: validate we won't split on NULL

[Wei Yang]

If this is not the right most node, we don't expect its last slot to be
NULL.

Let's validate this.

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index 98692704d773..2a00441130ee 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -7522,6 +7522,12 @@ static void mt_validate_nulls(struct maple_tree *mt)
 		MT_BUG_ON(mt, !last && !entry);
 		last = entry;
 		if (offset == mas_data_end(&mas)) {
+			if ((mas.max != ULONG_MAX) && !entry) {
+				pr_err("Last slot %p end with NULL\n",
+					mas_mn(&mas));
+				MT_BUG_ON(mas.tree, 1);
+			}
+
 			mas_next_node(&mas, mas_mn(&mas), ULONG_MAX);
 			if (mas_is_overflow(&mas))
 				return;
-- 
2.34.1

[PATCH 3/7] maple_tree: check mid_split only may have

[Wei Yang]

We only may set mid_split in the else clause, let's move the check in
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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index 2a00441130ee..f5606b4d0dba 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -2381,10 +2381,11 @@ static inline unsigned char mas_mab_to_node(struct ma_state *mas,
 	} else {
 		split = mab_calc_split(mas, b_node, mid_split);
 		*right = mas_new_ma_node(mas, b_node);
+
+		if (*mid_split)
+			*middle = mas_new_ma_node(mas, b_node);
 	}
 
-	if (*mid_split)
-		*middle = mas_new_ma_node(mas, b_node);
 
 	return split;
 
-- 
2.34.1

[PATCH 4/7] maple_tree: the return value of mast_spanning_rebalance() is not used

[Wei Yang]

The return value is not used, change it to a void function.

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, 4 insertions(+), 4 deletions(-)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index f5606b4d0dba..6cabee1371ec 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -2265,7 +2265,7 @@ static inline void mast_rebalance_prev(struct maple_subtree_state *mast)
  * @mast: The maple_subtree_state.
  */
 static inline
-bool mast_spanning_rebalance(struct maple_subtree_state *mast)
+void mast_spanning_rebalance(struct maple_subtree_state *mast)
 {
 	struct ma_state r_tmp = *mast->orig_r;
 	struct ma_state l_tmp = *mast->orig_l;
@@ -2284,7 +2284,7 @@ bool mast_spanning_rebalance(struct maple_subtree_state *mast)
 
 			mast_rebalance_next(mast);
 			*mast->orig_l = l_tmp;
-			return true;
+			return;
 		} else if (mast->orig_l->offset != 0) {
 			mast->orig_l->offset--;
 			do {
@@ -2295,13 +2295,13 @@ bool mast_spanning_rebalance(struct maple_subtree_state *mast)
 
 			mast_rebalance_prev(mast);
 			*mast->orig_r = r_tmp;
-			return true;
+			return;
 		}
 	} while (!mte_is_root(mast->orig_r->node));
 
 	*mast->orig_r = r_tmp;
 	*mast->orig_l = l_tmp;
-	return false;
+	return;
 }
 
 /*
-- 
2.34.1

[PATCH 5/7] maple_tree: the type of left subtree is already saved in bnode->type

[Wei Yang]

mast_sufficient()/mast_overflow() are only used in
mas_spanning_rebalance(). Before doing the check, mast_ascend() has
retrieved the left subtree type and stored in bnode->type.

So we can use the type in bnode->type directly to get minimum slot
count.

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 6cabee1371ec..56e9857ce681 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -2729,7 +2729,7 @@ static inline void mast_combine_cp_right(struct maple_subtree_state *mast)
  */
 static inline bool mast_sufficient(struct maple_subtree_state *mast)
 {
-	if (mast->bn->b_end > mt_min_slot_count(mast->orig_l->node))
+	if (mast->bn->b_end > mt_min_slots[mast->bn->type])
 		return true;
 
 	return false;
@@ -2742,7 +2742,7 @@ static inline bool mast_sufficient(struct maple_subtree_state *mast)
  */
 static inline bool mast_overflow(struct maple_subtree_state *mast)
 {
-	if (mast->bn->b_end >= mt_slot_count(mast->orig_l->node))
+	if (mast->bn->b_end >= mt_slots[mast->bn->type])
 		return true;
 
 	return false;
-- 
2.34.1

[PATCH 6/7] maple_tree: always need to update max of new left node

[Wei Yang]

If (split == mast->bn->b_end), this means we would put all data from
bnode to new left. And the original data source of bnode is orig_l +
orig_r, whose range is [orig_l->min, orig_r->max]

So we can always update max of new left node.

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 | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index 56e9857ce681..4ba9ae68c0a9 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -2664,19 +2664,12 @@ static inline void mast_cp_to_nodes(struct maple_subtree_state *mast,
 	struct maple_enode *left, struct maple_enode *middle,
 	struct maple_enode *right, unsigned char split, unsigned char mid_split)
 {
-	bool new_lmax = true;
-
 	mas_node_or_none(mast->l, left);
 	mas_node_or_none(mast->m, middle);
 	mas_node_or_none(mast->r, right);
 
 	mast->l->min = mast->orig_l->min;
-	if (split == mast->bn->b_end) {
-		mast->l->max = mast->orig_r->max;
-		new_lmax = false;
-	}
-
-	mab_mas_cp(mast->bn, 0, split, mast->l, new_lmax);
+	mab_mas_cp(mast->bn, 0, split, mast->l, true);
 
 	if (middle) {
 		mab_mas_cp(mast->bn, 1 + split, mid_split, mast->m, true);
-- 
2.34.1

[PATCH 7/7] maple_tree: only ascend left subtree to get the old node for replacement

[Wei Yang]

When we come to a new_root, we should go up the tree to get the old root
for replacement.

Instead of ascend both left/right subtree, we only ascend one of it is
enough. Let's use the left subtree as it dose now.

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 4ba9ae68c0a9..2c05919be168 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -2920,7 +2920,7 @@ static void mas_spanning_rebalance(struct ma_state *mas,
 new_root:
 		mas_mn(mast->l)->parent = ma_parent_ptr(mas_tree_parent(mas));
 		while (!mte_is_root(mast->orig_l->node))
-			mast_ascend(mast);
+			mas_ascend(mast->orig_l);
 	} else {
 		mas_mn(&l_mas)->parent = mas_mn(mast->orig_l)->parent;
 	}
-- 
2.34.1

Re: [PATCH 0/7] spanning write related cleanup

[Liam R. Howlett]

* Wei Yang <richard.weiyang@gmail.com> [241126 20:28]:
> Here is some cleanup related to spanning write.

None of these fix anything, but do fiddle with code that's pretty
critical to the kernel.  Most of the changes will be immeasurable in
change but carry risk to causing subtle changes.

Some are simple removal of returns that aren't used while others change
things because you think they are probably the equivalent.  This seems
like unnecessary chrun at this point.  I'm all for efficient code but
this is getting a bit much, some of these are just preference of what to
use that will already exist in the cpu cache.

I'll get back to you when I dig through them, as some need a deeper look
for sure.

Liam

> 
> Wei Yang (7):
>   maple_tree: not necessary to check ahead if !content
>   maple_tree: validate we won't split on NULL
>   maple_tree: check mid_split only may have
>   maple_tree: the return value of mast_spanning_rebalance() is not used
>   maple_tree: the type of left subtree is already saved in bnode->type
>   maple_tree: always need to update max of new left node
>   maple_tree: only ascend left subtree to get the old node for
>     replacement
> 
>  lib/maple_tree.c | 56 +++++++++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 27 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH 0/7] spanning write related cleanup

[Wei Yang]

On Wed, Nov 27, 2024 at 08:31:13AM -0500, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [241126 20:28]:
>> Here is some cleanup related to spanning write.
>
>None of these fix anything, but do fiddle with code that's pretty
>critical to the kernel.  Most of the changes will be immeasurable in
>change but carry risk to causing subtle changes.
>
>Some are simple removal of returns that aren't used while others change
>things because you think they are probably the equivalent.  This seems
>like unnecessary chrun at this point.  I'm all for efficient code but
>this is getting a bit much, some of these are just preference of what to
>use that will already exist in the cpu cache.
>
>I'll get back to you when I dig through them, as some need a deeper look
>for sure.

Thanks

>
>Liam
>
>> 
>> Wei Yang (7):
>>   maple_tree: not necessary to check ahead if !content
>>   maple_tree: validate we won't split on NULL
>>   maple_tree: check mid_split only may have
>>   maple_tree: the return value of mast_spanning_rebalance() is not used
>>   maple_tree: the type of left subtree is already saved in bnode->type
>>   maple_tree: always need to update max of new left node
>>   maple_tree: only ascend left subtree to get the old node for
>>     replacement
>> 
>>  lib/maple_tree.c | 56 +++++++++++++++++++++++++-----------------------
>>  1 file changed, 29 insertions(+), 27 deletions(-)
>> 
>> -- 
>> 2.34.1
>> 

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/7] spanning write related cleanup

[Wei Yang]

On Wed, Nov 27, 2024 at 08:31:13AM -0500, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [241126 20:28]:
>> Here is some cleanup related to spanning write.
>
>None of these fix anything, but do fiddle with code that's pretty
>critical to the kernel.  Most of the changes will be immeasurable in
>change but carry risk to causing subtle changes.
>
>Some are simple removal of returns that aren't used while others change
>things because you think they are probably the equivalent.  This seems
>like unnecessary chrun at this point.  I'm all for efficient code but
>this is getting a bit much, some of these are just preference of what to
>use that will already exist in the cpu cache.
>
>I'll get back to you when I dig through them, as some need a deeper look
>for sure.
>
>Liam
>

Hi, Liam

Would you mind taking a look when you have time?

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/7] spanning write related cleanup

[Liam R. Howlett]

* Wei Yang <richard.weiyang@gmail.com> [250117 00:49]:
> On Wed, Nov 27, 2024 at 08:31:13AM -0500, Liam R. Howlett wrote:
> >* Wei Yang <richard.weiyang@gmail.com> [241126 20:28]:
> >> Here is some cleanup related to spanning write.
> >
> >None of these fix anything, but do fiddle with code that's pretty
> >critical to the kernel.  Most of the changes will be immeasurable in
> >change but carry risk to causing subtle changes.
> >
> >Some are simple removal of returns that aren't used while others change
> >things because you think they are probably the equivalent.  This seems
> >like unnecessary chrun at this point.  I'm all for efficient code but
> >this is getting a bit much, some of these are just preference of what to
> >use that will already exist in the cpu cache.
> >
> >I'll get back to you when I dig through them, as some need a deeper look
> >for sure.
> >
> >Liam
> >
> 
> Hi, Liam
> 
> Would you mind taking a look when you have time?

Yes, I'll have a look soon.  I don't love changes that dive deep into
complex code that results in no gains (performance or feature wise).

It's also odd to have simple "this return isn't use" and things moving
code blocks to be executed only in certain scenarios, as the difficulty
to verify the latter is much higher.

Can we please limit changes to areas where there is a performance change
or coupled with a change that is needed?  ie: stop sending patches that
change things unless it's with a feature or improvement (performance or
otherwise).  I'm just not convinced some of these are worth the
cost vs risk.

Thanks,
Liam

Re: [PATCH 0/7] spanning write related cleanup

[Wei Yang]

On Thu, Jan 23, 2025 at 12:52:40PM -0500, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [250117 00:49]:
>> On Wed, Nov 27, 2024 at 08:31:13AM -0500, Liam R. Howlett wrote:
>> >* Wei Yang <richard.weiyang@gmail.com> [241126 20:28]:
>> >> Here is some cleanup related to spanning write.
>> >
>> >None of these fix anything, but do fiddle with code that's pretty
>> >critical to the kernel.  Most of the changes will be immeasurable in
>> >change but carry risk to causing subtle changes.
>> >
>> >Some are simple removal of returns that aren't used while others change
>> >things because you think they are probably the equivalent.  This seems
>> >like unnecessary chrun at this point.  I'm all for efficient code but
>> >this is getting a bit much, some of these are just preference of what to
>> >use that will already exist in the cpu cache.
>> >
>> >I'll get back to you when I dig through them, as some need a deeper look
>> >for sure.
>> >
>> >Liam
>> >
>> 
>> Hi, Liam
>> 
>> Would you mind taking a look when you have time?
>
>Yes, I'll have a look soon.  I don't love changes that dive deep into
>complex code that results in no gains (performance or feature wise).
>
>It's also odd to have simple "this return isn't use" and things moving
>code blocks to be executed only in certain scenarios, as the difficulty
>to verify the latter is much higher.
>
>Can we please limit changes to areas where there is a performance change
>or coupled with a change that is needed?  ie: stop sending patches that
>change things unless it's with a feature or improvement (performance or
>otherwise).  I'm just not convinced some of these are worth the
>cost vs risk.
>

Ok.

So you would drop this patch set or still want to take a look?

>Thanks,
>Liam

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/7] spanning write related cleanup

[Liam R. Howlett]

* Wei Yang <richard.weiyang@gmail.com> [250123 20:44]:
> On Thu, Jan 23, 2025 at 12:52:40PM -0500, Liam R. Howlett wrote:
> >* Wei Yang <richard.weiyang@gmail.com> [250117 00:49]:
> >> On Wed, Nov 27, 2024 at 08:31:13AM -0500, Liam R. Howlett wrote:
> >> >* Wei Yang <richard.weiyang@gmail.com> [241126 20:28]:
> >> >> Here is some cleanup related to spanning write.
> >> >
> >> >None of these fix anything, but do fiddle with code that's pretty
> >> >critical to the kernel.  Most of the changes will be immeasurable in
> >> >change but carry risk to causing subtle changes.
> >> >
> >> >Some are simple removal of returns that aren't used while others change
> >> >things because you think they are probably the equivalent.  This seems
> >> >like unnecessary chrun at this point.  I'm all for efficient code but
> >> >this is getting a bit much, some of these are just preference of what to
> >> >use that will already exist in the cpu cache.
> >> >
> >> >I'll get back to you when I dig through them, as some need a deeper look
> >> >for sure.
> >> >
> >> >Liam
> >> >
> >> 
> >> Hi, Liam
> >> 
> >> Would you mind taking a look when you have time?
> >
> >Yes, I'll have a look soon.  I don't love changes that dive deep into
> >complex code that results in no gains (performance or feature wise).
> >
> >It's also odd to have simple "this return isn't use" and things moving
> >code blocks to be executed only in certain scenarios, as the difficulty
> >to verify the latter is much higher.
> >
> >Can we please limit changes to areas where there is a performance change
> >or coupled with a change that is needed?  ie: stop sending patches that
> >change things unless it's with a feature or improvement (performance or
> >otherwise).  I'm just not convinced some of these are worth the
> >cost vs risk.
> >
> 
> Ok.
> 
> So you would drop this patch set or still want to take a look?

I was going to look at it, but after I send my reply, I received a
report of an issue caused in a certain configuration that caused the
stack frame to grow out of the configured 1024 limit, which was tracked
to a patch you added to simplify a previous function.

So, I think we should drop these patches since they don't make a
measurable difference and are not without risk.

Thanks,
Liam

Re: [PATCH 0/7] spanning write related cleanup

[Wei Yang]

On Mon, Jan 27, 2025 at 09:36:11AM -0500, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [250123 20:44]:
>> On Thu, Jan 23, 2025 at 12:52:40PM -0500, Liam R. Howlett wrote:
>> >* Wei Yang <richard.weiyang@gmail.com> [250117 00:49]:
>> >> On Wed, Nov 27, 2024 at 08:31:13AM -0500, Liam R. Howlett wrote:
>> >> >* Wei Yang <richard.weiyang@gmail.com> [241126 20:28]:
>> >> >> Here is some cleanup related to spanning write.
>> >> >
>> >> >None of these fix anything, but do fiddle with code that's pretty
>> >> >critical to the kernel.  Most of the changes will be immeasurable in
>> >> >change but carry risk to causing subtle changes.
>> >> >
>> >> >Some are simple removal of returns that aren't used while others change
>> >> >things because you think they are probably the equivalent.  This seems
>> >> >like unnecessary chrun at this point.  I'm all for efficient code but
>> >> >this is getting a bit much, some of these are just preference of what to
>> >> >use that will already exist in the cpu cache.
>> >> >
>> >> >I'll get back to you when I dig through them, as some need a deeper look
>> >> >for sure.
>> >> >
>> >> >Liam
>> >> >
>> >> 
>> >> Hi, Liam
>> >> 
>> >> Would you mind taking a look when you have time?
>> >
>> >Yes, I'll have a look soon.  I don't love changes that dive deep into
>> >complex code that results in no gains (performance or feature wise).
>> >
>> >It's also odd to have simple "this return isn't use" and things moving
>> >code blocks to be executed only in certain scenarios, as the difficulty
>> >to verify the latter is much higher.
>> >
>> >Can we please limit changes to areas where there is a performance change
>> >or coupled with a change that is needed?  ie: stop sending patches that
>> >change things unless it's with a feature or improvement (performance or
>> >otherwise).  I'm just not convinced some of these are worth the
>> >cost vs risk.
>> >
>> 
>> Ok.
>> 
>> So you would drop this patch set or still want to take a look?
>
>I was going to look at it, but after I send my reply, I received a
>report of an issue caused in a certain configuration that caused the
>stack frame to grow out of the configured 1024 limit, which was tracked
>to a patch you added to simplify a previous function.

Sorry for that. Would you mind letting me know what is the problem? or cc me
in case you will fix it?

>
>So, I think we should drop these patches since they don't make a
>measurable difference and are not without risk.
>
>Thanks,
>Liam

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/7] spanning write related cleanup

[Wei Yang]

On Mon, Jan 27, 2025 at 09:36:11AM -0500, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [250123 20:44]:
>> On Thu, Jan 23, 2025 at 12:52:40PM -0500, Liam R. Howlett wrote:
>> >* Wei Yang <richard.weiyang@gmail.com> [250117 00:49]:
>> >> On Wed, Nov 27, 2024 at 08:31:13AM -0500, Liam R. Howlett wrote:
>> >> >* Wei Yang <richard.weiyang@gmail.com> [241126 20:28]:
>> >> >> Here is some cleanup related to spanning write.
>> >> >
>> >> >None of these fix anything, but do fiddle with code that's pretty
>> >> >critical to the kernel.  Most of the changes will be immeasurable in
>> >> >change but carry risk to causing subtle changes.
>> >> >
>> >> >Some are simple removal of returns that aren't used while others change
>> >> >things because you think they are probably the equivalent.  This seems
>> >> >like unnecessary chrun at this point.  I'm all for efficient code but
>> >> >this is getting a bit much, some of these are just preference of what to
>> >> >use that will already exist in the cpu cache.
>> >> >
>> >> >I'll get back to you when I dig through them, as some need a deeper look
>> >> >for sure.
>> >> >
>> >> >Liam
>> >> >
>> >> 
>> >> Hi, Liam
>> >> 
>> >> Would you mind taking a look when you have time?
>> >
>> >Yes, I'll have a look soon.  I don't love changes that dive deep into
>> >complex code that results in no gains (performance or feature wise).
>> >
>> >It's also odd to have simple "this return isn't use" and things moving
>> >code blocks to be executed only in certain scenarios, as the difficulty
>> >to verify the latter is much higher.
>> >
>> >Can we please limit changes to areas where there is a performance change
>> >or coupled with a change that is needed?  ie: stop sending patches that
>> >change things unless it's with a feature or improvement (performance or
>> >otherwise).  I'm just not convinced some of these are worth the
>> >cost vs risk.
>> >
>> 
>> Ok.
>> 
>> So you would drop this patch set or still want to take a look?
>
>I was going to look at it, but after I send my reply, I received a
>report of an issue caused in a certain configuration that caused the
>stack frame to grow out of the configured 1024 limit, which was tracked
>to a patch you added to simplify a previous function.
>
>So, I think we should drop these patches since they don't make a
>measurable difference and are not without risk.
>

Hi, Liam

Maybe I found one issue in current code that we miss to set the root node
dead. Before sending a fix, I want to check with you first.

Current behavior:

  mte_destroy_walk()
    mt_destroy_walk()
      if (mte_is_leaf())
        goto free_leaf;
      mte_destroy_descend()
        mte_set_node_dead()    <-- set node dead here

Usually we set all node dead in mte_destroy_descend(). But if the maple tree
has just one root node, it jump to free_leaf without set it dead.

It seems not correct to leave a non-dead node. Is my understanding correct?

>Thanks,
>Liam

-- 
Wei Yang
Help you, Help me

Re: [PATCH 0/7] spanning write related cleanup

[Wei Yang]

On Mon, Jan 27, 2025 at 09:36:11AM -0500, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [250123 20:44]:
[...]
>> >> Hi, Liam
>> >> 
>> >> Would you mind taking a look when you have time?
>> >
>> >Yes, I'll have a look soon.  I don't love changes that dive deep into
>> >complex code that results in no gains (performance or feature wise).
>> >
>> >It's also odd to have simple "this return isn't use" and things moving
>> >code blocks to be executed only in certain scenarios, as the difficulty
>> >to verify the latter is much higher.
>> >
>> >Can we please limit changes to areas where there is a performance change
>> >or coupled with a change that is needed?  ie: stop sending patches that
>> >change things unless it's with a feature or improvement (performance or
>> >otherwise).  I'm just not convinced some of these are worth the
>> >cost vs risk.
>> >
>> 
>> Ok.
>> 
>> So you would drop this patch set or still want to take a look?
>
>I was going to look at it, but after I send my reply, I received a
>report of an issue caused in a certain configuration that caused the
>stack frame to grow out of the configured 1024 limit, which was tracked
>to a patch you added to simplify a previous function.
>

Liam,

I tried to locate which of my patch introduce the problem, but not succeed.
This is the issue of -Wframe-larger-than=, right?

Would you mind letting me know which one did something bad? So that I can
avoid such thing later.

>So, I think we should drop these patches since they don't make a
>measurable difference and are not without risk.
>
>Thanks,
>Liam

-- 
Wei Yang
Help you, Help me