mm/page_alloc: fix documentation error and remove magic numbers

PATCH Superseded Joel Savitz 3 participants 6 messages Jun 24, 2020

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Joel Savitz

author

2020-06-24 03:27 UTC #0 lore raw

PATCH 1/1 mm/page_alloc: fix documentation error and remove magic numbers

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.

Signed-off-by Joel Savitz <[email protected]>

---
 mm/page_alloc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

mm/page_alloc.c +8 -5

index 48eb0f1410d4..f725addc2748 100644
@@ -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);

Signature · 1 line

2.23.0

Matthew Wilcox

2020-06-24 11:13 UTC #1 lore raw

Quoted 2 lines · Joel Savitz

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.

Quoted 12 lines

mm/page_alloc.c +3 -0

@@ -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)

mm/page_alloc.c int __meminit init_per_zone_wmark_min(void) · line 7871

Quoted 10 lines

mm/page_alloc.c +4 -4

 	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!

Rafael Aquini

2020-06-24 14:07 UTC #2 lore raw

Quoted 8 lines · Matthew Wilcox

Quoted 2 lines · Joel Savitz
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.

Quoted 7 lines

mm/page_alloc.c void setup_per_zone_wmarks(void) · line 7857

Quoted 7 lines

mm/page_alloc.c +3 -0

@@ -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.

Quoted 21 lines

Quoted 5 lines

mm/page_alloc.c +0 -0

 int __meminit init_per_zone_wmark_min(void)
 {
 	unsigned long lowmem_kbytes;

@@ -7862,10 +7865,10 @@ int __meminit init_per_zone_wmark_min(void)

mm/page_alloc.c int __meminit init_per_zone_wmark_min(void) · line 7871

Quoted 10 lines

mm/page_alloc.c +4 -4

 	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!

Matthew Wilcox

2020-06-24 14:10 UTC #3 lore raw

Quoted 9 lines · Rafael Aquini

Quoted 8 lines · Matthew Wilcox
Quoted 2 lines · Joel Savitz
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.

Quoted 9 lines

Quoted 7 lines
mm/page_alloc.c void setup_per_zone_wmarks(void) · line 7857
Quoted 7 lines
mm/page_alloc.c +3 -0
@@ -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?

Rafael Aquini

2020-06-24 14:26 UTC #4 lore raw

Quoted 20 lines · Matthew Wilcox

Quoted 9 lines · Rafael Aquini
Quoted 8 lines · Matthew Wilcox
Quoted 2 lines · Joel Savitz
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.
Quoted 9 lines
Quoted 7 lines
mm/page_alloc.c void setup_per_zone_wmarks(void) · line 7857
Quoted 7 lines
mm/page_alloc.c +3 -0
@@ -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.

Quoted 2 lines

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

Matthew Wilcox

2020-06-24 14:27 UTC #5 lore raw

Quoted 2 lines · Rafael Aquini

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.