linux-kernel - Re: [PATCH 4/4] maple_tree: Simplify the code of mas_mab

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230310184501.6cdc34ask7fqelxq@revolver>
Date:   Fri, 10 Mar 2023 13:45:01 -0500
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Peng Zhang <zhangpeng.00@...edance.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        maple-tree@...ts.infradead.org
Subject: Re: [PATCH 4/4] maple_tree: Simplify the code of mas_mab_cp()

* Peng Zhang <zhangpeng.00@...edance.com> [230310 09:09]:
> Simplify the code of mas_mab_cp(), and improve readability.
> No change in functionality.
> 
> Signed-off-by: Peng Zhang <zhangpeng.00@...edance.com>
> ---
>  lib/maple_tree.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> index de43ff19da72..688b062728a2 100644
> --- a/lib/maple_tree.c
> +++ b/lib/maple_tree.c
> @@ -1914,32 +1914,18 @@ static inline void mas_mab_cp(struct ma_state *mas, unsigned char mas_start,
>  	void __rcu **slots;
>  	unsigned long *pivots, *gaps;
>  	int i = mas_start, j = mab_start;
> -	unsigned char piv_end;
>  
>  	node = mas_mn(mas);
>  	mt = mte_node_type(mas->node);
>  	pivots = ma_pivots(node, mt);
> -	if (!i) {
> -		b_node->pivot[j] = pivots[i++];
> -		if (unlikely(i > mas_end))
> -			goto complete;
> -		j++;
> -	}
>  
> -	piv_end = min(mas_end, mt_pivots[mt]);
> -	for (; i < piv_end; i++, j++) {
> -		b_node->pivot[j] = pivots[i];
> -		if (unlikely(!b_node->pivot[j]))
> +	for (; i < min(mas_end, mt_pivots[mt]); i++, j++) {

Please don't inline the min here, it is not improving readability.

> +		if (unlikely(!pivots[i] && i) ||
> +		    unlikely(mas->max == pivots[i]))

By not doing the special case outside the loop, you have added a check
to every loop iteration.  I took these special cases out after profiling
the code with perf.  I get they aren't as easy to read but they are
faster which is important for something executed this much.

You have also made this if statement more complex which is not improving
readability.

>  			break;
> -
> -		if (unlikely(mas->max == b_node->pivot[j]))
> -			goto complete;
> +		b_node->pivot[j] = pivots[i];
>  	}
> -
> -	if (likely(i <= mas_end))
> -		b_node->pivot[j] = mas_safe_pivot(mas, pivots, i, mt);
> -
> -complete:
> +	b_node->pivot[j] = mas_safe_pivot(mas, pivots, i, mt);
>  	b_node->b_end = ++j;
>  	j -= mab_start;
>  	slots = ma_slots(node, mt);
> -- 
> 2.20.1
>