linux-kernel - Re: [PATCH v2] mm/list_lru: optimize condition of exiting the loop

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>] [day] [month] [year] [list]

Message-ID: <f9da5b26-17b8-2d55-c725-215be4410368@suse.cz>
Date:   Mon, 2 Nov 2020 14:34:14 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Hui Su <sh_def@....com>, akpm@...ux-foundation.org,
        gustavo@...eddedor.com, songmuchun@...edance.com,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v2] mm/list_lru: optimize condition of exiting the loop

On 10/28/20 3:16 PM, Hui Su wrote:
> In list_lru_walk(), nr_to_walk type is 'unsigned long',
> so nr_to_walk won't be '< 0'.
> 
> In list_lru_walk_node(), nr_to_walk type is 'unsigned long',
> so *nr_to_walk won't be '< 0' too.
> 
> We can use '!nr_to_walk' instead of 'nr_to_walk <= 0', which
> is more precise.

Yes, imho comparisons that make no sense are only misleading for the readers. 
Compilers can probably find out easier, so maybe there's no code generation 
change, but for making it less misleading:

> Signed-off-by: Hui Su <sh_def@....com>

Acked-by: Vlastimil Babka <vbabka@...e.cz>

> ---
>   include/linux/list_lru.h | 2 +-
>   mm/list_lru.c            | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
> index 9dcaa3e582c9..b7bc4a2636b9 100644
> --- a/include/linux/list_lru.h
> +++ b/include/linux/list_lru.h
> @@ -214,7 +214,7 @@ list_lru_walk(struct list_lru *lru, list_lru_walk_cb isolate,
>   	for_each_node_state(nid, N_NORMAL_MEMORY) {
>   		isolated += list_lru_walk_node(lru, nid, isolate,
>   					       cb_arg, &nr_to_walk);
> -		if (nr_to_walk <= 0)
> +		if (!nr_to_walk)
>   			break;
>   	}
>   	return isolated;
> diff --git a/mm/list_lru.c b/mm/list_lru.c
> index 5aa6e44bc2ae..35be4de9fd77 100644
> --- a/mm/list_lru.c
> +++ b/mm/list_lru.c
> @@ -294,7 +294,7 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
>   
>   	isolated += list_lru_walk_one(lru, nid, NULL, isolate, cb_arg,
>   				      nr_to_walk);
> -	if (*nr_to_walk > 0 && list_lru_memcg_aware(lru)) {
> +	if (*nr_to_walk && list_lru_memcg_aware(lru)) {
>   		for_each_memcg_cache_index(memcg_idx) {
>   			struct list_lru_node *nlru = &lru->node[nid];
>   
> @@ -304,7 +304,7 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
>   							nr_to_walk);
>   			spin_unlock(&nlru->lock);
>   
> -			if (*nr_to_walk <= 0)
> +			if (!*nr_to_walk)
>   				break;
>   		}
>   	}
>