Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 24 Jun 2020 12:12:55 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Joel Savitz <jsavitz@redhat.com>
Cc:     linux-kernel@vger.kernel.org, Vlastimil Babka <vbabka@suse.cz>,
        John Hubbard <jhubbard@nvidia.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Rafael Aquini <aquini@redhat.com>, linux-mm@kvack.org
Subject: Re: [PATCH] mm/page_alloc: fix documentation error and remove magic
 numbers
Message-ID: <20200624111255.GL21350@casper.infradead.org>
References: <20200624032712.23263-1-jsavitz@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200624032712.23263-1-jsavitz@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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!