diff mbox series

[02/34] maple_tree: Clean up mas_parent_enum()

Message ID 20230425140955.3834476-3-Liam.Howlett@oracle.com (mailing list archive)
State New
Headers show
Series Maple tree mas_{next,prev}_range() and cleanup | expand

Commit Message

Liam R. Howlett April 25, 2023, 2:09 p.m. UTC
From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>

mas_parent_enum() is a simple wrapper for mte_parent_enum() which is
only called from that wrapper.  Remove the wrapper and inline
mte_parent_enum() into mas_parent_enum().

At the same time, clean up the bit masking of the root pointer since it
cannot be set by the time the bit masking occurs.  Change the check on
the root bit to a WARN_ON(), and fix the verification code to not
trigger the WARN_ON() before checking if the node is root.

Reported-by: Wei Yang <richard.weiyang@gmail.com>
Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
---
 lib/maple_tree.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

Comments

Wei Yang April 25, 2023, 4:13 p.m. UTC | #1
On Tue, Apr 25, 2023 at 10:09:23AM -0400, Liam R. Howlett wrote:
>From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
>
>mas_parent_enum() is a simple wrapper for mte_parent_enum() which is
>only called from that wrapper.  Remove the wrapper and inline
>mte_parent_enum() into mas_parent_enum().
>
>At the same time, clean up the bit masking of the root pointer since it
>cannot be set by the time the bit masking occurs.  Change the check on
>the root bit to a WARN_ON(), and fix the verification code to not
>trigger the WARN_ON() before checking if the node is root.
>
>Reported-by: Wei Yang <richard.weiyang@gmail.com>
>Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>

Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
Peng Zhang April 26, 2023, 4:14 a.m. UTC | #2
在 2023/4/25 22:09, Liam R. Howlett 写道:
> From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
>
> mas_parent_enum() is a simple wrapper for mte_parent_enum() which is
> only called from that wrapper.  Remove the wrapper and inline
> mte_parent_enum() into mas_parent_enum().
>
> At the same time, clean up the bit masking of the root pointer since it
> cannot be set by the time the bit masking occurs.  Change the check on
> the root bit to a WARN_ON(), and fix the verification code to not
> trigger the WARN_ON() before checking if the node is root.
>
> Reported-by: Wei Yang <richard.weiyang@gmail.com>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> ---
>   lib/maple_tree.c | 28 +++++++++++-----------------
>   1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> index 9cf4fca42310c..ac0245dd88dad 100644
> --- a/lib/maple_tree.c
> +++ b/lib/maple_tree.c
> @@ -428,25 +428,23 @@ static inline unsigned long mte_parent_slot_mask(unsigned long parent)
>    * mas_parent_enum() - Return the maple_type of the parent from the stored
>    * parent type.
>    * @mas: The maple state
> - * @node: The maple_enode to extract the parent's enum
> + * @enode: The maple_enode to extract the parent's enum
>    * Return: The node->parent maple_type
>    */
>   static inline
> -enum maple_type mte_parent_enum(struct maple_enode *p_enode,
> -				struct maple_tree *mt)
> +enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode)

Do you think it's better to rename this function to mas_parent_type()?
The meaning of enum is not obvious and there is already a similar
function mte_node_type().

