[PATCH 0/2] Reduce reclaim from per-zone LRU in global kswapd

[PATCH 0/2] Reduce reclaim from per-zone LRU in global kswapd

[Ying Han]

The global kswapd scans per-zone LRU and reclaims pages regardless of the
cgroup. It breaks memory isolation since one cgroup can end up reclaiming
pages from another cgroup. Instead we should rely on memcg-aware target
reclaim including per-memcg kswapd and soft_limit hierarchical reclaim under
memory pressure.

In the global background reclaim, we do soft reclaim before scanning the
per-zone LRU. However, the return value is ignored. This patch adds the logic
where no per-zone reclaim happens if the soft reclaim raise the free pages
above the zone's high_wmark.

This is part of the effort which tries to reduce reclaiming pages in global
LRU in memcg. The per-memcg background reclaim patchset further enhances the
per-cgroup targetting reclaim, which I should have V4 posted shortly.

Try running multiple memory intensive workloads within seperate memcgs. Watch
the counters for both soft_steal in memory.stat and skip_reclaim in zoneinfo.

$ egrep 'steal|scan' /dev/cgroup/A/memory.stat
soft_steal 304640
total_soft_steal 304640

$ egrep skip /proc/zoneinfo
nr_skip_reclaim_global 0
nr_skip_reclaim_global 381
nr_skip_reclaim_global 387

Ying Han (2):
  check the return value of soft_limit reclaim
  add two stats to monitor soft_limit reclaim.

 Documentation/cgroups/memory.txt |    2 ++
 include/linux/memcontrol.h       |    5 +++++
 include/linux/mmzone.h           |    1 +
 mm/memcontrol.c                  |   14 ++++++++++++++
 mm/vmscan.c                      |   16 +++++++++++++++-
 mm/vmstat.c                      |    1 +
 6 files changed, 38 insertions(+), 1 deletions(-)

-- 
1.7.3.1

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/2] check the return value of soft_limit reclaim

[Ying Han]

In the global background reclaim, we do soft reclaim before scanning the
per-zone LRU. However, the return value is ignored. This patch adds the logic
where no per-zone reclaim happens if the soft reclaim raise the free pages
above the zone's high_wmark.

I did notice a similar check exists but instead leaving a "gap" above the
high_wmark(the code right after my change in vmscan.c). There are discussions
on whether or not removing the "gap" which intends to balance pressures across
zones over time. Without fully understand the logic behind, I didn't try to
merge them into one, but instead adding the condition only for memcg users
who care a lot on memory isolation.

Signed-off-by: Ying Han <yinghan@google.com>
---
 mm/vmscan.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 060e4c1..e4601c5 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2320,6 +2320,7 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
 	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */
 	unsigned long total_scanned;
 	struct reclaim_state *reclaim_state = current->reclaim_state;
