| Message ID | 20250210083526.252955-1-richard120310@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | maple_tree: Remove redundant mte_to_node() in mte_dead_node() | expand |
* I Hsin Cheng <richard120310@gmail.com> [250210 03:35]: > In mte_dead_node(), it already assign "node" as "mte_to_node(enode)" in > the first place, calling "mte_parent(enode)" will result in the same > "mte_to_node(enode)" again which is redundant. This is a very confusing way of saying "avoid calling mte_to_node() in the mte_parent() call by using the ma_dead_node() instead." In fact, the subject is wrong as well, since the mte_to_node() was removed from the call path of mte_dead_node(), and not the function itself. > > Refactor mte_dead_node() and utilize ma_dead_node() to perform the > parent check without the redundant "mte_to_node()". > > Signed-off-by: I Hsin Cheng <richard120310@gmail.com> The code looks right, but the subject and change log are not. Please respin the patch, something like this: maple_tree: Use ma_dead_node() in mte_dead_node() Using ma_dead_node() in mte_dead_node() avoids decoding the maple enode for a second time to find the parent. Feel free to change it as you'd like, but I couldn't follow what you meant. > --- > lib/maple_tree.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/lib/maple_tree.c b/lib/maple_tree.c > index f7153ade1be5..362f85c62678 100644 > --- a/lib/maple_tree.c > +++ b/lib/maple_tree.c > @@ -584,13 +584,10 @@ static __always_inline bool ma_dead_node(const struct maple_node *node) > */ > static __always_inline bool mte_dead_node(const struct maple_enode *enode) > { > - struct maple_node *parent, *node; > + struct maple_node *node; > > node = mte_to_node(enode); > - /* Do not reorder reads from the node prior to the parent check */ > - smp_rmb(); > - parent = mte_parent(enode); > - return (parent == node); > + return ma_dead_node(node); > } > > /* > -- > 2.43.0 >
On Mon, Feb 10, 2025 at 09:51:42AM -0500, Liam R. Howlett wrote: > * I Hsin Cheng <richard120310@gmail.com> [250210 03:35]: > > > In mte_dead_node(), it already assign "node" as "mte_to_node(enode)" in > > the first place, calling "mte_parent(enode)" will result in the same > > "mte_to_node(enode)" again which is redundant. > > This is a very confusing way of saying "avoid calling mte_to_node() in > the mte_parent() call by using the ma_dead_node() instead." > > In fact, the subject is wrong as well, since the mte_to_node() was > removed from the call path of mte_dead_node(), and not the function > itself. > > > > > Refactor mte_dead_node() and utilize ma_dead_node() to perform the > > parent check without the redundant "mte_to_node()". > > > > Signed-off-by: I Hsin Cheng <richard120310@gmail.com> > > The code looks right, but the subject and change log are not. Please > respin the patch, something like this: > > maple_tree: Use ma_dead_node() in mte_dead_node() > > Using ma_dead_node() in mte_dead_node() avoids decoding the maple enode > for a second time to find the parent. > > Feel free to change it as you'd like, but I couldn't follow what you > meant. > > > --- > > lib/maple_tree.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/lib/maple_tree.c b/lib/maple_tree.c > > index f7153ade1be5..362f85c62678 100644 > > --- a/lib/maple_tree.c > > +++ b/lib/maple_tree.c > > @@ -584,13 +584,10 @@ static __always_inline bool ma_dead_node(const struct maple_node *node) > > */ > > static __always_inline bool mte_dead_node(const struct maple_enode *enode) > > { > > - struct maple_node *parent, *node; > > + struct maple_node *node; > > > > node = mte_to_node(enode); > > - /* Do not reorder reads from the node prior to the parent check */ > > - smp_rmb(); > > - parent = mte_parent(enode); > > - return (parent == node); > > + return ma_dead_node(node); > > } > > > > /* > > -- > > 2.43.0 > > Hello Liam, Thanks for your kindly review! > In fact, the subject is wrong as well, since the mte_to_node() was > removed from the call path of mte_dead_node(), and not the function > itself. I see, I'll rephrase the whole commit and send a new patch later, what I wrote is indeed too confusing. Thank you. Best regards, I Hsin Cheng
diff --git a/lib/maple_tree.c b/lib/maple_tree.c index f7153ade1be5..362f85c62678 100644 --- a/lib/maple_tree.c +++ b/lib/maple_tree.c @@ -584,13 +584,10 @@ static __always_inline bool ma_dead_node(const struct maple_node *node) */ static __always_inline bool mte_dead_node(const struct maple_enode *enode) { - struct maple_node *parent, *node; + struct maple_node *node; node = mte_to_node(enode); - /* Do not reorder reads from the node prior to the parent check */ - smp_rmb(); - parent = mte_parent(enode); - return (parent == node); + return ma_dead_node(node); } /*
In mte_dead_node(), it already assign "node" as "mte_to_node(enode)" in the first place, calling "mte_parent(enode)" will result in the same "mte_to_node(enode)" again which is redundant. Refactor mte_dead_node() and utilize ma_dead_node() to perform the parent check without the redundant "mte_to_node()". Signed-off-by: I Hsin Cheng <richard120310@gmail.com> --- lib/maple_tree.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)