>   {
>   	unsigned long p_type;
>   
> -	p_type = (unsigned long)p_enode;
> -	if (p_type & MAPLE_PARENT_ROOT)
> -		return 0; /* Validated in the caller. */
> +	p_type = (unsigned long)mte_to_node(enode)->parent;
> +	if (WARN_ON(p_type & MAPLE_PARENT_ROOT))
> +		return 0;
>   
>   	p_type &= MAPLE_NODE_MASK;
> -	p_type = p_type & ~(MAPLE_PARENT_ROOT | mte_parent_slot_mask(p_type));
> -
> +	p_type &= ~mte_parent_slot_mask(p_type);
>   	switch (p_type) {
>   	case MAPLE_PARENT_RANGE64: /* or MAPLE_PARENT_ARANGE64 */
> -		if (mt_is_alloc(mt))
> +		if (mt_is_alloc(mas->tree))
>   			return maple_arange_64;
>   		return maple_range_64;
>   	}
> @@ -454,12 +452,6 @@ enum maple_type mte_parent_enum(struct maple_enode *p_enode,
>   	return 0;
>   }
>   
> -static inline
> -enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode)
> -{
> -	return mte_parent_enum(ma_enode_ptr(mte_to_node(enode)->parent), mas->tree);
> -}
> -
>   /*
>    * mte_set_parent() - Set the parent node and encode the slot
>    * @enode: The encoded maple node.
> @@ -7008,14 +7000,16 @@ static void mas_validate_parent_slot(struct ma_state *mas)
>   {
>   	struct maple_node *parent;
>   	struct maple_enode *node;
> -	enum maple_type p_type = mas_parent_enum(mas, mas->node);
> -	unsigned char p_slot = mte_parent_slot(mas->node);
> +	enum maple_type p_type;
> +	unsigned char p_slot;
>   	void __rcu **slots;
>   	int i;
>   
>   	if (mte_is_root(mas->node))
>   		return;
>   
> +	p_slot = mte_parent_slot(mas->node);
> +	p_type = mas_parent_enum(mas, mas->node);
>   	parent = mte_parent(mas->node);
>   	slots = ma_slots(parent, p_type);
>   	MT_BUG_ON(mas->tree, mas_mn(mas) == parent);
Liam R. Howlett April 26, 2023, 9:07 p.m. UTC | #3
* Peng Zhang <zhangpeng.00@bytedance.com> [230426 00:15]:
> 
> 在 2023/4/25 22:09, Liam R. Howlett 写道:
> > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
> > 
> > mas_parent_enum() is a simple wrapper for mte_parent_enum() which is
> > only called from that wrapper.  Remove the wrapper and inline
> > mte_parent_enum() into mas_parent_enum().
> > 
> > At the same time, clean up the bit masking of the root pointer since it
> > cannot be set by the time the bit masking occurs.  Change the check on
> > the root bit to a WARN_ON(), and fix the verification code to not
> > trigger the WARN_ON() before checking if the node is root.
> > 
> > Reported-by: Wei Yang <richard.weiyang@gmail.com>
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > ---
> >   lib/maple_tree.c | 28 +++++++++++-----------------
> >   1 file changed, 11 insertions(+), 17 deletions(-)
> > 
> > diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> > index 9cf4fca42310c..ac0245dd88dad 100644
> > --- a/lib/maple_tree.c
> > +++ b/lib/maple_tree.c
> > @@ -428,25 +428,23 @@ static inline unsigned long mte_parent_slot_mask(unsigned long parent)
> >    * mas_parent_enum() - Return the maple_type of the parent from the stored
> >    * parent type.
> >    * @mas: The maple state
> > - * @node: The maple_enode to extract the parent's enum
> > + * @enode: The maple_enode to extract the parent's enum
> >    * Return: The node->parent maple_type
> >    */
> >   static inline
> > -enum maple_type mte_parent_enum(struct maple_enode *p_enode,
> > -				struct maple_tree *mt)
> > +enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode)
> 
> Do you think it's better to rename this function to mas_parent_type()?
> The meaning of enum is not obvious and there is already a similar
> function mte_node_type().

Yes, thanks. I'll make that change in v2.