+	unsigned long nr_soft_reclaimed;
 	struct scan_control sc = {
 		.gfp_mask = GFP_KERNEL,
 		.may_unmap = 1,
@@ -2413,7 +2414,20 @@ loop_again:
 			 * Call soft limit reclaim before calling shrink_zone.
 			 * For now we ignore the return value
 			 */
-			mem_cgroup_soft_limit_reclaim(zone, order, sc.gfp_mask);
+			nr_soft_reclaimed = mem_cgroup_soft_limit_reclaim(zone,
+							order, sc.gfp_mask);
+
+			/*
+			 * Check the watermark after the soft limit reclaim. If
+			 * the free pages is above the watermark, no need to
+			 * proceed to the zone reclaim.
+			 */
+			if (nr_soft_reclaimed && zone_watermark_ok_safe(zone,
+					order, high_wmark_pages(zone),
+					end_zone, 0)) {
+				__inc_zone_state(zone, NR_SKIP_RECLAIM_GLOBAL);
+				continue;
+			}
 
 			/*
 			 * We put equal pressure on every zone, unless
-- 
1.7.3.1

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] add two stats to monitor soft_limit reclaim.

[Ying Han]

Two new stats are added:
1. /dev/cgroup/*/memory.stat
soft_steal:        - # of pages reclaimed from soft_limit hierarchical reclaim
total_soft_steal:  - # sum of all children's "soft_steal"

2. /proc/zoneinfo
nr_skip_reclaim_global:  - # number of times the zone skips reclaim

Signed-off-by: Ying Han <yinghan@google.com>
---
 Documentation/cgroups/memory.txt |    2 ++
 include/linux/memcontrol.h       |    5 +++++
 include/linux/mmzone.h           |    1 +
 mm/memcontrol.c                  |   14 ++++++++++++++
 mm/vmstat.c                      |    1 +
 5 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index b6ed61c..dcda6c5 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -385,6 +385,7 @@ mapped_file	- # of bytes of mapped file (includes tmpfs/shmem)
 pgpgin		- # of pages paged in (equivalent to # of charging events).
 pgpgout		- # of pages paged out (equivalent to # of uncharging events).
 swap		- # of bytes of swap usage
+soft_steal	- # of pages reclaimed from global hierarchical reclaim
 inactive_anon	- # of bytes of anonymous memory and swap cache memory on
 		LRU list.
 active_anon	- # of bytes of anonymous and swap cache memory on active
@@ -406,6 +407,7 @@ total_mapped_file	- sum of all children's "cache"
 total_pgpgin		- sum of all children's "pgpgin"
 total_pgpgout		- sum of all children's "pgpgout"
 total_swap		- sum of all children's "swap"
+total_soft_steal	- sum of all children's "soft_steal"
 total_inactive_anon	- sum of all children's "inactive_anon"
 total_active_anon	- sum of all children's "active_anon"
 total_inactive_file	- sum of all children's "inactive_file"
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 5a5ce70..06566d7 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -115,6 +115,7 @@ struct zone_reclaim_stat*
 mem_cgroup_get_reclaim_stat_from_page(struct page *page);
 extern void mem_cgroup_print_oom_info(struct mem_cgroup *memcg,
 					struct task_struct *p);
+void mem_cgroup_soft_steal(struct mem_cgroup *memcg, int val);
 
 #ifdef CONFIG_CGROUP_MEM_RES_CTLR_SWAP
 extern int do_swap_account;
@@ -354,6 +355,10 @@ static inline void mem_cgroup_split_huge_fixup(struct page *head,
 {
 }
 
+static inline void mem_cgroup_soft_steal(struct mem_cgroup *memcg,
+					 int val)
+{
+}
 #endif /* CONFIG_CGROUP_MEM_CONT */
 
 #if !defined(CONFIG_CGROUP_MEM_RES_CTLR) || !defined(CONFIG_DEBUG_VM)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 628f07b..53216fe 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -104,6 +104,7 @@ enum zone_stat_item {
 	NR_ISOLATED_ANON,	/* Temporary isolated pages from anon lru */
 	NR_ISOLATED_FILE,	/* Temporary isolated pages from file lru */
 	NR_SHMEM,		/* shmem pages (included tmpfs/GEM pages) */
+	NR_SKIP_RECLAIM_GLOBAL,	/* kswapd skip reclaim from global lru */
 	NR_DIRTIED,		/* page dirtyings since bootup */
 	NR_WRITTEN,		/* page writings since bootup */
 #ifdef CONFIG_NUMA
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4407dd0..07d84a7 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -94,6 +94,8 @@ enum mem_cgroup_events_index {
 	MEM_CGROUP_EVENTS_PGPGIN,	/* # of pages paged in */
 	MEM_CGROUP_EVENTS_PGPGOUT,	/* # of pages paged out */
 	MEM_CGROUP_EVENTS_COUNT,	/* # of pages paged in/out */
+	MEM_CGROUP_EVENTS_SOFT_STEAL,	/* # of pages reclaimed from */
+					/* oft reclaim               */
 	MEM_CGROUP_EVENTS_NSTATS,
 };
 /*
@@ -624,6 +626,11 @@ static void mem_cgroup_charge_statistics(struct mem_cgroup *mem,
 	preempt_enable();
 }
 
+void mem_cgroup_soft_steal(struct mem_cgroup *mem, int val)
+{
+	this_cpu_add(mem->stat->events[MEM_CGROUP_EVENTS_SOFT_STEAL], val);
+}
+
 static unsigned long mem_cgroup_get_local_zonestat(struct mem_cgroup *mem,
 					enum lru_list idx)
 {
@@ -3315,6 +3322,9 @@ unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order,
 						gfp_mask,
 						MEM_CGROUP_RECLAIM_SOFT);
 		nr_reclaimed += reclaimed;
+
+		mem_cgroup_soft_steal(mz->mem, reclaimed);
+
 		spin_lock(&mctz->lock);
 
 		/*
@@ -3772,6 +3782,7 @@ enum {
 	MCS_PGPGIN,
 	MCS_PGPGOUT,
 	MCS_SWAP,
+	MCS_SOFT_STEAL,
 	MCS_INACTIVE_ANON,
 	MCS_ACTIVE_ANON,
 	MCS_INACTIVE_FILE,
@@ -3794,6 +3805,7 @@ struct {
 	{"pgpgin", "total_pgpgin"},
 	{"pgpgout", "total_pgpgout"},
 	{"swap", "total_swap"},
+	{"soft_steal", "total_soft_steal"},
 	{"inactive_anon", "total_inactive_anon"},
 	{"active_anon", "total_active_anon"},
 	{"inactive_file", "total_inactive_file"},
@@ -3822,6 +3834,8 @@ mem_cgroup_get_local_stat(struct mem_cgroup *mem, struct mcs_total_stat *s)
 		val = mem_cgroup_read_stat(mem, MEM_CGROUP_STAT_SWAPOUT);
 		s->stat[MCS_SWAP] += val * PAGE_SIZE;
 	}
+	val = mem_cgroup_read_events(mem, MEM_CGROUP_EVENTS_SOFT_STEAL);
+	s->stat[MCS_SOFT_STEAL] += val;
 
 	/* per zone stat */
 	val = mem_cgroup_get_local_zonestat(mem, LRU_INACTIVE_ANON);
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 2bfc31f..0118ec4 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -874,6 +874,7 @@ static const char * const vmstat_text[] = {
 	"nr_isolated_anon",
 	"nr_isolated_file",
 	"nr_shmem",
+	"nr_skip_reclaim_global",
 	"nr_dirtied",
 	"nr_written",
 
-- 
1.7.3.1

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] check the return value of soft_limit reclaim

[KOSAKI Motohiro]

> In the global background reclaim, we do soft reclaim before scanning the
> per-zone LRU. However, the return value is ignored. This patch adds the logic
> where no per-zone reclaim happens if the soft reclaim raise the free pages
> above the zone's high_wmark.
> 
> I did notice a similar check exists but instead leaving a "gap" above the
> high_wmark(the code right after my change in vmscan.c). There are discussions
> on whether or not removing the "gap" which intends to balance pressures across
> zones over time. Without fully understand the logic behind, I didn't try to
> merge them into one, but instead adding the condition only for memcg users
> who care a lot on memory isolation.
> 
> Signed-off-by: Ying Han <yinghan@google.com>

Looks good to me. But this depend on "memcg soft limit" spec. To be honest,
I don't know this return value ignorance is intentional or not. So I think 
you need to get ack from memcg folks.


> ---
>  mm/vmscan.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 060e4c1..e4601c5 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2320,6 +2320,7 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
>  	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */
>  	unsigned long total_scanned;
>  	struct reclaim_state *reclaim_state = current->reclaim_state;
> +	unsigned long nr_soft_reclaimed;
>  	struct scan_control sc = {
>  		.gfp_mask = GFP_KERNEL,
>  		.may_unmap = 1,
> @@ -2413,7 +2414,20 @@ loop_again:
>  			 * Call soft limit reclaim before calling shrink_zone.
>  			 * For now we ignore the return value
>  			 */
> -			mem_cgroup_soft_limit_reclaim(zone, order, sc.gfp_mask);
> +			nr_soft_reclaimed = mem_cgroup_soft_limit_reclaim(zone,
> +							order, sc.gfp_mask);
> +
> +			/*
> +			 * Check the watermark after the soft limit reclaim. If
> +			 * the free pages is above the watermark, no need to
> +			 * proceed to the zone reclaim.
> +			 */
> +			if (nr_soft_reclaimed && zone_watermark_ok_safe(zone,
> +					order, high_wmark_pages(zone),
> +					end_zone, 0)) {
> +				__inc_zone_state(zone, NR_SKIP_RECLAIM_GLOBAL);

NR_SKIP_RECLAIM_GLOBAL is defined by patch 2/2. please don't break bisectability.



> +				continue;
> +			}
>  
>  			/*
>  			 * We put equal pressure on every zone, unless
> -- 
> 1.7.3.1
> 



--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] check the return value of soft_limit reclaim

[Daisuke Nishimura]

Hi,

This patch looks good to me, except for one nitpick.

On Sun, 27 Mar 2011 23:12:54 -0700
Ying Han <yinghan@google.com> wrote:

> In the global background reclaim, we do soft reclaim before scanning the
> per-zone LRU. However, the return value is ignored. This patch adds the logic
> where no per-zone reclaim happens if the soft reclaim raise the free pages
> above the zone's high_wmark.
> 
> I did notice a similar check exists but instead leaving a "gap" above the
> high_wmark(the code right after my change in vmscan.c). There are discussions
> on whether or not removing the "gap" which intends to balance pressures across
> zones over time. Without fully understand the logic behind, I didn't try to
> merge them into one, but instead adding the condition only for memcg users
> who care a lot on memory isolation.
> 
> Signed-off-by: Ying Han <yinghan@google.com>
> ---
>  mm/vmscan.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 060e4c1..e4601c5 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2320,6 +2320,7 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
>  	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */
>  	unsigned long total_scanned;
>  	struct reclaim_state *reclaim_state = current->reclaim_state;
> +	unsigned long nr_soft_reclaimed;
>  	struct scan_control sc = {
>  		.gfp_mask = GFP_KERNEL,
>  		.may_unmap = 1,
> @@ -2413,7 +2414,20 @@ loop_again:
>  			 * Call soft limit reclaim before calling shrink_zone.
>  			 * For now we ignore the return value

You should remove this comment too.

But, Balbir-san, do you remember why did you ignore the return value here ?

Thanks,
Daisuke Nishimura.

>  			 */
> -			mem_cgroup_soft_limit_reclaim(zone, order, sc.gfp_mask);
> +			nr_soft_reclaimed = mem_cgroup_soft_limit_reclaim(zone,
> +							order, sc.gfp_mask);
> +
> +			/*
> +			 * Check the watermark after the soft limit reclaim. If
> +			 * the free pages is above the watermark, no need to
> +			 * proceed to the zone reclaim.
> +			 */
> +			if (nr_soft_reclaimed && zone_watermark_ok_safe(zone,
> +					order, high_wmark_pages(zone),
> +					end_zone, 0)) {
> +				__inc_zone_state(zone, NR_SKIP_RECLAIM_GLOBAL);
> +				continue;
> +			}
>  
>  			/*
>  			 * We put equal pressure on every zone, unless
> -- 
> 1.7.3.1
> 
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] check the return value of soft_limit reclaim

[KAMEZAWA Hiroyuki]

On Mon, 28 Mar 2011 15:39:59 +0900 (JST)
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > In the global background reclaim, we do soft reclaim before scanning the
> > per-zone LRU. However, the return value is ignored. This patch adds the logic
> > where no per-zone reclaim happens if the soft reclaim raise the free pages
> > above the zone's high_wmark.
> > 
> > I did notice a similar check exists but instead leaving a "gap" above the
> > high_wmark(the code right after my change in vmscan.c). There are discussions
> > on whether or not removing the "gap" which intends to balance pressures across
> > zones over time. Without fully understand the logic behind, I didn't try to
> > merge them into one, but instead adding the condition only for memcg users
> > who care a lot on memory isolation.
> > 
> > Signed-off-by: Ying Han <yinghan@google.com>
> 
> Looks good to me. But this depend on "memcg soft limit" spec. To be honest,
> I don't know this return value ignorance is intentional or not. So I think 
> you need to get ack from memcg folks.
> 
> 
Hi,


> > ---
> >  mm/vmscan.c |   16 +++++++++++++++-
> >  1 files changed, 15 insertions(+), 1 deletions(-)
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 060e4c1..e4601c5 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -2320,6 +2320,7 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
> >  	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */
> >  	unsigned long total_scanned;
> >  	struct reclaim_state *reclaim_state = current->reclaim_state;
> > +	unsigned long nr_soft_reclaimed;
> >  	struct scan_control sc = {
> >  		.gfp_mask = GFP_KERNEL,
> >  		.may_unmap = 1,
> > @@ -2413,7 +2414,20 @@ loop_again:
> >  			 * Call soft limit reclaim before calling shrink_zone.
> >  			 * For now we ignore the return value
> >  			 */
> > -			mem_cgroup_soft_limit_reclaim(zone, order, sc.gfp_mask);
> > +			nr_soft_reclaimed = mem_cgroup_soft_limit_reclaim(zone,
> > +							order, sc.gfp_mask);
> > +
> > +			/*
> > +			 * Check the watermark after the soft limit reclaim. If
> > +			 * the free pages is above the watermark, no need to
> > +			 * proceed to the zone reclaim.
> > +			 */
> > +			if (nr_soft_reclaimed && zone_watermark_ok_safe(zone,
> > +					order, high_wmark_pages(zone),
> > +					end_zone, 0)) {
> > +				__inc_zone_state(zone, NR_SKIP_RECLAIM_GLOBAL);
> 
> NR_SKIP_RECLAIM_GLOBAL is defined by patch 2/2. please don't break bisectability.
> 
> 
> 
> > +				continue;
> > +			}

Hmm, this "continue" seems not good to me. And, IIUC, this was a reason
we ignore the result. But yes, ignore the result is bad.
I think you should just do sc.nr_reclaimed += nr_soft_reclaimed.
Or mem_cgroup_soft_limit_reclaim() should update sc.


And allow kswapd to do some jobs as
 - call shrink_slab()
 - update total_scanned
 - update other flags.. etc...etc..

If extra shink_zone() seems bad, please skip it, if mem_cgroup_soft_limit_reclaim()
did enough jobs.

IOW, mem_cgroup_soft_limit_reclaim() can't do enough jobs to satisfy
==
   2426 			balance_gap = min(low_wmark_pages(zone),
   2427 				(zone->present_pages +
   2428 					KSWAPD_ZONE_BALANCE_GAP_RATIO-1) /
   2429 				KSWAPD_ZONE_BALANCE_GAP_RATIO);
   2430 			if (!zone_watermark_ok_safe(zone, order,
   2431 					high_wmark_pages(zone) + balance_gap,
   2432 					end_zone, 0))
   2433 				shrink_zone(priority, zone, &sc);
==
This condition, you should update mem_cgroup_soft_limit_relcaim() to satisfy this,
rather than continue here.

I guess this is not easy...So, how about starting from updating 'sc' passed to 
mem_cgroup_soft_limit_reclaim() ? Then, we can think of algorithm.

Thanks,
-Kame

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] check the return value of soft_limit reclaim

[Minchan Kim]

On Mon, Mar 28, 2011 at 05:44:21PM +0900, KAMEZAWA Hiroyuki wrote:
> On Mon, 28 Mar 2011 15:39:59 +0900 (JST)
> KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
> 
> > > In the global background reclaim, we do soft reclaim before scanning the
> > > per-zone LRU. However, the return value is ignored. This patch adds the logic
> > > where no per-zone reclaim happens if the soft reclaim raise the free pages
> > > above the zone's high_wmark.
> > > 
> > > I did notice a similar check exists but instead leaving a "gap" above the
> > > high_wmark(the code right after my change in vmscan.c). There are discussions
> > > on whether or not removing the "gap" which intends to balance pressures across
> > > zones over time. Without fully understand the logic behind, I didn't try to
> > > merge them into one, but instead adding the condition only for memcg users
> > > who care a lot on memory isolation.
> > > 
> > > Signed-off-by: Ying Han <yinghan@google.com>
> > 
> > Looks good to me. But this depend on "memcg soft limit" spec. To be honest,
> > I don't know this return value ignorance is intentional or not. So I think 
> > you need to get ack from memcg folks.
> > 
> > 
> Hi,
> 
> 
> > > ---
> > >  mm/vmscan.c |   16 +++++++++++++++-
> > >  1 files changed, 15 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 060e4c1..e4601c5 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -2320,6 +2320,7 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
> > >  	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */
> > >  	unsigned long total_scanned;
> > >  	struct reclaim_state *reclaim_state = current->reclaim_state;
> > > +	unsigned long nr_soft_reclaimed;
> > >  	struct scan_control sc = {
> > >  		.gfp_mask = GFP_KERNEL,
> > >  		.may_unmap = 1,
> > > @@ -2413,7 +2414,20 @@ loop_again:
> > >  			 * Call soft limit reclaim before calling shrink_zone.
> > >  			 * For now we ignore the return value
> > >  			 */
> > > -			mem_cgroup_soft_limit_reclaim(zone, order, sc.gfp_mask);
> > > +			nr_soft_reclaimed = mem_cgroup_soft_limit_reclaim(zone,
> > > +							order, sc.gfp_mask);
> > > +
> > > +			/*
> > > +			 * Check the watermark after the soft limit reclaim. If
> > > +			 * the free pages is above the watermark, no need to
> > > +			 * proceed to the zone reclaim.
> > > +			 */
> > > +			if (nr_soft_reclaimed && zone_watermark_ok_safe(zone,
> > > +					order, high_wmark_pages(zone),
> > > +					end_zone, 0)) {
> > > +				__inc_zone_state(zone, NR_SKIP_RECLAIM_GLOBAL);
> > 
> > NR_SKIP_RECLAIM_GLOBAL is defined by patch 2/2. please don't break bisectability.
> > 
> > 
> > 
> > > +				continue;
> > > +			}
> 
> Hmm, this "continue" seems not good to me. And, IIUC, this was a reason
> we ignore the result. But yes, ignore the result is bad.
> I think you should just do sc.nr_reclaimed += nr_soft_reclaimed.
> Or mem_cgroup_soft_limit_reclaim() should update sc.
> 
> 
> And allow kswapd to do some jobs as
>  - call shrink_slab()
>  - update total_scanned
>  - update other flags.. etc...etc..
> 
> If extra shink_zone() seems bad, please skip it, if mem_cgroup_soft_limit_reclaim()
> did enough jobs.
> 
> IOW, mem_cgroup_soft_limit_reclaim() can't do enough jobs to satisfy
> ==
>    2426 			balance_gap = min(low_wmark_pages(zone),
>    2427 				(zone->present_pages +
>    2428 					KSWAPD_ZONE_BALANCE_GAP_RATIO-1) /
>    2429 				KSWAPD_ZONE_BALANCE_GAP_RATIO);
>    2430 			if (!zone_watermark_ok_safe(zone, order,
>    2431 					high_wmark_pages(zone) + balance_gap,
>    2432 					end_zone, 0))
>    2433 				shrink_zone(priority, zone, &sc);
> ==

Good point. 
We should consider balancing the pressure on every zones.


> Thanks,
> -Kame
> 

-- 
Kind regards,
Minchan Kim

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] check the return value of soft_limit reclaim

[Ying Han]

On Sun, Mar 27, 2011 at 11:39 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
>> In the global background reclaim, we do soft reclaim before scanning the
>> per-zone LRU. However, the return value is ignored. This patch adds the logic
>> where no per-zone reclaim happens if the soft reclaim raise the free pages
>> above the zone's high_wmark.
>>
>> I did notice a similar check exists but instead leaving a "gap" above the
>> high_wmark(the code right after my change in vmscan.c). There are discussions
>> on whether or not removing the "gap" which intends to balance pressures across
>> zones over time. Without fully understand the logic behind, I didn't try to
>> merge them into one, but instead adding the condition only for memcg users
>> who care a lot on memory isolation.
>>
>> Signed-off-by: Ying Han <yinghan@google.com>
>
> Looks good to me. But this depend on "memcg soft limit" spec. To be honest,
> I don't know this return value ignorance is intentional or not. So I think
> you need to get ack from memcg folks.
>
>
>> ---
>>  mm/vmscan.c |   16 +++++++++++++++-
>>  1 files changed, 15 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 060e4c1..e4601c5 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -2320,6 +2320,7 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
>>       int end_zone = 0;       /* Inclusive.  0 = ZONE_DMA */
>>       unsigned long total_scanned;
>>       struct reclaim_state *reclaim_state = current->reclaim_state;
>> +     unsigned long nr_soft_reclaimed;
>>       struct scan_control sc = {
>>               .gfp_mask = GFP_KERNEL,
>>               .may_unmap = 1,
>> @@ -2413,7 +2414,20 @@ loop_again:
>>                        * Call soft limit reclaim before calling shrink_zone.
>>                        * For now we ignore the return value
>>                        */
>> -                     mem_cgroup_soft_limit_reclaim(zone, order, sc.gfp_mask);
>> +                     nr_soft_reclaimed = mem_cgroup_soft_limit_reclaim(zone,
>> +                                                     order, sc.gfp_mask);
>> +
>> +                     /*
>> +                      * Check the watermark after the soft limit reclaim. If
>> +                      * the free pages is above the watermark, no need to
>> +                      * proceed to the zone reclaim.
>> +                      */
>> +                     if (nr_soft_reclaimed && zone_watermark_ok_safe(zone,
>> +                                     order, high_wmark_pages(zone),
>> +                                     end_zone, 0)) {
>> +                             __inc_zone_state(zone, NR_SKIP_RECLAIM_GLOBAL);
>
> NR_SKIP_RECLAIM_GLOBAL is defined by patch 2/2. please don't break bisectability.

Thanks and I will fix that.

--Ying
>
>
>
>> +                             continue;
>> +                     }
>>
>>                       /*
>>                        * We put equal pressure on every zone, unless
>> --
>> 1.7.3.1
>>
>
>
>
>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] check the return value of soft_limit reclaim

[Ying Han]

On Mon, Mar 28, 2011 at 1:44 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Mon, 28 Mar 2011 15:39:59 +0900 (JST)
> KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
>
>> > In the global background reclaim, we do soft reclaim before scanning the
>> > per-zone LRU. However, the return value is ignored. This patch adds the logic
>> > where no per-zone reclaim happens if the soft reclaim raise the free pages
>> > above the zone's high_wmark.
>> >
>> > I did notice a similar check exists but instead leaving a "gap" above the
>> > high_wmark(the code right after my change in vmscan.c). There are discussions
>> > on whether or not removing the "gap" which intends to balance pressures across
>> > zones over time. Without fully understand the logic behind, I didn't try to
>> > merge them into one, but instead adding the condition only for memcg users
>> > who care a lot on memory isolation.
>> >
>> > Signed-off-by: Ying Han <yinghan@google.com>
>>
>> Looks good to me. But this depend on "memcg soft limit" spec. To be honest,
>> I don't know this return value ignorance is intentional or not. So I think
>> you need to get ack from memcg folks.
>>
>>
> Hi,
>
>
>> > ---
>> >  mm/vmscan.c |   16 +++++++++++++++-
>> >  1 files changed, 15 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/mm/vmscan.c b/mm/vmscan.c
>> > index 060e4c1..e4601c5 100644
>> > --- a/mm/vmscan.c
>> > +++ b/mm/vmscan.c
>> > @@ -2320,6 +2320,7 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
>> >     int end_zone = 0;       /* Inclusive.  0 = ZONE_DMA */
>> >     unsigned long total_scanned;
>> >     struct reclaim_state *reclaim_state = current->reclaim_state;
>> > +   unsigned long nr_soft_reclaimed;
>> >     struct scan_control sc = {
>> >             .gfp_mask = GFP_KERNEL,
>> >             .may_unmap = 1,
>> > @@ -2413,7 +2414,20 @@ loop_again:
>> >                      * Call soft limit reclaim before calling shrink_zone.
>> >                      * For now we ignore the return value
>> >                      */
>> > -                   mem_cgroup_soft_limit_reclaim(zone, order, sc.gfp_mask);
>> > +                   nr_soft_reclaimed = mem_cgroup_soft_limit_reclaim(zone,
>> > +                                                   order, sc.gfp_mask);
>> > +
>> > +                   /*
>> > +                    * Check the watermark after the soft limit reclaim. If
>> > +                    * the free pages is above the watermark, no need to
>> > +                    * proceed to the zone reclaim.
>> > +                    */
>> > +                   if (nr_soft_reclaimed && zone_watermark_ok_safe(zone,
>> > +                                   order, high_wmark_pages(zone),
>> > +                                   end_zone, 0)) {
>> > +                           __inc_zone_state(zone, NR_SKIP_RECLAIM_GLOBAL);
>>
>> NR_SKIP_RECLAIM_GLOBAL is defined by patch 2/2. please don't break bisectability.
>>
>>
>>
>> > +                           continue;
>> > +                   }
>
> Hmm, this "continue" seems not good to me. And, IIUC, this was a reason
> we ignore the result. But yes, ignore the result is bad.
> I think you should just do sc.nr_reclaimed += nr_soft_reclaimed.
> Or mem_cgroup_soft_limit_reclaim() should update sc.
>
>
> And allow kswapd to do some jobs as
>  - call shrink_slab()
>  - update total_scanned
>  - update other flags.. etc...etc..

The change make sense to me. I will make the next patch to update
total_scanned and sc.nr_reclaimed.
Also, we might not want to skip shrink_slab() in this case, so i will add that.

>
> If extra shink_zone() seems bad, please skip it, if mem_cgroup_soft_limit_reclaim()
> did enough jobs.
>
> IOW, mem_cgroup_soft_limit_reclaim() can't do enough jobs to satisfy
> ==
>   2426                         balance_gap = min(low_wmark_pages(zone),
>   2427                                 (zone->present_pages +
>   2428                                         KSWAPD_ZONE_BALANCE_GAP_RATIO-1) /
>   2429                                 KSWAPD_ZONE_BALANCE_GAP_RATIO);
>   2430                         if (!zone_watermark_ok_safe(zone, order,
>   2431                                         high_wmark_pages(zone) + balance_gap,
>   2432                                         end_zone, 0))
>   2433                                 shrink_zone(priority, zone, &sc);
> ==
> This condition, you should update mem_cgroup_soft_limit_relcaim() to satisfy this,
> rather than continue here.
>
> I guess this is not easy...So, how about starting from updating 'sc' passed to
> mem_cgroup_soft_limit_reclaim() ? Then, we can think of algorithm.

The original patch introducing the "gap" was doing memory pressure
balancing across physical zones. Eventually we should get rid of
global per-zone reclaim in memcg(due to isolation), and maybe we need
something similar on per-memcg-per-zone. I will think about that.

So i will make the change on updating the two counters in scan_control
in next patch.

Thanks

--Ying

>
> Thanks,
> -Kame
>
>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] check the return value of soft_limit reclaim

[Balbir Singh]

* nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2011-03-28 16:33:11]:

> Hi,
> 
> This patch looks good to me, except for one nitpick.
> 
> On Sun, 27 Mar 2011 23:12:54 -0700
> Ying Han <yinghan@google.com> wrote:
> 
> > In the global background reclaim, we do soft reclaim before scanning the
> > per-zone LRU. However, the return value is ignored. This patch adds the logic
> > where no per-zone reclaim happens if the soft reclaim raise the free pages
> > above the zone's high_wmark.
> > 
> > I did notice a similar check exists but instead leaving a "gap" above the
> > high_wmark(the code right after my change in vmscan.c). There are discussions
> > on whether or not removing the "gap" which intends to balance pressures across
> > zones over time. Without fully understand the logic behind, I didn't try to
> > merge them into one, but instead adding the condition only for memcg users
> > who care a lot on memory isolation.
> > 
> > Signed-off-by: Ying Han <yinghan@google.com>
> > ---
> >  mm/vmscan.c |   16 +++++++++++++++-
> >  1 files changed, 15 insertions(+), 1 deletions(-)
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 060e4c1..e4601c5 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -2320,6 +2320,7 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
> >  	int end_zone = 0;	/* Inclusive.  0 = ZONE_DMA */
> >  	unsigned long total_scanned;
> >  	struct reclaim_state *reclaim_state = current->reclaim_state;
> > +	unsigned long nr_soft_reclaimed;
> >  	struct scan_control sc = {
> >  		.gfp_mask = GFP_KERNEL,
> >  		.may_unmap = 1,
> > @@ -2413,7 +2414,20 @@ loop_again:
> >  			 * Call soft limit reclaim before calling shrink_zone.
> >  			 * For now we ignore the return value
> 
> You should remove this comment too.
> 
> But, Balbir-san, do you remember why did you ignore the return value here ?
>

We do that since soft limit reclaim cannot help us make a decision
from the return value. balance_gap is recomputed following this
routine. May be it might make sense to increment sc.nr_reclaimed based
on the return value? 

-- 
	Three Cheers,
	Balbir

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] check the return value of soft_limit reclaim

[Ying Han]

On Tue, Mar 29, 2011 at 8:38 AM, Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> * nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2011-03-28 16:33:11]:
>
>> Hi,
>>
>> This patch looks good to me, except for one nitpick.
>>
>> On Sun, 27 Mar 2011 23:12:54 -0700
>> Ying Han <yinghan@google.com> wrote:
>>
>> > In the global background reclaim, we do soft reclaim before scanning the
>> > per-zone LRU. However, the return value is ignored. This patch adds the logic
>> > where no per-zone reclaim happens if the soft reclaim raise the free pages
>> > above the zone's high_wmark.
>> >
>> > I did notice a similar check exists but instead leaving a "gap" above the
>> > high_wmark(the code right after my change in vmscan.c). There are discussions
>> > on whether or not removing the "gap" which intends to balance pressures across
>> > zones over time. Without fully understand the logic behind, I didn't try to
>> > merge them into one, but instead adding the condition only for memcg users
>> > who care a lot on memory isolation.
>> >
>> > Signed-off-by: Ying Han <yinghan@google.com>
>> > ---
>> >  mm/vmscan.c |   16 +++++++++++++++-
>> >  1 files changed, 15 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/mm/vmscan.c b/mm/vmscan.c
>> > index 060e4c1..e4601c5 100644
>> > --- a/mm/vmscan.c
>> > +++ b/mm/vmscan.c
>> > @@ -2320,6 +2320,7 @@ static unsigned long balance_pgdat(pg_data_t *pgdat, int order,
>> >     int end_zone = 0;       /* Inclusive.  0 = ZONE_DMA */
>> >     unsigned long total_scanned;
>> >     struct reclaim_state *reclaim_state = current->reclaim_state;
>> > +   unsigned long nr_soft_reclaimed;
>> >     struct scan_control sc = {
>> >             .gfp_mask = GFP_KERNEL,
>> >             .may_unmap = 1,
>> > @@ -2413,7 +2414,20 @@ loop_again:
>> >                      * Call soft limit reclaim before calling shrink_zone.
>> >                      * For now we ignore the return value
>>
>> You should remove this comment too.
>>
>> But, Balbir-san, do you remember why did you ignore the return value here ?
>>
>
> We do that since soft limit reclaim cannot help us make a decision from the return value. balance_gap is recomputed following this routine.

I don't fully understand the "balance_gap" at the first place, and
maybe that is something interesting to talk about
in LSF :)


May be it might make sense to increment sc.nr_reclaimed based on the
return value?

Yes, that is how it is implemented now in V3 where we contribute the
sc.nr_scanned and sc.nr_reclaimed from soft_limit reclaim.

Thanks

--Ying

>
> --
>        Three Cheers,
>        Balbir
>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>