Re: [PATCH v4 04/17] maple_tree: introduce mas_wr_store_type()

[Sid Kumar <sidhartha.kumar@oracle.com>]

On 9/25/24 2:33 PM, Sid Kumar wrote:
>
> On 9/24/24 9:04 PM, Wei Yang wrote:
>> On Wed, Aug 14, 2024 at 12:19:31PM -0400, Sidhartha Kumar wrote:
>>
>> Sorry for a late reply, I just see this change.
>>
>>> +
>>> +/*
>>> + * mas_wr_store_type() - Set the store type for a given
>>> + * store operation.
>>> + * @wr_mas: The maple write state
>>> + */
>>> +static inline void mas_wr_store_type(struct ma_wr_state *wr_mas)
>>> +{
>>> +    struct ma_state *mas = wr_mas->mas;
>>> +    unsigned char new_end;
>>> +
>>> +    if (unlikely(mas_is_none(mas) || mas_is_ptr(mas))) {
>>> +        mas->store_type = wr_store_root;
>>> +        return;
>>> +    }
>>> +
>>> +    if (unlikely(!mas_wr_walk(wr_mas))) {
>>> +        mas->store_type = wr_spanning_store;
>>> +        return;
>>> +    }
>>> +
>>> +    /* At this point, we are at the leaf node that needs to be 
>>> altered. */
>>> +    mas_wr_end_piv(wr_mas);
>>> +    if (!wr_mas->entry)
>>> +        mas_wr_extend_null(wr_mas);
>>> +
>>> +    new_end = mas_wr_new_end(wr_mas);
>>> +    if ((wr_mas->r_min == mas->index) && (wr_mas->r_max == 
>>> mas->last)) {
>>> +        mas->store_type = wr_exact_fit;
>>> +        return;
>>> +    }
>>> +
>>> +    if (unlikely(!mas->index && mas->last == ULONG_MAX)) {
>>> +        mas->store_type = wr_new_root;
>>> +        return;
>>> +    }
>>> +
>>> +    /* Potential spanning rebalance collapsing a node */
>>> +    if (new_end < mt_min_slots[wr_mas->type]) {
>>> +        if (!mte_is_root(mas->node)) {
>>> +            mas->store_type = wr_rebalance;
>>> +            return;
>>> +        }
>>> +        mas->store_type = wr_node_store;
>>> +        return;
>>> +    }
>> After this check, we are sure new_end >= mt_min_slots[wr_mas->type].
>>
>>> +
>>> +    if (new_end >= mt_slots[wr_mas->type]) {
>>> +        mas->store_type = wr_split_store;
>>> +        return;
>>> +    }
>>> +
>>> +    if (!mt_in_rcu(mas->tree) && (mas->offset == mas->end)) {
>>> +        mas->store_type = wr_append;
>>> +        return;
>>> +    }
>>> +
>>> +    if ((new_end == mas->end) && (!mt_in_rcu(mas->tree) ||
>>> +        (wr_mas->offset_end - mas->offset == 1))) {
>>> +        mas->store_type = wr_slot_store;
>>> +        return;
>>> +    }
>>> +
>>> +    if (mte_is_root(mas->node) || (new_end >= 
>>> mt_min_slots[wr_mas->type]) ||
>>> +        (mas->mas_flags & MA_STATE_BULK)) {
>> The check (new_end >= mt_min_slots[wr_mas->type]) here seems always 
>> be true.
>>
>> So the if here seems not necessary. Do I miss something?
>
> It is true that at this point new_end >= mt_min_slots[wr_mas->type] 
> must be true but if we remove that check we won't catch this 
> wr_node_store case if !mte_is_root() and !(mas->mas_flags & 
> MA_STATE_BULK).
>
> We could change the default store type to be wr_node_store and get rid 
> of that whole if statement entirely.
>
> This diff passes the tests:
>
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c index 
> 4f34e50c92b5..2ae0c4da9d74 100644 --- a/lib/maple_tree.c +++ 
> b/lib/maple_tree.c @@ -4242,14 +4242,7 @@ static inline void 
> mas_wr_store_type(struct ma_wr_state *wr_mas) return; } - if 
> (mte_is_root(mas->node) || (new_end >= mt_min_slots[wr_mas->type]) || 
> - (mas->mas_flags & MA_STATE_BULK)) { - mas->store_type = 
> wr_node_store; - return; - } - - mas->store_type = wr_invalid; - 
> MAS_WARN_ON(mas, 1); + mas->store_type = wr_node_store; }
>
> do you think this makes sense?
>
Sorry this diff wasn't formatted correctly, it should look normal now:

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index 4f34e50c92b5..2ae0c4da9d74 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -4242,14 +4242,7 @@ static inline void mas_wr_store_type(struct 
ma_wr_state *wr_mas)
                 return;
         }

-       if (mte_is_root(mas->node) || (new_end >= 
mt_min_slots[wr_mas->type]) ||
-               (mas->mas_flags & MA_STATE_BULK)) {
-               mas->store_type = wr_node_store;
-               return;
-       }
-
-       mas->store_type = wr_invalid;
-       MAS_WARN_ON(mas, 1);
+       mas->store_type = wr_node_store;
  }

> Thanks,
>
> Sid
>
>>> +        mas->store_type = wr_node_store;
>>> +        return;
>>> +    }
>>> +
>>> +    mas->store_type = wr_invalid;
>>> +    MAS_WARN_ON(mas, 1);
>>> +}
>>> +