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

[Wei Yang <richard.weiyang@xxxxxxxxx>]

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