Re: [PATCH 1/3] maple_tree: use ma_data_end() in mas_data_end()

Wei Yang
2024-09-04T14:53:11Z

On Wed, Sep 04, 2024 at 07:58:19AM +0000, Wei Yang wrote:
[...]
>>It is only changing code for the sake of changing code. And it looks
>>like it will be slower, or the same speed if we are lucky. I have to
>>take time to verify things aren't slower or add subtle issues (maybe an
>>RCU race) because the code looked similar. It's just not worth it.
>>
>
>I am trying to make the code more easy to read, but seems not helping.
>
>BTW, I found in mas_update_gap(), if (p_gap != max_gap), we would access
>the first parent, parent's type and its gap twice. Once in mas_update_gap()
>and once in mas_parent_gap().
>
>Do you think it worth a change to reduce one?
>

Liam,

I am trying to understand what kind code change you don't like.

Is the following change worth?

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index 2b310dd3addf..e331d086eb7c 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -1595,32 +1595,33 @@ static inline unsigned long mas_max_gap(struct ma_state *mas)
/*
* mas_parent_gap() - Set the parent gap and any gaps above, as needed
* @mas: The maple state
- * @offset: The gap offset in the parent to set
* @new: The new gap value.
*
* Set the parent gap then continue to set the gap upwards, using the metadata
* of the parent to see if it is necessary to check the node above.
*/
-static inline void mas_parent_gap(struct ma_state *mas, unsigned char offset,
- unsigned long new)
+static inline void mas_parent_gap(struct ma_state *mas, unsigned long new)
{
unsigned long meta_gap = 0;
struct maple_node *pnode;
- struct maple_enode *penode;
+ struct maple_enode *enode = mas->node;
unsigned long *pgaps;
- unsigned char meta_offset;
+ unsigned char offset, meta_offset;
enum maple_type pmt;

- pnode = mte_parent(mas->node);
- pmt = mas_parent_type(mas, mas->node);
- penode = mt_mk_node(pnode, pmt);
+ascend:
+ pnode = mte_parent(enode);
+ pmt = mas_parent_type(mas, enode);
+ offset = mte_parent_slot(enode);
pgaps = ma_gaps(pnode, pmt);

-ascend:
MAS_BUG_ON(mas, pmt != maple_arange_64);
meta_offset = ma_meta_gap(pnode);
meta_gap = pgaps[meta_offset];

+ if (pgaps[offset] == new)
+ return;
+
pgaps[offset] = new;

if (meta_gap == new)
@@ -1640,11 +1641,7 @@ static inline void mas_parent_gap(struct ma_state *mas, unsigned char offset,
return;

/* Go to the parent node. */
- pnode = mte_parent(penode);
- pmt = mas_parent_type(mas, penode);
- pgaps = ma_gaps(pnode, pmt);
- offset = mte_parent_slot(penode);
- penode = mt_mk_node(pnode, pmt);
+ enode = mt_mk_node(pnode, pmt);
goto ascend;
}

@@ -1654,24 +1651,13 @@ static inline void mas_parent_gap(struct ma_state *mas, unsigned char offset,
*/
static inline void mas_update_gap(struct ma_state *mas)
{
- unsigned char pslot;
- unsigned long p_gap;
- unsigned long max_gap;
-
if (!mt_is_alloc(mas->tree))
return;

if (mte_is_root(mas->node))
return;

- max_gap = mas_max_gap(mas);
-
- pslot = mte_parent_slot(mas->node);
- p_gap = ma_gaps(mte_parent(mas->node),
- mas_parent_type(mas, mas->node))[pslot];
-
- if (p_gap != max_gap)
- mas_parent_gap(mas, pslot, max_gap);
+ mas_parent_gap(mas, mas_max_gap(mas));
}

/*
--
2.34.1

--
Wei Yang
Help you, Help me