* Re: [PATCH] maple_tree: use xa_is_internal() for better reading [not found] <20240808043717.10930-1-richard.weiyang@gmail.com> @ 2024-08-08 16:53 ` Liam R. Howlett 2024-08-09 1:58 ` Wei Yang 0 siblings, 1 reply; 3+ messages in thread From: Liam R. Howlett @ 2024-08-08 16:53 UTC (permalink / raw) To: Wei Yang; +Cc: akpm, maple-tree, linux-mm * Wei Yang <richard.weiyang@gmail.com> [240808 00:38]: > If entry is a special case, we need to expand root to store it. This > case is exactly the case of xa_is_internal(). > > Let's use xa_is_internal() for the check, which is friendly for > audience. Nack This check is to see if it will be detected as an internal node - this much is correct. But changing the check to use this function reads far worse than what exists today. If you look at the code below, it seems to indicate that the internal entry is being inserted into the tree - but it's not an internal entry, it's an entry that would be detected as an internal entry. This is not friendlier for the audience, it's confusing. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.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 aa3a5df15b8e..d39b0bceb802 100644 > --- a/lib/maple_tree.c > +++ b/lib/maple_tree.c > @@ -3515,7 +3515,7 @@ static inline void mas_store_root(struct ma_state *mas, void *entry) > { > if (likely((mas->last != 0) || (mas->index != 0))) > mas_root_expand(mas, entry); > - else if (((unsigned long) (entry) & 3) == 2) > + else if (xa_is_internal(entry)) > mas_root_expand(mas, entry); > else { > rcu_assign_pointer(mas->tree->ma_root, entry); > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] maple_tree: use xa_is_internal() for better reading 2024-08-08 16:53 ` [PATCH] maple_tree: use xa_is_internal() for better reading Liam R. Howlett @ 2024-08-09 1:58 ` Wei Yang 2024-08-09 14:16 ` Liam R. Howlett 0 siblings, 1 reply; 3+ messages in thread From: Wei Yang @ 2024-08-09 1:58 UTC (permalink / raw) To: Liam R. Howlett, Wei Yang, akpm, maple-tree, linux-mm On Thu, Aug 08, 2024 at 12:53:55PM -0400, Liam R. Howlett wrote: >* Wei Yang <richard.weiyang@gmail.com> [240808 00:38]: >> If entry is a special case, we need to expand root to store it. This >> case is exactly the case of xa_is_internal(). >> >> Let's use xa_is_internal() for the check, which is friendly for >> audience. > >Nack > >This check is to see if it will be detected as an internal node - this >much is correct. > >But changing the check to use this function reads far worse than what >exists today. If you look at the code below, it seems to indicate that >the internal entry is being inserted into the tree - but it's not an >internal entry, it's an entry that would be detected as an internal >entry. > From my understanding, you want to say xa_is_internal() only applies on "an entry" in the tree instead of a "value" we want to insert into the tree. But it looks current code already use xa_is_internal() to check the value we want to insert in the tree. For example: * xa_is_advanced() * mt_is_reserved() xa_is_advanced() is used in mtree_insert_range() to prevent inserting an advanced value for a normal API. mt_is_reserved() is used in mtree_alloc_range() to prevent inserting reserved value. Both check apply to an entry which will be inserted, instead of an entry in current tree. And they are clearly to inform me the value we want to insert has special meaning for maple tree and need to be handled carefully. >This is not friendlier for the audience, it's confusing. > The confusing point comes from audience would think the "entry" is already an internal entry in the tree? If you really think audience would be misled, I would suggest introduce a new helper, e.g. mt_is_internal(), just like xa_is_advanced()/mt_is_reserved(). To be honest, an open coded check doesn't looks a good practice. -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] maple_tree: use xa_is_internal() for better reading 2024-08-09 1:58 ` Wei Yang @ 2024-08-09 14:16 ` Liam R. Howlett 0 siblings, 0 replies; 3+ messages in thread From: Liam R. Howlett @ 2024-08-09 14:16 UTC (permalink / raw) To: Wei Yang; +Cc: akpm, maple-tree, linux-mm * Wei Yang <richard.weiyang@gmail.com> [240808 21:59]: > On Thu, Aug 08, 2024 at 12:53:55PM -0400, Liam R. Howlett wrote: > >* Wei Yang <richard.weiyang@gmail.com> [240808 00:38]: > >> If entry is a special case, we need to expand root to store it. This > >> case is exactly the case of xa_is_internal(). > >> > >> Let's use xa_is_internal() for the check, which is friendly for > >> audience. > > > >Nack > > > >This check is to see if it will be detected as an internal node - this > >much is correct. > > > >But changing the check to use this function reads far worse than what > >exists today. If you look at the code below, it seems to indicate that > >the internal entry is being inserted into the tree - but it's not an > >internal entry, it's an entry that would be detected as an internal > >entry. > > > > From my understanding, you want to say xa_is_internal() only applies on "an > entry" in the tree instead of a "value" we want to insert into the tree. > > But it looks current code already use xa_is_internal() to check the value we > want to insert in the tree. > > For example: > > * xa_is_advanced() This is used much lower in the stack, where you see the entry is coming in and it seems obvious what is going on. > * mt_is_reserved() reading that something is reserved makes sense. Saying an entry is an internal tree node makes no sense, it's also buried in a function that doesn't immediately make sense to be branching on a value. > > xa_is_advanced() is used in mtree_insert_range() to prevent inserting an > advanced value for a normal API. > mt_is_reserved() is used in mtree_alloc_range() to prevent inserting reserved > value. > > Both check apply to an entry which will be inserted, instead of an entry in > current tree. And they are clearly to inform me the value we want to insert > has special meaning for maple tree and need to be handled carefully. > > >This is not friendlier for the audience, it's confusing. > > > > The confusing point comes from audience would think the "entry" is already an > internal entry in the tree? The confusing point is the resulting code doesn't make this any more friendly for the audience, why are we expanding the root node on mt_is_internal(entry)? Why is entry an internal node? How do we get to this function with a node as an entry - That seems wrong, let's waste time digging into how that happened.. You didn't make it more friendly, you made it more puzzling. > > If you really think audience would be misled, I would suggest introduce a new > helper, e.g. mt_is_internal(), just like xa_is_advanced()/mt_is_reserved(). I'm all for readable code - in fact, I'd be more inclined to accept a static inline function that just does the check with a comment about what it's doing.. > > To be honest, an open coded check doesn't looks a good practice. Thank you for your valuable insight. The entire point of your patch is to make it more readable and it does not do that. If you want to make it more readable, then I will look at that patch, but I don't particularly feel motivated to spend time doing that after this exchange. Liam ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-08-09 14:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20240808043717.10930-1-richard.weiyang@gmail.com>
2024-08-08 16:53 ` [PATCH] maple_tree: use xa_is_internal() for better reading Liam R. Howlett
2024-08-09 1:58 ` Wei Yang
2024-08-09 14:16 ` Liam R. Howlett
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox