linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/page_alloc: fix documentation error and remove magic numbers
@ 2020-06-24  3:27 Joel Savitz
  2020-06-24 11:12 ` Matthew Wilcox
  0 siblings, 1 reply; 6+ messages in thread
From: Joel Savitz @ 2020-06-24  3:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Savitz, Vlastimil Babka, John Hubbard, Andrew Morton,
	Rafael Aquini, linux-mm

When I increased the upper bound of the min_free_kbytes value in
ee8eb9a5fe863, I forgot to tweak the above comment to reflect
the new value. This patch fixes that mistake.

In addition, this patch replaces the magic number bounds with symbolic
constants to clarify the logic.

Suggested-by: John Hubbard <jhubbard@nvidia.com>
Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Joel Savitz <jsavitz@redhat.com>
---
 mm/page_alloc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 48eb0f1410d4..f725addc2748 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7832,7 +7832,7 @@ void setup_per_zone_wmarks(void)
  * Initialise min_free_kbytes.
  *
  * For small machines we want it small (128k min).  For large machines
- * we want it large (64MB max).  But it is not linear, because network
+ * we want it large (256MB max).  But it is not linear, because network
  * bandwidth does not increase linearly with machine size.  We use
  *
  *	min_free_kbytes = 4 * sqrt(lowmem_kbytes), for better accuracy:
@@ -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;
 	} else {
 		pr_warn("min_free_kbytes is not updated to %d because user defined value %d is preferred\n",
 				new_min_free_kbytes, user_min_free_kbytes);
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/page_alloc: fix documentation error and remove magic numbers
  2020-06-24  3:27 [PATCH] mm/page_alloc: fix documentation error and remove magic numbers Joel Savitz
@ 2020-06-24 11:12 ` Matthew Wilcox
  2020-06-24 14:07   ` Rafael Aquini
  0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2020-06-24 11:12 UTC (permalink / raw)
  To: Joel Savitz
  Cc: linux-kernel, Vlastimil Babka, John Hubbard, Andrew Morton,
	Rafael Aquini, linux-mm

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!


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/page_alloc: fix documentation error and remove magic numbers
  2020-06-24 11:12 ` Matthew Wilcox
@ 2020-06-24 14:07   ` Rafael Aquini
  2020-06-24 14:09     ` Matthew Wilcox
  0 siblings, 1 reply; 6+ messages in thread
From: Rafael Aquini @ 2020-06-24 14:07 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Joel Savitz, linux-kernel, Vlastimil Babka, John Hubbard,
	Andrew Morton, linux-mm

On Wed, Jun 24, 2020 at 12:12:55PM +0100, Matthew Wilcox wrote:
> 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;
> > +

I think these constants would look better if declared as an enum.

> >  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!


> 



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/page_alloc: fix documentation error and remove magic numbers
  2020-06-24 14:07   ` Rafael Aquini
@ 2020-06-24 14:09     ` Matthew Wilcox
  2020-06-24 14:26       ` Rafael Aquini
  0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2020-06-24 14:09 UTC (permalink / raw)
  To: Rafael Aquini
  Cc: Joel Savitz, linux-kernel, Vlastimil Babka, John Hubbard,
	Andrew Morton, linux-mm

On Wed, Jun 24, 2020 at 10:07:27AM -0400, Rafael Aquini wrote:
> On Wed, Jun 24, 2020 at 12:12:55PM +0100, Matthew Wilcox wrote:
> > 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;
> > > +
> 
> I think these constants would look better if declared as an enum.

Why does having to look in two different places make the code clearer?



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/page_alloc: fix documentation error and remove magic numbers
  2020-06-24 14:09     ` Matthew Wilcox
@ 2020-06-24 14:26       ` Rafael Aquini
  2020-06-24 14:27         ` Matthew Wilcox
  0 siblings, 1 reply; 6+ messages in thread
From: Rafael Aquini @ 2020-06-24 14:26 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Joel Savitz, linux-kernel, Vlastimil Babka, John Hubbard,
	Andrew Morton, linux-mm

On Wed, Jun 24, 2020 at 03:09:58PM +0100, Matthew Wilcox wrote:
> On Wed, Jun 24, 2020 at 10:07:27AM -0400, Rafael Aquini wrote:
> > On Wed, Jun 24, 2020 at 12:12:55PM +0100, Matthew Wilcox wrote:
> > > 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;
> > > > +
> > 
> > I think these constants would look better if declared as an enum.
> 
> Why does having to look in two different places make the code clearer?
>

It might not make it clearer in this particular case, because it was
easy to take the meaning from the code, but it also doesn't make it
harder to read, so I don't have any strong opinion on this case. 

Joel's approach, however, makes sense if you consider it's generally a 
good practice to get rid of the unnamed magic numbers anti-pattern.

Cheers,
-- Rafael



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/page_alloc: fix documentation error and remove magic numbers
  2020-06-24 14:26       ` Rafael Aquini
@ 2020-06-24 14:27         ` Matthew Wilcox
  0 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2020-06-24 14:27 UTC (permalink / raw)
  To: Rafael Aquini
  Cc: Joel Savitz, linux-kernel, Vlastimil Babka, John Hubbard,
	Andrew Morton, linux-mm

On Wed, Jun 24, 2020 at 10:26:26AM -0400, Rafael Aquini wrote:
> Joel's approach, however, makes sense if you consider it's generally a 
> good practice to get rid of the unnamed magic numbers anti-pattern.

But I don't.  I care about how easy it is to understand the code, not
conforming to some bullshit trendy word salad.


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-06-24 14:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-24  3:27 [PATCH] mm/page_alloc: fix documentation error and remove magic numbers Joel Savitz
2020-06-24 11:12 ` Matthew Wilcox
2020-06-24 14:07   ` Rafael Aquini
2020-06-24 14:09     ` Matthew Wilcox
2020-06-24 14:26       ` Rafael Aquini
2020-06-24 14:27         ` Matthew Wilcox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).