linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] clean up setup_per_zone_pages_min
@ 2009-05-20  7:18 Minchan Kim
  2009-05-20  7:23 ` KOSAKI Motohiro
  2009-05-20  8:54 ` Mel Gorman
  0 siblings, 2 replies; 9+ messages in thread
From: Minchan Kim @ 2009-05-20  7:18 UTC (permalink / raw)
  To: Andrew Morton, LKML, linux-mm
  Cc: Mel Gorman, KOSAKI Motohiro, Rik van Riel, Johannes Weiner


Mel changed zone->pages_[high/low/min] with zone->watermark array.
So, setup_per_zone_pages_min also have to be changed.

CC: Mel Gorman <mel@csn.ul.ie>
Signed-off-by: Minchan kim <minchan.kim@gmail.com>
---
 include/linux/mm.h  |    2 +-
 mm/memory_hotplug.c |    2 +-
 mm/page_alloc.c     |   14 +++++++-------
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index a569862..1b2cb16 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1058,7 +1058,7 @@ extern int __meminit __early_pfn_to_nid(unsigned long pfn);
 extern void set_dma_reserve(unsigned long new_dma_reserve);
 extern void memmap_init_zone(unsigned long, int, unsigned long,
 				unsigned long, enum memmap_context);
-extern void setup_per_zone_pages_min(void);
+extern void setup_per_zone_wmark_min(void);
 extern void mem_init(void);
 extern void __init mmap_init(void);
 extern void show_mem(void);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index c083cf5..40bf385 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -422,7 +422,7 @@ int online_pages(unsigned long pfn, unsigned long nr_pages)
 	zone->present_pages += onlined_pages;
 	zone->zone_pgdat->node_present_pages += onlined_pages;
 
-	setup_per_zone_pages_min();
+	setup_per_zone_wmark_min();
 	if (onlined_pages) {
 		kswapd_run(zone_to_nid(zone));
 		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 9c712f0..273526b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4471,12 +4471,12 @@ static void setup_per_zone_lowmem_reserve(void)
 }
 
 /**
- * setup_per_zone_pages_min - called when min_free_kbytes changes.
+ * setup_per_zone_wmark_min - called when min_free_kbytes changes.
  *
- * Ensures that the pages_{min,low,high} values for each zone are set correctly
+ * Ensures that the watermark[min,low,high] values for each zone are set correctly
  * with respect to min_free_kbytes.
  */
