Re: [PATCH 4/5] vmscan: Have kswapd sleep for a short interval and double check it should be asleep

[Mel Gorman <mel@csn.ul.ie>]

On Fri, Nov 13, 2009 at 07:43:09PM +0900, KOSAKI Motohiro wrote:
> > After kswapd balances all zones in a pgdat, it goes to sleep. In the event
> > of no IO congestion, kswapd can go to sleep very shortly after the high
> > watermark was reached. If there are a constant stream of allocations from
> > parallel processes, it can mean that kswapd went to sleep too quickly and
> > the high watermark is not being maintained for sufficient length time.
> > 
> > This patch makes kswapd go to sleep as a two-stage process. It first
> > tries to sleep for HZ/10. If it is woken up by another process or the
> > high watermark is no longer met, it's considered a premature sleep and
> > kswapd continues work. Otherwise it goes fully to sleep.
> > 
> > This adds more counters to distinguish between fast and slow breaches of
> > watermarks. A "fast" premature sleep is one where the low watermark was
> > hit in a very short time after kswapd going to sleep. A "slow" premature
> > sleep indicates that the high watermark was breached after a very short
> > interval.
> > 
> > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> 
> Why do you submit this patch to mainline? this is debugging patch
> no more and no less.
> 

Do you mean the stats part? The stats are included until such time as the page
allocator failure reports stop or are significantly reduced. In the event a
report is received, the value of the counters help determine if kswapd was
struggling or not. They should be removed once this mess is ironed out.

If there is a preference, I can split out the stats part and send it to
people with page allocator failure reports for retesting.

> 
> > ---
> >  include/linux/vmstat.h |    1 +
> >  mm/vmscan.c            |   44 ++++++++++++++++++++++++++++++++++++++++++--
> >  mm/vmstat.c            |    2 ++
> >  3 files changed, 45 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
> > index 2d0f222..9716003 100644
> > --- a/include/linux/vmstat.h
> > +++ b/include/linux/vmstat.h
> > @@ -40,6 +40,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
> >  		PGSCAN_ZONE_RECLAIM_FAILED,
> >  #endif
> >  		PGINODESTEAL, SLABS_SCANNED, KSWAPD_STEAL, KSWAPD_INODESTEAL,
> > +		KSWAPD_PREMATURE_FAST, KSWAPD_PREMATURE_SLOW,
> >  		PAGEOUTRUN, ALLOCSTALL, PGROTATED,
> 
> Please don't use the word of "premature" and "fast". it is too hard to understand the meanings.

How about KSWAPD_LOW_WMARK_HIT_QUICKLY and
KSWAPD_HIGH_WMARK_HIT_QUICKLY?


> Plus, please use per-zone stastics (like NUMA_HIT).
> 

Is that not overkill? The intention is just to track in general terms if
kswapd is struggling or not. I hadn't been considering the problem on a
per-zone basis to date.