> 
> >   {
> >   	unsigned long p_type;
> > -	p_type = (unsigned long)p_enode;
> > -	if (p_type & MAPLE_PARENT_ROOT)
> > -		return 0; /* Validated in the caller. */
> > +	p_type = (unsigned long)mte_to_node(enode)->parent;
> > +	if (WARN_ON(p_type & MAPLE_PARENT_ROOT))
> > +		return 0;
> >   	p_type &= MAPLE_NODE_MASK;
> > -	p_type = p_type & ~(MAPLE_PARENT_ROOT | mte_parent_slot_mask(p_type));
> > -
> > +	p_type &= ~mte_parent_slot_mask(p_type);
> >   	switch (p_type) {
> >   	case MAPLE_PARENT_RANGE64: /* or MAPLE_PARENT_ARANGE64 */
> > -		if (mt_is_alloc(mt))
> > +		if (mt_is_alloc(mas->tree))
> >   			return maple_arange_64;
> >   		return maple_range_64;
> >   	}
> > @@ -454,12 +452,6 @@ enum maple_type mte_parent_enum(struct maple_enode *p_enode,
> >   	return 0;
> >   }
> > -static inline
> > -enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode)
> > -{
> > -	return mte_parent_enum(ma_enode_ptr(mte_to_node(enode)->parent), mas->tree);
> > -}
> > -
> >   /*
> >    * mte_set_parent() - Set the parent node and encode the slot
> >    * @enode: The encoded maple node.
> > @@ -7008,14 +7000,16 @@ static void mas_validate_parent_slot(struct ma_state *mas)
> >   {
> >   	struct maple_node *parent;
> >   	struct maple_enode *node;
> > -	enum maple_type p_type = mas_parent_enum(mas, mas->node);
> > -	unsigned char p_slot = mte_parent_slot(mas->node);
> > +	enum maple_type p_type;
> > +	unsigned char p_slot;
> >   	void __rcu **slots;
> >   	int i;
> >   	if (mte_is_root(mas->node))
> >   		return;
> > +	p_slot = mte_parent_slot(mas->node);
> > +	p_type = mas_parent_enum(mas, mas->node);
> >   	parent = mte_parent(mas->node);
> >   	slots = ma_slots(parent, p_type);
> >   	MT_BUG_ON(mas->tree, mas_mn(mas) == parent);
diff mbox series

Patch

diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index 9cf4fca42310c..ac0245dd88dad 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -428,25 +428,23 @@  static inline unsigned long mte_parent_slot_mask(unsigned long parent)
  * mas_parent_enum() - Return the maple_type of the parent from the stored
  * parent type.
  * @mas: The maple state
- * @node: The maple_enode to extract the parent's enum
+ * @enode: The maple_enode to extract the parent's enum
  * Return: The node->parent maple_type
  */
 static inline
-enum maple_type mte_parent_enum(struct maple_enode *p_enode,
-				struct maple_tree *mt)
+enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode)
 {
 	unsigned long p_type;
 
-	p_type = (unsigned long)p_enode;
-	if (p_type & MAPLE_PARENT_ROOT)
-		return 0; /* Validated in the caller. */
+	p_type = (unsigned long)mte_to_node(enode)->parent;
+	if (WARN_ON(p_type & MAPLE_PARENT_ROOT))
+		return 0;
 
 	p_type &= MAPLE_NODE_MASK;
-	p_type = p_type & ~(MAPLE_PARENT_ROOT | mte_parent_slot_mask(p_type));
-
+	p_type &= ~mte_parent_slot_mask(p_type);
 	switch (p_type) {
 	case MAPLE_PARENT_RANGE64: /* or MAPLE_PARENT_ARANGE64 */
-		if (mt_is_alloc(mt))
+		if (mt_is_alloc(mas->tree))
 			return maple_arange_64;
 		return maple_range_64;
 	}
@@ -454,12 +452,6 @@  enum maple_type mte_parent_enum(struct maple_enode *p_enode,
 	return 0;
 }
 
-static inline
-enum maple_type mas_parent_enum(struct ma_state *mas, struct maple_enode *enode)
-{
-	return mte_parent_enum(ma_enode_ptr(mte_to_node(enode)->parent), mas->tree);
-}
-
 /*
  * mte_set_parent() - Set the parent node and encode the slot
  * @enode: The encoded maple node.
@@ -7008,14 +7000,16 @@  static void mas_validate_parent_slot(struct ma_state *mas)
 {
 	struct maple_node *parent;
 	struct maple_enode *node;
-	enum maple_type p_type = mas_parent_enum(mas, mas->node);
-	unsigned char p_slot = mte_parent_slot(mas->node);
+	enum maple_type p_type;
+	unsigned char p_slot;
 	void __rcu **slots;
 	int i;
 
 	if (mte_is_root(mas->node))
 		return;
 
+	p_slot = mte_parent_slot(mas->node);
+	p_type = mas_parent_enum(mas, mas->node);
 	parent = mte_parent(mas->node);
 	slots = ma_slots(parent, p_type);
 	MT_BUG_ON(mas->tree, mas_mn(mas) == parent);