linux-kernel - Re: [PATCH] mm/page_alloc: fix documentation error and remove magic numbers

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: <20200624111255.GL21350@casper.infradead.org>
Date:   Wed, 24 Jun 2020 12:12:55 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Joel Savitz <jsavitz@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Vlastimil Babka <vbabka@...e.cz>,
        John Hubbard <jhubbard@...dia.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Rafael Aquini <aquini@...hat.com>, linux-mm@...ck.org
Subject: Re: [PATCH] mm/page_alloc: fix documentation error and remove magic
 numbers

On Tue, Jun 23, 2020 at 11:27:12PM -0400, Joel Savitz wrote:
> In addition, this patch replaces the magic number bounds with symbolic
> constants to clarify the logic.

Why do people think this kind of thing makes the code easier to read?
It actually makes it harder.  Unless the constants are used in more
than one place, just leave the numbers where they are.

> @@ -7852,6 +7852,9 @@ void setup_per_zone_wmarks(void)
>   * 8192MB:	11584k
>   * 16384MB:	16384k
>   */
> +static const int MIN_FREE_KBYTES_LOWER_BOUND = 1 << 7;
> +static const int MIN_FREE_KBYTES_UPPER_BOUND = 1 << 18;
> +
>  int __meminit init_per_zone_wmark_min(void)
>  {
>  	unsigned long lowmem_kbytes;
> @@ -7862,10 +7865,10 @@ int __meminit init_per_zone_wmark_min(void)
>  
>  	if (new_min_free_kbytes > user_min_free_kbytes) {
>  		min_free_kbytes = new_min_free_kbytes;
> -		if (min_free_kbytes < 128)
> -			min_free_kbytes = 128;
> -		if (min_free_kbytes > 262144)
> -			min_free_kbytes = 262144;
> +		if (min_free_kbytes < MIN_FREE_KBYTES_LOWER_BOUND)
> +			min_free_kbytes = MIN_FREE_KBYTES_LOWER_BOUND;
> +		if (min_free_kbytes > MIN_FREE_KBYTES_UPPER_BOUND)
> +			min_free_kbytes = MIN_FREE_KBYTES_UPPER_BOUND;

The only thing I'd consider changing there is replacing 262144 with 256
* 1024.  1 << 18 is not clearer!