Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <75ebddd7-641f-0274-72c5-415007d60cdd@bytedance.com>
Date:   Wed, 26 Apr 2023 12:14:52 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.9.1
Subject: Re: [PATCH 02/34] maple_tree: Clean up mas_parent_enum()
To:     "Liam R. Howlett" <Liam.Howlett@oracle.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        maple-tree@lists.infradead.org,
        Wei Yang <richard.weiyang@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>
References: <20230425140955.3834476-1-Liam.Howlett@oracle.com>
 <20230425140955.3834476-3-Liam.Howlett@oracle.com>
From:   Peng Zhang <zhangpeng.00@bytedance.com>
In-Reply-To: <20230425140955.3834476-3-Liam.Howlett@oracle.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 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);