> >
> >  #ifdef CONFIG_HUGETLB_PAGE
> >  		HTLB_BUDDY_PGALLOC, HTLB_BUDDY_PGALLOC_FAIL,
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 190bae1..ffa1766 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1904,6 +1904,24 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *mem_cont,
> >  }
> >  #endif
> >  
> > +/* is kswapd sleeping prematurely? */
> > +static int sleeping_prematurely(int order, long remaining)
> > +{
> > +	struct zone *zone;
> > +
> > +	/* If a direct reclaimer woke kswapd within HZ/10, it's premature */
> > +	if (remaining)
> > +		return 1;
> > +
> > +	/* If after HZ/10, a zone is below the high mark, it's premature */
> > +	for_each_populated_zone(zone)
> > +		if (!zone_watermark_ok(zone, order, high_wmark_pages(zone),
> > +								0, 0))
> > +			return 1;
> 
> for_each_populated_zone() iterate all populated zone. but kswapd shuld't see another node.
> 

Good spot.

How about the following?

==== CUT HERE ====
vmscan: Have kswapd sleep for a short interval and double check it should be asleep fix 1

This patch is a fix and a claritifacation to the patch "vmscan: Have
kswapd sleep for a short interval and double check it should be asleep".
The fix is for kswapd to only check zones in the node it is responsible
for. The clarification is to rename two counters to better explain what is
being counted.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
--- 
 include/linux/vmstat.h |    2 +-
 mm/vmscan.c            |   20 +++++++++++++-------
 mm/vmstat.c            |    4 ++--
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h
index 7d66695..0591a48 100644
--- a/include/linux/vmstat.h
+++ b/include/linux/vmstat.h
@@ -40,7 +40,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 		PGSCAN_ZONE_RECLAIM_FAILED,
 #endif
 		PGINODESTEAL, SLABS_SCANNED, KSWAPD_STEAL, KSWAPD_INODESTEAL,
-		KSWAPD_PREMATURE_FAST, KSWAPD_PREMATURE_SLOW,
+		KSWAPD_LOW_WMARK_HIT_QUICKLY, KSWAPD_HIGH_WMARK_HIT_QUICKLY,
 		KSWAPD_NO_CONGESTION_WAIT,
 		PAGEOUTRUN, ALLOCSTALL, PGROTATED,
 #ifdef CONFIG_HUGETLB_PAGE
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 70967e1..5557555 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1905,19 +1905,25 @@ unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *mem_cont,
 #endif
 
 /* is kswapd sleeping prematurely? */
-static int sleeping_prematurely(int order, long remaining)
+static int sleeping_prematurely(pg_data_t *pgdat, int order, long remaining)
 {
-	struct zone *zone;
+	int i;
 
 	/* If a direct reclaimer woke kswapd within HZ/10, it's premature */
 	if (remaining)
 		return 1;
 
 	/* If after HZ/10, a zone is below the high mark, it's premature */
-	for_each_populated_zone(zone)
+	for (i = 0; i < pgdat->nr_zones; i++) {
+		struct zone *zone = pgdat->node_zones + i;
+
+		if (!populated_zone(zone))
+			continue;
+
 		if (!zone_watermark_ok(zone, order, high_wmark_pages(zone),
 								0, 0))
 			return 1;
+	}
 
 	return 0;
 }
@@ -2221,7 +2227,7 @@ static int kswapd(void *p)
 				long remaining = 0;
 
 				/* Try to sleep for a short interval */
-				if (!sleeping_prematurely(order, remaining)) {
+				if (!sleeping_prematurely(pgdat, order, remaining)) {
 					remaining = schedule_timeout(HZ/10);
 					finish_wait(&pgdat->kswapd_wait, &wait);
 					prepare_to_wait(&pgdat->kswapd_wait, &wait, TASK_INTERRUPTIBLE);
@@ -2232,13 +2238,13 @@ static int kswapd(void *p)
 				 * premature sleep. If not, then go fully
 				 * to sleep until explicitly woken up
 				 */
-				if (!sleeping_prematurely(order, remaining))
+				if (!sleeping_prematurely(pgdat, order, remaining))
 					schedule();
 				else {
 					if (remaining)
-						count_vm_event(KSWAPD_PREMATURE_FAST);
+						count_vm_event(KSWAPD_LOW_WMARK_HIT_QUICKLY);
 					else
-						count_vm_event(KSWAPD_PREMATURE_SLOW);
+						count_vm_event(KSWAPD_HIGH_WMARK_HIT_QUICKLY);
 				}
 			}
 
diff --git a/mm/vmstat.c b/mm/vmstat.c
index bc09547..6cc8dc6 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -683,8 +683,8 @@ static const char * const vmstat_text[] = {
 	"slabs_scanned",
 	"kswapd_steal",
 	"kswapd_inodesteal",
-	"kswapd_slept_prematurely_fast",
-	"kswapd_slept_prematurely_slow",
+	"kswapd_low_wmark_hit_quickly",
+	"kswapd_high_wmark_hit_quickly",
 	"kswapd_no_congestion_wait",
 	"pageoutrun",
 	"allocstall",

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>