linux-kernel - Re: [PATCH 2/2] mm/mmzone.h: simplify is_highmem

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

Message-ID: <df17080c-8867-259e-5a09-1f0ec053f68e@arm.com>
Date:   Mon, 26 Apr 2021 11:49:25 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Mike Rapoport <rppt@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Jonathan Corbet <corbet@....net>,
        Mike Rapoport <rppt@...ux.ibm.com>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH 2/2] mm/mmzone.h: simplify is_highmem_idx()



On 4/24/21 2:08 AM, Mike Rapoport wrote:
> From: Mike Rapoport <rppt@...ux.ibm.com>
> 
> There is a lot of historical ifdefery in is_highmem_idx() and its helper
> zone_movable_is_highmem() that was required because of two different paths
> for nodes and zones initialization that were selected at compile time.
> 
> Until commit 3f08a302f533 ("mm: remove CONFIG_HAVE_MEMBLOCK_NODE_MAP
> option") the movable_zone variable was only available for configurations
> that had CONFIG_HAVE_MEMBLOCK_NODE_MAP enabled so the test in
> zone_movable_is_highmem() used that variable only for such configurations.
> For other configurations the test checked if the index of ZONE_MOVABLE was
> greater by 1 than the index of ZONE_HIGMEM and then movable zone was
> considered a highmem zone. Needless to say, ZONE_MOVABLE - 1 equals
> ZONE_HIGMEM by definition when CONFIG_HIGHMEM=y.

Right with CONFIG_HIGHMEM is enabled, ZONE_MOVABLE = ZONE_HIGHMEM + 1 holds
always true from the very definition in enum zone_type { }.

> 
> Commit 3f08a302f533 ("mm: remove CONFIG_HAVE_MEMBLOCK_NODE_MAP option")
> made movable_zone variable always available. Since this variable is set to
> ZONE_HIGHMEM if CONFIG_HIGHMEM is enabled and highmem zone is populated, it
> is enough to check whether
> 
> 	zone_idx == ZONE_MOVABLE && movable_zone == ZONE_HIGMEM

Right. A small nit. s/HIGMEM/HIGHMEM

> 
> to test if zone index points to a highmem zone.

If the CONFIG_HIGHMEM is enabled but ZONE_HIGHMEM is not populated, then
movable_node would point to a zone below ZONE_HIGHMEM and is_highmem_idx()
would also return false.

> 
> Remove zone_movable_is_highmem() that is not used anywhere except
> is_highmem_idx() and use the test above in is_highmem_idx() instead.
> 
> Signed-off-by: Mike Rapoport <rppt@...ux.ibm.com>
> ---
>  include/linux/mmzone.h | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 3b2205741048..6a1ac643b65e 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -968,22 +968,11 @@ static inline void zone_set_nid(struct zone *zone, int nid) {}
>  
>  extern int movable_zone;
>  
> -#ifdef CONFIG_HIGHMEM
> -static inline int zone_movable_is_highmem(void)
> -{
> -#ifdef CONFIG_NEED_MULTIPLE_NODES
> -	return movable_zone == ZONE_HIGHMEM;
> -#else
> -	return (ZONE_MOVABLE - 1) == ZONE_HIGHMEM;
> -#endif
> -}
> -#endif
> -
>  static inline int is_highmem_idx(enum zone_type idx)
>  {
>  #ifdef CONFIG_HIGHMEM
>  	return (idx == ZONE_HIGHMEM ||
> -		(idx == ZONE_MOVABLE && zone_movable_is_highmem()));
> +		(idx == ZONE_MOVABLE && movable_zone == ZONE_HIGHMEM));
>  #else
>  	return 0;
>  #endif
> 

LGTM.

Reviewed-by: Anshuman Khandual <anshuman.khandual@....com>