-void setup_per_zone_pages_min(void)
+void setup_per_zone_wmark_min(void)
 {
 	unsigned long pages_min = min_free_kbytes >> (PAGE_SHIFT - 10);
 	unsigned long lowmem_pages = 0;
@@ -4594,7 +4594,7 @@ static void __init setup_per_zone_inactive_ratio(void)
  * 8192MB:	11584k
  * 16384MB:	16384k
  */
-static int __init init_per_zone_pages_min(void)
+static int __init init_per_zone_wmark_min(void)
 {
 	unsigned long lowmem_kbytes;
 
@@ -4605,12 +4605,12 @@ static int __init init_per_zone_pages_min(void)
 		min_free_kbytes = 128;
 	if (min_free_kbytes > 65536)
 		min_free_kbytes = 65536;
-	setup_per_zone_pages_min();
+	setup_per_zone_wmark_min();
 	setup_per_zone_lowmem_reserve();
 	setup_per_zone_inactive_ratio();
 	return 0;
 }
-module_init(init_per_zone_pages_min)
+module_init(init_per_zone_wmark_min)
 
 /*
  * min_free_kbytes_sysctl_handler - just a wrapper around proc_dointvec() so 
@@ -4622,7 +4622,7 @@ int min_free_kbytes_sysctl_handler(ctl_table *table, int write,
 {
 	proc_dointvec(table, write, file, buffer, length, ppos);
 	if (write)
-		setup_per_zone_pages_min();
+		setup_per_zone_wmark_min();
 	return 0;
 }
 
-- 
1.5.4.3



-- 
Kinds Regards
Minchan Kim

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

* Re: [PATCH 1/3] clean up setup_per_zone_pages_min
  2009-05-20  7:18 [PATCH 1/3] clean up setup_per_zone_pages_min Minchan Kim
@ 2009-05-20  7:23 ` KOSAKI Motohiro
  2009-05-20  7:26   ` Minchan Kim
  2009-05-20  8:54 ` Mel Gorman
  1 sibling, 1 reply; 9+ messages in thread
From: KOSAKI Motohiro @ 2009-05-20  7:23 UTC (permalink / raw)
  To: Minchan Kim
  Cc: kosaki.motohiro, Andrew Morton, LKML, linux-mm, Mel Gorman,
	Rik van Riel, Johannes Weiner

> 
> Mel changed zone->pages_[high/low/min] with zone->watermark array.
> So, setup_per_zone_pages_min also have to be changed.

Only naming change?
if so, the description sould talk about this explicitly.

 - kosaki

> 
> CC: Mel Gorman <mel@csn.ul.ie>
> Signed-off-by: Minchan kim <minchan.kim@gmail.com>
> ---
>  include/linux/mm.h  |    2 +-
>  mm/memory_hotplug.c |    2 +-
>  mm/page_alloc.c     |   14 +++++++-------
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a569862..1b2cb16 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1058,7 +1058,7 @@ extern int __meminit __early_pfn_to_nid(unsigned long pfn);
>  extern void set_dma_reserve(unsigned long new_dma_reserve);
>  extern void memmap_init_zone(unsigned long, int, unsigned long,
>  				unsigned long, enum memmap_context);
> -extern void setup_per_zone_pages_min(void);
> +extern void setup_per_zone_wmark_min(void);
>  extern void mem_init(void);
>  extern void __init mmap_init(void);
>  extern void show_mem(void);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c083cf5..40bf385 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -422,7 +422,7 @@ int online_pages(unsigned long pfn, unsigned long nr_pages)
>  	zone->present_pages += onlined_pages;
>  	zone->zone_pgdat->node_present_pages += onlined_pages;
>  
> -	setup_per_zone_pages_min();
> +	setup_per_zone_wmark_min();
>  	if (onlined_pages) {
>  		kswapd_run(zone_to_nid(zone));
>  		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 9c712f0..273526b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4471,12 +4471,12 @@ static void setup_per_zone_lowmem_reserve(void)
>  }
>  
>  /**
> - * setup_per_zone_pages_min - called when min_free_kbytes changes.
> + * setup_per_zone_wmark_min - called when min_free_kbytes changes.
>   *
> - * Ensures that the pages_{min,low,high} values for each zone are set correctly
> + * Ensures that the watermark[min,low,high] values for each zone are set correctly
>   * with respect to min_free_kbytes.
>   */
> -void setup_per_zone_pages_min(void)
> +void setup_per_zone_wmark_min(void)
>  {
>  	unsigned long pages_min = min_free_kbytes >> (PAGE_SHIFT - 10);
>  	unsigned long lowmem_pages = 0;
> @@ -4594,7 +4594,7 @@ static void __init setup_per_zone_inactive_ratio(void)
>   * 8192MB:	11584k
>   * 16384MB:	16384k
>   */
> -static int __init init_per_zone_pages_min(void)
> +static int __init init_per_zone_wmark_min(void)
>  {
>  	unsigned long lowmem_kbytes;
>  
> @@ -4605,12 +4605,12 @@ static int __init init_per_zone_pages_min(void)
>  		min_free_kbytes = 128;
>  	if (min_free_kbytes > 65536)
>  		min_free_kbytes = 65536;
> -	setup_per_zone_pages_min();
> +	setup_per_zone_wmark_min();
>  	setup_per_zone_lowmem_reserve();
>  	setup_per_zone_inactive_ratio();
>  	return 0;
>  }
> -module_init(init_per_zone_pages_min)
> +module_init(init_per_zone_wmark_min)
>  
>  /*
>   * min_free_kbytes_sysctl_handler - just a wrapper around proc_dointvec() so 
> @@ -4622,7 +4622,7 @@ int min_free_kbytes_sysctl_handler(ctl_table *table, int write,
>  {
>  	proc_dointvec(table, write, file, buffer, length, ppos);
>  	if (write)
> -		setup_per_zone_pages_min();
> +		setup_per_zone_wmark_min();
>  	return 0;
>  }
>  
> -- 
> 1.5.4.3
> 
> 
> 
> -- 
> Kinds Regards
> Minchan Kim




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

* Re: [PATCH 1/3] clean up setup_per_zone_pages_min
  2009-05-20  7:23 ` KOSAKI Motohiro
@ 2009-05-20  7:26   ` Minchan Kim
  0 siblings, 0 replies; 9+ messages in thread
From: Minchan Kim @ 2009-05-20  7:26 UTC (permalink / raw)
  To: KOSAKI Motohiro
  Cc: Minchan Kim, Andrew Morton, LKML, linux-mm, Mel Gorman,
	Rik van Riel, Johannes Weiner

On Wed, 20 May 2009 16:23:46 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > 
> > Mel changed zone->pages_[high/low/min] with zone->watermark array.
> > So, setup_per_zone_pages_min also have to be changed.
> 
> Only naming change?
> if so, the description sould talk about this explicitly.
> 
>  - kosaki
> 

OK. I will add it in next version. 
Thanks, Kosaki. 

-- 
Kinds Regards
Minchan Kim

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

* Re: [PATCH 1/3] clean up setup_per_zone_pages_min
  2009-05-20  7:18 [PATCH 1/3] clean up setup_per_zone_pages_min Minchan Kim
  2009-05-20  7:23 ` KOSAKI Motohiro
@ 2009-05-20  8:54 ` Mel Gorman
  2009-05-20  9:58   ` Minchan Kim
  1 sibling, 1 reply; 9+ messages in thread
From: Mel Gorman @ 2009-05-20  8:54 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Andrew Morton, LKML, linux-mm, KOSAKI Motohiro, Rik van Riel,
	Johannes Weiner

On Wed, May 20, 2009 at 04:18:53PM +0900, Minchan Kim wrote:
> 
> Mel changed zone->pages_[high/low/min] with zone->watermark array.
> So, setup_per_zone_pages_min also have to be changed.
> 

Just to be clear, this is a function renaming to match the new zone
field name, not something I missed. As the function changes min, low and
max, a better name might have been setup_per_zone_watermarks but whether
you go with that name or not, this is better than what is there so;

Acked-by: Mel Gorman <mel@csn.ul.ie>

> CC: Mel Gorman <mel@csn.ul.ie>
> Signed-off-by: Minchan kim <minchan.kim@gmail.com>
> ---
>  include/linux/mm.h  |    2 +-
>  mm/memory_hotplug.c |    2 +-
>  mm/page_alloc.c     |   14 +++++++-------
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a569862..1b2cb16 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1058,7 +1058,7 @@ extern int __meminit __early_pfn_to_nid(unsigned long pfn);
>  extern void set_dma_reserve(unsigned long new_dma_reserve);
>  extern void memmap_init_zone(unsigned long, int, unsigned long,
>  				unsigned long, enum memmap_context);
> -extern void setup_per_zone_pages_min(void);
> +extern void setup_per_zone_wmark_min(void);
>  extern void mem_init(void);
>  extern void __init mmap_init(void);
>  extern void show_mem(void);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index c083cf5..40bf385 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -422,7 +422,7 @@ int online_pages(unsigned long pfn, unsigned long nr_pages)
>  	zone->present_pages += onlined_pages;
>  	zone->zone_pgdat->node_present_pages += onlined_pages;
>  
> -	setup_per_zone_pages_min();
> +	setup_per_zone_wmark_min();
>  	if (onlined_pages) {
>  		kswapd_run(zone_to_nid(zone));
>  		node_set_state(zone_to_nid(zone), N_HIGH_MEMORY);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 9c712f0..273526b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4471,12 +4471,12 @@ static void setup_per_zone_lowmem_reserve(void)
>  }
>  
>  /**
> - * setup_per_zone_pages_min - called when min_free_kbytes changes.
> + * setup_per_zone_wmark_min - called when min_free_kbytes changes.
>   *
> - * Ensures that the pages_{min,low,high} values for each zone are set correctly
> + * Ensures that the watermark[min,low,high] values for each zone are set correctly
>   * with respect to min_free_kbytes.
>   */
> -void setup_per_zone_pages_min(void)
> +void setup_per_zone_wmark_min(void)
>  {
>  	unsigned long pages_min = min_free_kbytes >> (PAGE_SHIFT - 10);
>  	unsigned long lowmem_pages = 0;
> @@ -4594,7 +4594,7 @@ static void __init setup_per_zone_inactive_ratio(void)
>   * 8192MB:	11584k
>   * 16384MB:	16384k
>   */
> -static int __init init_per_zone_pages_min(void)
> +static int __init init_per_zone_wmark_min(void)
>  {
>  	unsigned long lowmem_kbytes;
>  
> @@ -4605,12 +4605,12 @@ static int __init init_per_zone_pages_min(void)
>  		min_free_kbytes = 128;
>  	if (min_free_kbytes > 65536)
>  		min_free_kbytes = 65536;
> -	setup_per_zone_pages_min();
> +	setup_per_zone_wmark_min();
>  	setup_per_zone_lowmem_reserve();
>  	setup_per_zone_inactive_ratio();
>  	return 0;
>  }
> -module_init(init_per_zone_pages_min)
> +module_init(init_per_zone_wmark_min)
>  
>  /*
>   * min_free_kbytes_sysctl_handler - just a wrapper around proc_dointvec() so 
> @@ -4622,7 +4622,7 @@ int min_free_kbytes_sysctl_handler(ctl_table *table, int write,
>  {
>  	proc_dointvec(table, write, file, buffer, length, ppos);
>  	if (write)
> -		setup_per_zone_pages_min();
> +		setup_per_zone_wmark_min();
>  	return 0;
>  }
>  

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

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

* Re: [PATCH 1/3] clean up setup_per_zone_pages_min
  2009-05-20  8:54 ` Mel Gorman
@ 2009-05-20  9:58   ` Minchan Kim
  2009-05-20 10:21     ` Mel Gorman
  0 siblings, 1 reply; 9+ messages in thread
From: Minchan Kim @ 2009-05-20  9:58 UTC (permalink / raw)
  To: Mel Gorman
  Cc: Minchan Kim, Andrew Morton, LKML, linux-mm, KOSAKI Motohiro,
	Rik van Riel, Johannes Weiner

Hi, Mel. 

On Wed, 20 May 2009 09:54:16 +0100
Mel Gorman <mel@csn.ul.ie> wrote:

> On Wed, May 20, 2009 at 04:18:53PM +0900, Minchan Kim wrote:
> > 
> > Mel changed zone->pages_[high/low/min] with zone->watermark array.
> > So, setup_per_zone_pages_min also have to be changed.
> > 
> 
> Just to be clear, this is a function renaming to match the new zone
> field name, not something I missed. As the function changes min, low and
> max, a better name might have been setup_per_zone_watermarks but whether

At first, I thouht, too. But It's handle of min_free_kbytes.
Documentation said, it's to compute a watermark[WMARK_MIN]. 
I think many people already used that knob to contorl pages_min to keep the 
low pages. 

So, I determined function name is proper now. 
If setup_per_zone_watermark is better than it, we also have to change with 
documentation. 

> you go with that name or not, this is better than what is there so;
> 
> Acked-by: Mel Gorman <mel@csn.ul.ie>


-- 
Kinds Regards
Minchan Kim

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

* Re: [PATCH 1/3] clean up setup_per_zone_pages_min
  2009-05-20  9:58   ` Minchan Kim
@ 2009-05-20 10:21     ` Mel Gorman
  2009-05-20 10:30       ` Minchan Kim
  0 siblings, 1 reply; 9+ messages in thread
From: Mel Gorman @ 2009-05-20 10:21 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Andrew Morton, LKML, linux-mm, KOSAKI Motohiro, Rik van Riel,
	Johannes Weiner

On Wed, May 20, 2009 at 06:58:03PM +0900, Minchan Kim wrote:
> Hi, Mel. 
> 
> On Wed, 20 May 2009 09:54:16 +0100
> Mel Gorman <mel@csn.ul.ie> wrote:
> 
> > On Wed, May 20, 2009 at 04:18:53PM +0900, Minchan Kim wrote:
> > > 
> > > Mel changed zone->pages_[high/low/min] with zone->watermark array.
> > > So, setup_per_zone_pages_min also have to be changed.
> > > 
> > 
> > Just to be clear, this is a function renaming to match the new zone
> > field name, not something I missed. As the function changes min, low and
> > max, a better name might have been setup_per_zone_watermarks but whether
> 
> At first, I thouht, too. But It's handle of min_free_kbytes.
> Documentation said, it's to compute a watermark[WMARK_MIN]. 
> I think many people already used that knob to contorl pages_min to keep the 
> low pages. 

Which documentation?

I'm looking at the function comment and see

 * setup_per_zone_pages_min - called when min_free_kbytes changes.
 *
 * Ensures that the pages_{min,low,high} values for each zone are set
 * correctly with respect to min_free_kbytes.

So, the values of all the watermarks are updated by that function depending
on what the new value of min_free_kbytes is. It is a bit wrong I suppose as
it missed memory hot-add

setup_per_zone_pages_min - called when min_free_kbytes changes or when memory is hot-added

> 
> So, I determined function name is proper now. 
> If setup_per_zone_watermark is better than it, we also have to change with 
> documentation. 
> 
> > you go with that name or not, this is better than what is there so;
> > 
> > Acked-by: Mel Gorman <mel@csn.ul.ie>
> 
> 
> -- 
> Kinds Regards
> Minchan Kim
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

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

* Re: [PATCH 1/3] clean up setup_per_zone_pages_min
  2009-05-20 10:21     ` Mel Gorman
@ 2009-05-20 10:30       ` Minchan Kim
  2009-05-20 10:47         ` Mel Gorman
  0 siblings, 1 reply; 9+ messages in thread
From: Minchan Kim @ 2009-05-20 10:30 UTC (permalink / raw)
  To: Mel Gorman
  Cc: Minchan Kim, Andrew Morton, LKML, linux-mm, KOSAKI Motohiro,
	Rik van Riel, Johannes Weiner

On Wed, 20 May 2009 11:21:29 +0100
Mel Gorman <mel@csn.ul.ie> wrote:

> On Wed, May 20, 2009 at 06:58:03PM +0900, Minchan Kim wrote:
> > Hi, Mel. 
> > 
> > On Wed, 20 May 2009 09:54:16 +0100
> > Mel Gorman <mel@csn.ul.ie> wrote:
> > 
> > > On Wed, May 20, 2009 at 04:18:53PM +0900, Minchan Kim wrote:
> > > > 
> > > > Mel changed zone->pages_[high/low/min] with zone->watermark array.
> > > > So, setup_per_zone_pages_min also have to be changed.
> > > > 
> > > 
> > > Just to be clear, this is a function renaming to match the new zone
> > > field name, not something I missed. As the function changes min, low and
> > > max, a better name might have been setup_per_zone_watermarks but whether
> > 
> > At first, I thouht, too. But It's handle of min_free_kbytes.
> > Documentation said, it's to compute a watermark[WMARK_MIN]. 
> > I think many people already used that knob to contorl pages_min to keep the 
> > low pages. 
> 
> Which documentation?

Documentation/sysctl/vm.txt - min_free_kbytes.

> I'm looking at the function comment and see
> 
>  * setup_per_zone_pages_min - called when min_free_kbytes changes.
>  *
>  * Ensures that the pages_{min,low,high} values for each zone are set
>  * correctly with respect to min_free_kbytes.
> 
> So, the values of all the watermarks are updated by that function depending
> on what the new value of min_free_kbytes is. It is a bit wrong I suppose as
> it missed memory hot-add
> 
> setup_per_zone_pages_min - called when min_free_kbytes changes or when memory is hot-added

God!. I changed this function's comments with memory hot plug. 
With my mistake, My patch 3/3 lost it. 
I will add comment about memory hotplug.

Thanks for pointing me out. :)


> > 
> > So, I determined function name is proper now. 
> > If setup_per_zone_watermark is better than it, we also have to change with 
> > documentation. 
> > 
> > > you go with that name or not, this is better than what is there so;
> > > 
> > > Acked-by: Mel Gorman <mel@csn.ul.ie>
> > 
> > 
> > -- 
> > Kinds Regards
> > Minchan Kim
> > 
> 
> -- 
> Mel Gorman
> Part-time Phd Student                          Linux Technology Center
> University of Limerick                         IBM Dublin Software Lab


-- 
Kinds Regards
Minchan Kim

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

* Re: [PATCH 1/3] clean up setup_per_zone_pages_min
  2009-05-20 10:30       ` Minchan Kim
@ 2009-05-20 10:47         ` Mel Gorman
  2009-05-20 10:53           ` Minchan Kim
  0 siblings, 1 reply; 9+ messages in thread
From: Mel Gorman @ 2009-05-20 10:47 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Andrew Morton, LKML, linux-mm, KOSAKI Motohiro, Rik van Riel,
	Johannes Weiner

On Wed, May 20, 2009 at 07:30:45PM +0900, Minchan Kim wrote:
> On Wed, 20 May 2009 11:21:29 +0100
> Mel Gorman <mel@csn.ul.ie> wrote:
> 
> > On Wed, May 20, 2009 at 06:58:03PM +0900, Minchan Kim wrote:
> > > Hi, Mel. 
> > > 
> > > On Wed, 20 May 2009 09:54:16 +0100
> > > Mel Gorman <mel@csn.ul.ie> wrote:
> > > 
> > > > On Wed, May 20, 2009 at 04:18:53PM +0900, Minchan Kim wrote:
> > > > > 
> > > > > Mel changed zone->pages_[high/low/min] with zone->watermark array.
> > > > > So, setup_per_zone_pages_min also have to be changed.
> > > > > 
> > > > 
> > > > Just to be clear, this is a function renaming to match the new zone
> > > > field name, not something I missed. As the function changes min, low and
> > > > max, a better name might have been setup_per_zone_watermarks but whether
> > > 
> > > At first, I thouht, too. But It's handle of min_free_kbytes.
> > > Documentation said, it's to compute a watermark[WMARK_MIN]. 
> > > I think many people already used that knob to contorl pages_min to keep the 
> > > low pages. 
> > 
> > Which documentation?
> 
> Documentation/sysctl/vm.txt - min_free_kbytes.
> 

That documentation states

==== 
This is used to force the Linux VM to keep a minimum number
of kilobytes free.  The VM uses this number to compute a pages_min
value for each lowmem zone in the system.  Each lowmem zone gets
a number of reserved free pages based proportionally on its size.
====

This is true. It just happens in the implementation that sets pages_min
(or it's renamed value) also sets the low and high watermarks are also set
based on the value of the minimum value. It doesn't need to be updated.

> > I'm looking at the function comment and see
> > 
> >  * setup_per_zone_pages_min - called when min_free_kbytes changes.
> >  *
> >  * Ensures that the pages_{min,low,high} values for each zone are set
> >  * correctly with respect to min_free_kbytes.
> > 
> > So, the values of all the watermarks are updated by that function depending
> > on what the new value of min_free_kbytes is. It is a bit wrong I suppose as
> > it missed memory hot-add
> > 
> > setup_per_zone_pages_min - called when min_free_kbytes changes or when memory is hot-added
> 
> God!. I changed this function's comments with memory hot plug. 
> With my mistake, My patch 3/3 lost it. 
> I will add comment about memory hotplug.
> 
> Thanks for pointing me out. :)
> 
> 
> > > 
> > > So, I determined function name is proper now. 
> > > If setup_per_zone_watermark is better than it, we also have to change with 
> > > documentation. 
> > > 
> > > > you go with that name or not, this is better than what is there so;
> > > > 
> > > > Acked-by: Mel Gorman <mel@csn.ul.ie>
> > > 
> > > 
> > > -- 
> > > Kinds Regards
> > > Minchan Kim
> > > 
> > 
> > -- 
> > Mel Gorman
> > Part-time Phd Student                          Linux Technology Center
> > University of Limerick                         IBM Dublin Software Lab
> 
> 
> -- 
> Kinds Regards
> Minchan Kim
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

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

* Re: [PATCH 1/3] clean up setup_per_zone_pages_min
  2009-05-20 10:47         ` Mel Gorman
@ 2009-05-20 10:53           ` Minchan Kim
  0 siblings, 0 replies; 9+ messages in thread
From: Minchan Kim @ 2009-05-20 10:53 UTC (permalink / raw)
  To: Mel Gorman
  Cc: Andrew Morton, LKML, linux-mm, KOSAKI Motohiro, Rik van Riel,
	Johannes Weiner

On Wed, May 20, 2009 at 7:47 PM, Mel Gorman <mel@csn.ul.ie> wrote:
> On Wed, May 20, 2009 at 07:30:45PM +0900, Minchan Kim wrote:
>> On Wed, 20 May 2009 11:21:29 +0100
>> Mel Gorman <mel@csn.ul.ie> wrote:
>>
>> > On Wed, May 20, 2009 at 06:58:03PM +0900, Minchan Kim wrote:
>> > > Hi, Mel.
>> > >
>> > > On Wed, 20 May 2009 09:54:16 +0100
>> > > Mel Gorman <mel@csn.ul.ie> wrote:
>> > >
>> > > > On Wed, May 20, 2009 at 04:18:53PM +0900, Minchan Kim wrote:
>> > > > >
>> > > > > Mel changed zone->pages_[high/low/min] with zone->watermark array.
>> > > > > So, setup_per_zone_pages_min also have to be changed.
>> > > > >
>> > > >
>> > > > Just to be clear, this is a function renaming to match the new zone
>> > > > field name, not something I missed. As the function changes min, low and
>> > > > max, a better name might have been setup_per_zone_watermarks but whether
>> > >
>> > > At first, I thouht, too. But It's handle of min_free_kbytes.
>> > > Documentation said, it's to compute a watermark[WMARK_MIN].
>> > > I think many people already used that knob to contorl pages_min to keep the
>> > > low pages.
>> >
>> > Which documentation?
>>
>> Documentation/sysctl/vm.txt - min_free_kbytes.
>>
>
> That documentation states
>
> ====
> This is used to force the Linux VM to keep a minimum number
> of kilobytes free.  The VM uses this number to compute a pages_min
> value for each lowmem zone in the system.  Each lowmem zone gets
> a number of reserved free pages based proportionally on its size.
> ====
>
> This is true. It just happens in the implementation that sets pages_min
> (or it's renamed value) also sets the low and high watermarks are also set
> based on the value of the minimum value. It doesn't need to be updated.
>

Okay.
I will modify function name with setup_per_zone_watermarks at next version.
Thanks for careful review, Mel.


-- 
Kinds regards,
Minchan Kim

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

end of thread, other threads:[~2009-05-20 10:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-05-20  7:18 [PATCH 1/3] clean up setup_per_zone_pages_min Minchan Kim
2009-05-20  7:23 ` KOSAKI Motohiro
2009-05-20  7:26   ` Minchan Kim
2009-05-20  8:54 ` Mel Gorman
2009-05-20  9:58   ` Minchan Kim
2009-05-20 10:21     ` Mel Gorman
2009-05-20 10:30       ` Minchan Kim
2009-05-20 10:47         ` Mel Gorman
2009-05-20 10:53           ` Minchan Kim

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