linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] mm/page_alloc: fix documentation error and remove magic numbers
@ 2020-06-24 16:49 Joel Savitz
  2020-06-24 17:01 ` Matthew Wilcox
  0 siblings, 1 reply; 3+ messages in thread
From: Joel Savitz @ 2020-06-24 16:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Savitz, Vlastimil Babka, John Hubbard, Andrew Morton,
	Rafael Aquini, Fabrizio D'Angelo, 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.

changes from v1:
- declare constants via enum instead of separate integers

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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 48eb0f1410d4..733c81678b0e 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,8 @@ void setup_per_zone_wmarks(void)
  * 8192MB:	11584k
  * 16384MB:	16384k
  */
+static enum { MIN_FREE_KBYTES_LOWER_BOUND = 1 << 7, MIN_FREE_KBYTES_UPPER_BOUND = 1 << 18 };
+
 int __meminit init_per_zone_wmark_min(void)
 {
 	unsigned long lowmem_kbytes;
@@ -7862,10 +7864,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] 3+ messages in thread

* Re: [PATCH v2] mm/page_alloc: fix documentation error and remove magic numbers
  2020-06-24 16:49 [PATCH v2] mm/page_alloc: fix documentation error and remove magic numbers Joel Savitz
@ 2020-06-24 17:01 ` Matthew Wilcox
  2020-06-24 18:56   ` Andrew Morton
  0 siblings, 1 reply; 3+ messages in thread
From: Matthew Wilcox @ 2020-06-24 17:01 UTC (permalink / raw)
  To: Joel Savitz
  Cc: linux-kernel, Vlastimil Babka, John Hubbard, Andrew Morton,
	Rafael Aquini, Fabrizio D'Angelo, linux-mm

On Wed, Jun 24, 2020 at 12:49:43PM -0400, Joel Savitz wrote:
> 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.
> 
> changes from v1:
> - declare constants via enum instead of separate integers
> 
> Suggested-by: John Hubbard <jhubbard@nvidia.com>
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Joel Savitz <jsavitz@redhat.com>

NAK.  Just fix the documentation.  It's bad form to combine two completely
different things in one patch anyway.


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

* Re: [PATCH v2] mm/page_alloc: fix documentation error and remove magic numbers
  2020-06-24 17:01 ` Matthew Wilcox
@ 2020-06-24 18:56   ` Andrew Morton
  0 siblings, 0 replies; 3+ messages in thread
From: Andrew Morton @ 2020-06-24 18:56 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Joel Savitz, linux-kernel, Vlastimil Babka, John Hubbard,
	Rafael Aquini, Fabrizio D'Angelo, linux-mm

On Wed, 24 Jun 2020 18:01:31 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Wed, Jun 24, 2020 at 12:49:43PM -0400, Joel Savitz wrote:
> > 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.
> > 
> > changes from v1:
> > - declare constants via enum instead of separate integers
> > 
> > Suggested-by: John Hubbard <jhubbard@nvidia.com>
> > Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> > Signed-off-by: Joel Savitz <jsavitz@redhat.com>
> 
> NAK.  Just fix the documentation.  It's bad form to combine two completely
> different things in one patch anyway.

I don't think the enums add value anyway - if these numbers are only
used in a single place, it's clearer to just open-code them.

One advantage of using a #define/enum for single-use constants is that
the #define/enum site provides a neat location for documenting
(commenting) the reasoning behind the particular values which were
chosen.  But this patch didn't do that.



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

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

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-24 16:49 [PATCH v2] mm/page_alloc: fix documentation error and remove magic numbers Joel Savitz
2020-06-24 17:01 ` Matthew Wilcox
2020-06-24 18:56   ` Andrew Morton

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