Re: [PATCH] maple_tree: use xa_is_internal() for better reading

Re: [PATCH] maple_tree: use xa_is_internal() for better reading

[Liam R. Howlett]

* 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
> 

Re: [PATCH] maple_tree: use xa_is_internal() for better reading

[Wei Yang]

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

Re: [PATCH] maple_tree: use xa_is_internal() for better reading

[Liam R. Howlett]

* 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