[RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

If we can't isolate pages from LRU list, we don't have to account page movement, either.
Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.

This patch removes unnecessary overhead of page accounting
and locking in shrink_active_list as follow-up work of commit 5343daceec.

Signed-off-by: Vincent Li <macli@brc.ubc.ca>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
Acked-by: Rik van Riel <riel@redhat.com>

---
 mm/vmscan.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 460a6f7..2d1c846 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
 	if (scanning_global_lru(sc)) {
 		zone->pages_scanned += pgscanned;
 	}
-	reclaim_stat->recent_scanned[file] += nr_taken;
-
 	__count_zone_vm_events(PGREFILL, zone, pgscanned);
+
+	if (nr_taken == 0)
+		goto done;
+
+	reclaim_stat->recent_scanned[file] += nr_taken;
 	if (file)
 		__mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
 	else
@@ -1383,6 +1386,8 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
 	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
 	__mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
 	__mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
+
+done:
 	spin_unlock_irq(&zone->lru_lock);
 }
 
-- 
1.6.0.4

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Andrew Morton]

On Wed,  2 Sep 2009 16:49:25 -0700
Vincent Li <macli@brc.ubc.ca> wrote:

> If we can't isolate pages from LRU list, we don't have to account page movement, either.
> Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> 
> This patch removes unnecessary overhead of page accounting
> and locking in shrink_active_list as follow-up work of commit 5343daceec.
> 
> Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> Acked-by: Rik van Riel <riel@redhat.com>
> 
> ---
>  mm/vmscan.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 460a6f7..2d1c846 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>  	if (scanning_global_lru(sc)) {
>  		zone->pages_scanned += pgscanned;
>  	}
> -	reclaim_stat->recent_scanned[file] += nr_taken;
> -
>  	__count_zone_vm_events(PGREFILL, zone, pgscanned);
> +
> +	if (nr_taken == 0)
> +		goto done;
> +
> +	reclaim_stat->recent_scanned[file] += nr_taken;
>  	if (file)
>  		__mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
>  	else
> @@ -1383,6 +1386,8 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>  	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
>  	__mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
>  	__mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
> +
> +done:
>  	spin_unlock_irq(&zone->lru_lock);
>  }

How do we know this patch is a net gain?

IOW, with what frequency is `nr_taken' zero here?

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Thu, 3 Sep 2009, Andrew Morton wrote:

> On Wed,  2 Sep 2009 16:49:25 -0700
> Vincent Li <macli@brc.ubc.ca> wrote:
> 
> > If we can't isolate pages from LRU list, we don't have to account page movement, either.
> > Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> > 
> > This patch removes unnecessary overhead of page accounting
> > and locking in shrink_active_list as follow-up work of commit 5343daceec.
> > 
> > Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> > Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> > Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> > Acked-by: Rik van Riel <riel@redhat.com>
> > 
> > ---
> >  mm/vmscan.c |    9 +++++++--
> >  1 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 460a6f7..2d1c846 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> >  	if (scanning_global_lru(sc)) {
> >  		zone->pages_scanned += pgscanned;
> >  	}
> > -	reclaim_stat->recent_scanned[file] += nr_taken;
> > -
> >  	__count_zone_vm_events(PGREFILL, zone, pgscanned);
> > +
> > +	if (nr_taken == 0)
> > +		goto done;
> > +
> > +	reclaim_stat->recent_scanned[file] += nr_taken;
> >  	if (file)
> >  		__mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
> >  	else
> > @@ -1383,6 +1386,8 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> >  	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
> >  	__mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
> >  	__mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
> > +
> > +done:
> >  	spin_unlock_irq(&zone->lru_lock);
> >  }
> 
> How do we know this patch is a net gain?
> 
> IOW, with what frequency is `nr_taken' zero here?
> 

Actually, I have asked myself the same question, Anyway I can verify this, 
Kim, KOSAKI? 

Vincent Li
Biomedical Research Center
University of British Columbia

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Andrew Morton]

On Thu, 3 Sep 2009 15:02:58 -0700 (PDT)
Vincent Li <macli@brc.ubc.ca> wrote:

> On Thu, 3 Sep 2009, Andrew Morton wrote:
> 
> > On Wed,  2 Sep 2009 16:49:25 -0700
> > Vincent Li <macli@brc.ubc.ca> wrote:
> > 
> > > If we can't isolate pages from LRU list, we don't have to account page movement, either.
> > > Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> > > 
> > > This patch removes unnecessary overhead of page accounting
> > > and locking in shrink_active_list as follow-up work of commit 5343daceec.
> > > 
> > > Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> > > Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> > > Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > > Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> > > Acked-by: Rik van Riel <riel@redhat.com>
> > > 
> > > ---
> > >  mm/vmscan.c |    9 +++++++--
> > >  1 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 460a6f7..2d1c846 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> > >  	if (scanning_global_lru(sc)) {
> > >  		zone->pages_scanned += pgscanned;
> > >  	}
> > > -	reclaim_stat->recent_scanned[file] += nr_taken;
> > > -
> > >  	__count_zone_vm_events(PGREFILL, zone, pgscanned);
> > > +
> > > +	if (nr_taken == 0)
> > > +		goto done;
> > > +
> > > +	reclaim_stat->recent_scanned[file] += nr_taken;
> > >  	if (file)
> > >  		__mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
> > >  	else
> > > @@ -1383,6 +1386,8 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> > >  	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
> > >  	__mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
> > >  	__mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
> > > +
> > > +done:
> > >  	spin_unlock_irq(&zone->lru_lock);
> > >  }
> > 
> > How do we know this patch is a net gain?
> > 
> > IOW, with what frequency is `nr_taken' zero here?
> > 
> 
> Actually, I have asked myself the same question, Anyway I can verify this, 
> Kim, KOSAKI? 

Put two counters in there.

They could be ad-hoc displayed-in-/proc counters.  Or ad-hoc additions
to /proc/vmstat.  Or you could dive into the tracing framework and use
that.  These patches in -mm:

tracing-page-allocator-add-trace-events-for-page-allocation-and-page-freeing.patch
tracing-page-allocator-add-trace-events-for-anti-fragmentation-falling-back-to-other-migratetypes.patch
tracing-page-allocator-add-trace-event-for-page-traffic-related-to-the-buddy-lists.patch
tracing-page-allocator-add-trace-event-for-page-traffic-related-to-the-buddy-lists-fix.patch
tracing-page-allocator-add-a-postprocessing-script-for-page-allocator-related-ftrace-events.patch
tracing-documentation-add-a-document-describing-how-to-do-some-performance-analysis-with-tracepoints.patch
tracing-documentation-add-a-document-on-the-kmem-tracepoints.patch


would be a suitable guide.


The way I used to do stuff like this is:

int akpm1;
int akpm2;

	...
	if (nr_taken)
		akpm1++;
	else
		akpm2++;

then inspect the values of akpm1 and akpm2 in the running kernel using kgdb.

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Thu, 3 Sep 2009, Andrew Morton wrote:

> On Thu, 3 Sep 2009 15:02:58 -0700 (PDT)
> Vincent Li <macli@brc.ubc.ca> wrote:
> 
> > On Thu, 3 Sep 2009, Andrew Morton wrote:
> > 
> > > On Wed,  2 Sep 2009 16:49:25 -0700
> > > Vincent Li <macli@brc.ubc.ca> wrote:
> > > 
> > > > If we can't isolate pages from LRU list, we don't have to account page movement, either.
> > > > Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> > > > 
> > > > This patch removes unnecessary overhead of page accounting
> > > > and locking in shrink_active_list as follow-up work of commit 5343daceec.
> > > > 
> > > > Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> > > > Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> > > > Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > > > Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> > > > Acked-by: Rik van Riel <riel@redhat.com>
> > > > 
> > > > ---
> > > >  mm/vmscan.c |    9 +++++++--
> > > >  1 files changed, 7 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > > index 460a6f7..2d1c846 100644
> > > > --- a/mm/vmscan.c
> > > > +++ b/mm/vmscan.c
> > > > @@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> > > >  	if (scanning_global_lru(sc)) {
> > > >  		zone->pages_scanned += pgscanned;
> > > >  	}
> > > > -	reclaim_stat->recent_scanned[file] += nr_taken;
> > > > -
> > > >  	__count_zone_vm_events(PGREFILL, zone, pgscanned);
> > > > +
> > > > +	if (nr_taken == 0)
> > > > +		goto done;
> > > > +
> > > > +	reclaim_stat->recent_scanned[file] += nr_taken;
> > > >  	if (file)
> > > >  		__mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
> > > >  	else
> > > > @@ -1383,6 +1386,8 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> > > >  	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
> > > >  	__mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
> > > >  	__mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
> > > > +
> > > > +done:
> > > >  	spin_unlock_irq(&zone->lru_lock);
> > > >  }
> > > 
> > > How do we know this patch is a net gain?
> > > 
> > > IOW, with what frequency is `nr_taken' zero here?
> > > 
> > 
> > Actually, I have asked myself the same question, Anyway I can verify this, 
> > Kim, KOSAKI? 
> 
> Put two counters in there.
> 
> They could be ad-hoc displayed-in-/proc counters.  Or ad-hoc additions
> to /proc/vmstat.  Or you could dive into the tracing framework and use
> that.  These patches in -mm:
> 
> tracing-page-allocator-add-trace-events-for-page-allocation-and-page-freeing.patch
> tracing-page-allocator-add-trace-events-for-anti-fragmentation-falling-back-to-other-migratetypes.patch
> tracing-page-allocator-add-trace-event-for-page-traffic-related-to-the-buddy-lists.patch
> tracing-page-allocator-add-trace-event-for-page-traffic-related-to-the-buddy-lists-fix.patch
> tracing-page-allocator-add-a-postprocessing-script-for-page-allocator-related-ftrace-events.patch
> tracing-documentation-add-a-document-describing-how-to-do-some-performance-analysis-with-tracepoints.patch
> tracing-documentation-add-a-document-on-the-kmem-tracepoints.patch
> 
> 
> would be a suitable guide.
> 

Ok, I followed the patches above to make following testing code:

---
diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
index eaf46bd..863820a 100644
--- a/include/trace/events/kmem.h
+++ b/include/trace/events/kmem.h
@@ -388,6 +388,24 @@ TRACE_EVENT(mm_page_alloc_extfrag,
 		__entry->alloc_migratetype == __entry->fallback_migratetype)
 );
 
+TRACE_EVENT(mm_vmscan_isolate_pages,
+
+	TP_PROTO(int nr_taken_zeros),
+
+	TP_ARGS(nr_taken_zeros),
+
+	TP_STRUCT__entry(
+		__field(int,		nr_taken_zeros)
+	),
+
+	TP_fast_assign(
+		__entry->nr_taken_zeros	= nr_taken_zeros;
+	),
+
+	TP_printk("nr_taken_zeros=%d",
+		__entry->nr_taken_zeros)
+);
+
 #endif /* _TRACE_KMEM_H */
 
 /* This part must be outside protection */
diff --git a/mm/vmscan.c b/mm/vmscan.c
index ad93096..c2cf4dd 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -40,6 +40,7 @@
 #include <linux/memcontrol.h>
 #include <linux/delayacct.h>
 #include <linux/sysctl.h>
+#include <trace/events/kmem.h>
 
 #include <asm/tlbflush.h>
 #include <asm/div64.h>
@@ -1306,6 +1307,7 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
 	struct zone_reclaim_stat *reclaim_stat = get_reclaim_stat(zone, sc);
 	unsigned long nr_rotated = 0;
 	unsigned long nr_deactivated = 0;
+	int nr_taken_zeros = 0;
 
 	lru_add_drain();
 	spin_lock_irq(&zone->lru_lock);
@@ -1321,8 +1323,11 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
 	}
 	__count_zone_vm_events(PGREFILL, zone, pgscanned);
 
-	if (nr_taken == 0)
+	if (nr_taken == 0) {
+		nr_taken_zeros++;
+		trace_mm_vmscan_isolate_pages(nr_taken_zeros);
 		goto done;
+	}
 
 	reclaim_stat->recent_scanned[file] += nr_taken;
 	if (file)

Then I got test result with:

root@kernelhack:/usr/src/mmotm-0903# perf  stat --repeat 5  -e \ 
kmem:mm_vmscan_isolate_pages hackbench 100

Running with 100*40 (== 4000) tasks.
Time: 52.736
Running with 100*40 (== 4000) tasks.
Time: 64.982
Running with 100*40 (== 4000) tasks.
Time: 56.866
Running with 100*40 (== 4000) tasks.
Time: 37.137
Running with 100*40 (== 4000) tasks.
Time: 48.415

 Performance counter stats for 'hackbench 100' (5 runs):

          14189  kmem:mm_vmscan_isolate_pages   ( +-   9.084% )

   52.680621973  seconds time elapsed   ( +-   0.689% )

Is the testing patch written write? I don't understand what the number 
14189 means? Does it make any sense?

> 
> The way I used to do stuff like this is:
> 
> int akpm1;
> int akpm2;
> 
> 	...
> 	if (nr_taken)
> 		akpm1++;
> 	else
> 		akpm2++;
> 
> then inspect the values of akpm1 and akpm2 in the running kernel using kgdb.
> 
> 
> 

Vincent Li
Biomedical Research Center
University of British Columbia

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Andrew Morton]

On Fri, 4 Sep 2009 14:39:32 -0700 (PDT)
Vincent Li <macli@brc.ubc.ca> wrote:

> 
> Ok, I followed the patches above to make following testing code:
> 
> ---
> diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
> index eaf46bd..863820a 100644
> --- a/include/trace/events/kmem.h
> +++ b/include/trace/events/kmem.h
> @@ -388,6 +388,24 @@ TRACE_EVENT(mm_page_alloc_extfrag,
>  		__entry->alloc_migratetype == __entry->fallback_migratetype)
>  );
>  
> +TRACE_EVENT(mm_vmscan_isolate_pages,
> +
> +	TP_PROTO(int nr_taken_zeros),
> +
> +	TP_ARGS(nr_taken_zeros),
> +
> +	TP_STRUCT__entry(
> +		__field(int,		nr_taken_zeros)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->nr_taken_zeros	= nr_taken_zeros;
> +	),
> +
> +	TP_printk("nr_taken_zeros=%d",
> +		__entry->nr_taken_zeros)
> +);
> +
>  #endif /* _TRACE_KMEM_H */
>  
>  /* This part must be outside protection */
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index ad93096..c2cf4dd 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -40,6 +40,7 @@
>  #include <linux/memcontrol.h>
>  #include <linux/delayacct.h>
>  #include <linux/sysctl.h>
> +#include <trace/events/kmem.h>
>  
>  #include <asm/tlbflush.h>
>  #include <asm/div64.h>
> @@ -1306,6 +1307,7 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>  	struct zone_reclaim_stat *reclaim_stat = get_reclaim_stat(zone, sc);
>  	unsigned long nr_rotated = 0;
>  	unsigned long nr_deactivated = 0;
> +	int nr_taken_zeros = 0;
>  
>  	lru_add_drain();
>  	spin_lock_irq(&zone->lru_lock);
> @@ -1321,8 +1323,11 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>  	}
>  	__count_zone_vm_events(PGREFILL, zone, pgscanned);
>  
> -	if (nr_taken == 0)
> +	if (nr_taken == 0) {
> +		nr_taken_zeros++;
> +		trace_mm_vmscan_isolate_pages(nr_taken_zeros);
>  		goto done;
> +	}
>  
>  	reclaim_stat->recent_scanned[file] += nr_taken;
>  	if (file)

Well you want to count two things: 1: how many times nr_taken==0 and 2:
how many times nr_taken!=0.

> Then I got test result with:
> 
> root@kernelhack:/usr/src/mmotm-0903# perf  stat --repeat 5  -e \ 
> kmem:mm_vmscan_isolate_pages hackbench 100
> 
> Running with 100*40 (== 4000) tasks.
> Time: 52.736
> Running with 100*40 (== 4000) tasks.
> Time: 64.982
> Running with 100*40 (== 4000) tasks.
> Time: 56.866
> Running with 100*40 (== 4000) tasks.
> Time: 37.137
> Running with 100*40 (== 4000) tasks.
> Time: 48.415
> 
>  Performance counter stats for 'hackbench 100' (5 runs):
> 
>           14189  kmem:mm_vmscan_isolate_pages   ( +-   9.084% )
> 
>    52.680621973  seconds time elapsed   ( +-   0.689% )
> 
> Is the testing patch written write? I don't understand what the number 
> 14189 means? Does it make any sense?

I don't think you need nr_taken_zeros at all.  You'd want something like

	if (nr_taken == 0)
		trace_mm_vmscan_nr_taken_zero();
	else
		trace_mm_vmscan_nr_taken_nonzero();

which would pointlessly generate a huge stream of events which would
have to be added up downstream, which is dumb.

I don't know if the tracing code is capable of maintaining the counters
for you.  Perhaps you _do_ need nr_taken_zeros.  In which case you want

	if (nr_taken == 0) {
		nr_taken_zeros++;
		trace_mm_vmscan_isolate_pages_zero(nr_taken_zeros);
	} else {
		nr_taken_nonzeros++;
		trace_mm_vmscan_isolate_pages_nonzero(nr_taken_nonzeros);
	}

which is awkward.  Mel will know.

> > 
> > The way I used to do stuff like this is:
> > 
> > int akpm1;
> > int akpm2;
> > 
> > 	...
> > 	if (nr_taken)
> > 		akpm1++;
> > 	else
> > 		akpm2++;
> > 
> > then inspect the values of akpm1 and akpm2 in the running kernel using kgdb.

That's looking more attractive ;)

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Mel Gorman]

On Fri, Sep 04, 2009 at 04:53:05PM -0700, Andrew Morton wrote:
> On Fri, 4 Sep 2009 14:39:32 -0700 (PDT)
> Vincent Li <macli@brc.ubc.ca> wrote:
> 
> > 
> > Ok, I followed the patches above to make following testing code:
> > 
> > ---
> > diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
> > index eaf46bd..863820a 100644
> > --- a/include/trace/events/kmem.h
> > +++ b/include/trace/events/kmem.h
> > @@ -388,6 +388,24 @@ TRACE_EVENT(mm_page_alloc_extfrag,
> >  		__entry->alloc_migratetype == __entry->fallback_migratetype)
> >  );
> >  
> > +TRACE_EVENT(mm_vmscan_isolate_pages,
> > +
> > +	TP_PROTO(int nr_taken_zeros),
> > +
> > +	TP_ARGS(nr_taken_zeros),
> > +
> > +	TP_STRUCT__entry(
> > +		__field(int,		nr_taken_zeros)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		__entry->nr_taken_zeros	= nr_taken_zeros;
> > +	),
> > +
> > +	TP_printk("nr_taken_zeros=%d",
> > +		__entry->nr_taken_zeros)
> > +);
> > +
> >  #endif /* _TRACE_KMEM_H */
> >  
> >  /* This part must be outside protection */
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index ad93096..c2cf4dd 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -40,6 +40,7 @@
> >  #include <linux/memcontrol.h>
> >  #include <linux/delayacct.h>
> >  #include <linux/sysctl.h>
> > +#include <trace/events/kmem.h>
> >  
> >  #include <asm/tlbflush.h>
> >  #include <asm/div64.h>
> > @@ -1306,6 +1307,7 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> >  	struct zone_reclaim_stat *reclaim_stat = get_reclaim_stat(zone, sc);
> >  	unsigned long nr_rotated = 0;
> >  	unsigned long nr_deactivated = 0;
> > +	int nr_taken_zeros = 0;
> >  
> >  	lru_add_drain();
> >  	spin_lock_irq(&zone->lru_lock);
> > @@ -1321,8 +1323,11 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> >  	}
> >  	__count_zone_vm_events(PGREFILL, zone, pgscanned);
> >  
> > -	if (nr_taken == 0)
> > +	if (nr_taken == 0) {
> > +		nr_taken_zeros++;
> > +		trace_mm_vmscan_isolate_pages(nr_taken_zeros);
> >  		goto done;
> > +	}
> >  
> >  	reclaim_stat->recent_scanned[file] += nr_taken;
> >  	if (file)
> 
> Well you want to count two things: 1: how many times nr_taken==0 and 2:
> how many times nr_taken!=0.
> 

Indeed. I'm not aware of the specifics that led to this patch, but minimally
one would be interested in the exact value of nr_taken as it can be used to
answer more than one question.

> > Then I got test result with:
> > 
> > root@kernelhack:/usr/src/mmotm-0903# perf  stat --repeat 5  -e \ 
> > kmem:mm_vmscan_isolate_pages hackbench 100
> > 
> > Running with 100*40 (== 4000) tasks.
> > Time: 52.736
> > Running with 100*40 (== 4000) tasks.
> > Time: 64.982
> > Running with 100*40 (== 4000) tasks.
> > Time: 56.866
> > Running with 100*40 (== 4000) tasks.
> > Time: 37.137
> > Running with 100*40 (== 4000) tasks.
> > Time: 48.415
> > 
> >  Performance counter stats for 'hackbench 100' (5 runs):
> > 
> >           14189  kmem:mm_vmscan_isolate_pages   ( +-   9.084% )
> > 
> >    52.680621973  seconds time elapsed   ( +-   0.689% )
> > 
> > Is the testing patch written write? I don't understand what the number 
> > 14189 means? Does it make any sense?
> 

Broadly speaking

"For each of the 5 runs of hackbench, there were 14189 times the
kmem:mm_vmscan_isolate_pages was sampled  +/- 9.084%"

Without knowing how many times nr_taken_zero was positive, it's
difficult to tell whether 14189 is common or not.

> I don't think you need nr_taken_zeros at all.  You'd want something like
> 
> 	if (nr_taken == 0)
> 		trace_mm_vmscan_nr_taken_zero();
> 	else
> 		trace_mm_vmscan_nr_taken_nonzero();
> 
> which would pointlessly generate a huge stream of events which would
> have to be added up downstream, which is dumb.
> 

Dumb it might be, but perf acts as that aggregator. For the purposes of
debugging, it would be fine although it would not be a very suitable pair
of events to merge to mainline. A more sensible trace point for mainline
would record what nr_taken was so a higher-level tool could answer the zero
vs non-zero question or optionally do things like figure out how many pages
were being taken of the lists and being put back.

For this question though, use the two tracepoints with no additional parameters
and have perf how many times each event occurred.

> I don't know if the tracing code is capable of maintaining the counters
> for you.  Perhaps you _do_ need nr_taken_zeros.  In which case you want
> 
> 	if (nr_taken == 0) {
> 		nr_taken_zeros++;
> 		trace_mm_vmscan_isolate_pages_zero(nr_taken_zeros);
> 	} else {
> 		nr_taken_nonzeros++;
> 		trace_mm_vmscan_isolate_pages_nonzero(nr_taken_nonzeros);
> 	}
> 
> which is awkward.  Mel will know.
> 

I am not aware of a way of maintaining counters within tracing. The difficulty
is figuring out when that event should be recorded. Worse, the count is
being aggregated for all processes that enter this path so there is noise
in the value that cannot be filtered out. An aggregated counter like this
makes sense when reporting to /proc or for discovering via kgdb, but less
suitable for tracing.

> > > 
> > > The way I used to do stuff like this is:
> > > 
> > > int akpm1;
> > > int akpm2;
> > > 
> > > 	...
> > > 	if (nr_taken)
> > > 		akpm1++;
> > > 	else
> > > 		akpm2++;
> > > 
> > > then inspect the values of akpm1 and akpm2 in the running kernel using kgdb.
> 
> That's looking more attractive ;)
> 

It's simplier but it's global in nature so it's harder to figure out how much
of the counter is for the target workload and how much of it is everything
else. The main advantage of using the two tracepoints is that you know exactly
how many of the events were due to hackbench and not the rest of the system.

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

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Tue, 8 Sep 2009, Mel Gorman wrote:

> On Fri, Sep 04, 2009 at 04:53:05PM -0700, Andrew Morton wrote:
> > On Fri, 4 Sep 2009 14:39:32 -0700 (PDT)
> > Vincent Li <macli@brc.ubc.ca> wrote:
> > 
> > Well you want to count two things: 1: how many times nr_taken==0 and 2:
> > how many times nr_taken!=0.
> > 
> 
> Indeed. I'm not aware of the specifics that led to this patch, but minimally
> one would be interested in the exact value of nr_taken as it can be used to
> answer more than one question.
> 
> > > Then I got test result with:
> > > 
> > > root@kernelhack:/usr/src/mmotm-0903# perf  stat --repeat 5  -e \ 
> > > kmem:mm_vmscan_isolate_pages hackbench 100
> > > 
> > > Running with 100*40 (== 4000) tasks.
> > > Time: 52.736
> > > Running with 100*40 (== 4000) tasks.
> > > Time: 64.982
> > > Running with 100*40 (== 4000) tasks.
> > > Time: 56.866
> > > Running with 100*40 (== 4000) tasks.
> > > Time: 37.137
> > > Running with 100*40 (== 4000) tasks.
> > > Time: 48.415
> > > 
> > >  Performance counter stats for 'hackbench 100' (5 runs):
> > > 
> > >           14189  kmem:mm_vmscan_isolate_pages   ( +-   9.084% )
> > > 
> > >    52.680621973  seconds time elapsed   ( +-   0.689% )
> > > 
> > > Is the testing patch written write? I don't understand what the number 
> > > 14189 means? Does it make any sense?
> > 
> 
> Broadly speaking
> 
> "For each of the 5 runs of hackbench, there were 14189 times the
> kmem:mm_vmscan_isolate_pages was sampled  +/- 9.084%"
> 
> Without knowing how many times nr_taken_zero was positive, it's
> difficult to tell whether 14189 is common or not.
> 
> > I don't think you need nr_taken_zeros at all.  You'd want something like
> > 
> > 	if (nr_taken == 0)
> > 		trace_mm_vmscan_nr_taken_zero();
> > 	else
> > 		trace_mm_vmscan_nr_taken_nonzero();
> > 
> > which would pointlessly generate a huge stream of events which would
> > have to be added up downstream, which is dumb.
> > 
> 
> Dumb it might be, but perf acts as that aggregator. For the purposes of
> debugging, it would be fine although it would not be a very suitable pair
> of events to merge to mainline. A more sensible trace point for mainline
> would record what nr_taken was so a higher-level tool could answer the zero
> vs non-zero question or optionally do things like figure out how many pages
> were being taken of the lists and being put back.
> 
> For this question though, use the two tracepoints with no additional parameters
> and have perf how many times each event occurred.

Thank you for the explaintion. I am not sure I follow your discussion 
correctly, no additional parameters means something like:

TRACE_EVENT(mm_vmscan_nr_taken_zero,
       TP_PROTO( ),
       TP_ARGS( ),
       TP_STRUCT__entry( ),
       TP_fast_assign( ),
       TP_printk( )
);

? which looks strange to me and does not compile. I guess that is not what 
you mean.

I ended up with a following patch:

----PATCH BEGIN---

---
diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
index eaf46bd..1f9e7bf 100644
--- a/include/trace/events/kmem.h
+++ b/include/trace/events/kmem.h
@@ -388,6 +388,42 @@ TRACE_EVENT(mm_page_alloc_extfrag,
 		__entry->alloc_migratetype == __entry->fallback_migratetype)
 );
 
+TRACE_EVENT(mm_vmscan_nr_taken_zero,
+
+	TP_PROTO(unsigned long nr_taken),
+
+	TP_ARGS(nr_taken),
+
+	TP_STRUCT__entry(
+		__field(        unsigned long,          nr_taken        )
+	),
+
+	TP_fast_assign(
+		__entry->nr_taken       = nr_taken;
+	),
+
+	TP_printk("nr_taken=%lu",
+	__entry->nr_taken)
+);
+
+TRACE_EVENT(mm_vmscan_nr_taken_nonzero,
+
+	TP_PROTO(unsigned long nr_taken),
+
+	TP_ARGS(nr_taken),
+
+	TP_STRUCT__entry(
+		__field(        unsigned long,          nr_taken        )
+	),
+
+	TP_fast_assign(
+		__entry->nr_taken       = nr_taken;
+	),
+
+	TP_printk("nr_taken=%lu",
+		__entry->nr_taken)
+);
+
 #endif /* _TRACE_KMEM_H */
 
 /* This part must be outside protection */
diff --git a/mm/vmscan.c b/mm/vmscan.c
index ad93096..eec4099 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -40,6 +40,7 @@
 #include <linux/memcontrol.h>
 #include <linux/delayacct.h>
 #include <linux/sysctl.h>
+#include <trace/events/kmem.h>
 
 #include <asm/tlbflush.h>
 #include <asm/div64.h>
@@ -1322,7 +1323,9 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
 	__count_zone_vm_events(PGREFILL, zone, pgscanned);
 
 	if (nr_taken == 0)
-		goto done;
+		trace_mm_vmscan_nr_taken_zero(nr_taken);
+	else
+		trace_mm_vmscan_nr_taken_nonzero(nr_taken);
 
 	reclaim_stat->recent_scanned[file] += nr_taken;
 	if (file)
@@ -1388,7 +1391,6 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
 							nr_rotated);
 	__mod_zone_page_state(zone, NR_INACTIVE_ANON + file * LRU_FILE,
 							nr_deactivated);
-done:
 	spin_unlock_irq(&zone->lru_lock);
 }

----PATCH END---
 
/usr/src/mmotm-0903# perf stat --repeat 5 -e kmem:mm_vmscan_nr_taken_zero \
 -e kmem:mm_vmscan_nr_taken_nonzero hackbench 100

Running with 100*40 (== 4000) tasks.
Time: 41.599
Running with 100*40 (== 4000) tasks.
Time: 80.192
Running with 100*40 (== 4000) tasks.
Time: 26.451
Running with 100*40 (== 4000) tasks.
Time: 65.428
Running with 100*40 (== 4000) tasks.
Time: 30.054

 Performance counter stats for 'hackbench 100' (5 runs):

          10330  kmem:mm_vmscan_nr_taken_zero   ( +-  11.732% )
           2601  kmem:mm_vmscan_nr_taken_nonzero   ( +-  10.876% )

   49.509328260  seconds time elapsed   ( +-   0.934% )

Sampling of nr_taken_zero is way bigger than sampling of nr_taken_nonzero 
in the 5 hackbench runs. I thought the sampling result would be the 
opposite. 

Maybe I get it all wrong :-).


> 
> > I don't know if the tracing code is capable of maintaining the counters
> > for you.  Perhaps you _do_ need nr_taken_zeros.  In which case you want
> > 
> > 	if (nr_taken == 0) {
> > 		nr_taken_zeros++;
> > 		trace_mm_vmscan_isolate_pages_zero(nr_taken_zeros);
> > 	} else {
> > 		nr_taken_nonzeros++;
> > 		trace_mm_vmscan_isolate_pages_nonzero(nr_taken_nonzeros);
> > 	}
> > 
> > which is awkward.  Mel will know.
> > 
> 
> I am not aware of a way of maintaining counters within tracing. The difficulty
> is figuring out when that event should be recorded. Worse, the count is
> being aggregated for all processes that enter this path so there is noise
> in the value that cannot be filtered out. An aggregated counter like this
> makes sense when reporting to /proc or for discovering via kgdb, but less
> suitable for tracing.
> 
> > > > 
> > > > The way I used to do stuff like this is:
> > > > 
> > > > int akpm1;
> > > > int akpm2;
> > > > 
> > > > 	...
> > > > 	if (nr_taken)
> > > > 		akpm1++;
> > > > 	else
> > > > 		akpm2++;
> > > > 
> > > > then inspect the values of akpm1 and akpm2 in the running kernel using kgdb.
> > 
> > That's looking more attractive ;)
> > 
> 
> It's simplier but it's global in nature so it's harder to figure out how much
> of the counter is for the target workload and how much of it is everything
> else. The main advantage of using the two tracepoints is that you know exactly
> how many of the events were due to hackbench and not the rest of the system.
> 
> -- 
> Mel Gorman
> Part-time Phd Student                          Linux Technology Center
> University of Limerick                         IBM Dublin Software Lab
> 
> 

Vincent Li
Biomedical Research Center
University of British Columbia

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Minchan Kim]

On Tue, 8 Sep 2009 15:39:59 -0700 (PDT)
Vincent Li <macli@brc.ubc.ca> wrote:

> On Tue, 8 Sep 2009, Mel Gorman wrote:
> 
> > On Fri, Sep 04, 2009 at 04:53:05PM -0700, Andrew Morton wrote:
> > > On Fri, 4 Sep 2009 14:39:32 -0700 (PDT)
> > > Vincent Li <macli@brc.ubc.ca> wrote:
> > > 
> > > Well you want to count two things: 1: how many times nr_taken==0 and 2:
> > > how many times nr_taken!=0.
> > > 
> > 
> > Indeed. I'm not aware of the specifics that led to this patch, but minimally
> > one would be interested in the exact value of nr_taken as it can be used to
> > answer more than one question.
> > 
> > > > Then I got test result with:
> > > > 
> > > > root@kernelhack:/usr/src/mmotm-0903# perf  stat --repeat 5  -e \ 
> > > > kmem:mm_vmscan_isolate_pages hackbench 100
> > > > 
> > > > Running with 100*40 (== 4000) tasks.
> > > > Time: 52.736
> > > > Running with 100*40 (== 4000) tasks.
> > > > Time: 64.982
> > > > Running with 100*40 (== 4000) tasks.
> > > > Time: 56.866
> > > > Running with 100*40 (== 4000) tasks.
> > > > Time: 37.137
> > > > Running with 100*40 (== 4000) tasks.
> > > > Time: 48.415
> > > > 
> > > >  Performance counter stats for 'hackbench 100' (5 runs):
> > > > 
> > > >           14189  kmem:mm_vmscan_isolate_pages   ( +-   9.084% )
> > > > 
> > > >    52.680621973  seconds time elapsed   ( +-   0.689% )
> > > > 
> > > > Is the testing patch written write? I don't understand what the number 
> > > > 14189 means? Does it make any sense?
> > > 
> > 
> > Broadly speaking
> > 
> > "For each of the 5 runs of hackbench, there were 14189 times the
> > kmem:mm_vmscan_isolate_pages was sampled  +/- 9.084%"
> > 
> > Without knowing how many times nr_taken_zero was positive, it's
> > difficult to tell whether 14189 is common or not.
> > 
> > > I don't think you need nr_taken_zeros at all.  You'd want something like
> > > 
> > > 	if (nr_taken == 0)
> > > 		trace_mm_vmscan_nr_taken_zero();
> > > 	else
> > > 		trace_mm_vmscan_nr_taken_nonzero();
> > > 
> > > which would pointlessly generate a huge stream of events which would
> > > have to be added up downstream, which is dumb.
> > > 
> > 
> > Dumb it might be, but perf acts as that aggregator. For the purposes of
> > debugging, it would be fine although it would not be a very suitable pair
> > of events to merge to mainline. A more sensible trace point for mainline
> > would record what nr_taken was so a higher-level tool could answer the zero
> > vs non-zero question or optionally do things like figure out how many pages
> > were being taken of the lists and being put back.
> > 
> > For this question though, use the two tracepoints with no additional parameters
> > and have perf how many times each event occurred.
> 
> Thank you for the explaintion. I am not sure I follow your discussion 
> correctly, no additional parameters means something like:
> 
> TRACE_EVENT(mm_vmscan_nr_taken_zero,
>        TP_PROTO( ),
>        TP_ARGS( ),
>        TP_STRUCT__entry( ),
>        TP_fast_assign( ),
>        TP_printk( )
> );
> 
> ? which looks strange to me and does not compile. I guess that is not what 
> you mean.
> 
> I ended up with a following patch:
> 
> ----PATCH BEGIN---
> 
> ---
> diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
> index eaf46bd..1f9e7bf 100644
> --- a/include/trace/events/kmem.h
> +++ b/include/trace/events/kmem.h
> @@ -388,6 +388,42 @@ TRACE_EVENT(mm_page_alloc_extfrag,
>  		__entry->alloc_migratetype == __entry->fallback_migratetype)
>  );
>  
> +TRACE_EVENT(mm_vmscan_nr_taken_zero,
> +
> +	TP_PROTO(unsigned long nr_taken),
> +
> +	TP_ARGS(nr_taken),
> +
> +	TP_STRUCT__entry(
> +		__field(        unsigned long,          nr_taken        )
> +	),
> +
> +	TP_fast_assign(
> +		__entry->nr_taken       = nr_taken;
> +	),
> +
> +	TP_printk("nr_taken=%lu",
> +	__entry->nr_taken)
> +);
> +
> +TRACE_EVENT(mm_vmscan_nr_taken_nonzero,
> +
> +	TP_PROTO(unsigned long nr_taken),
> +
> +	TP_ARGS(nr_taken),
> +
> +	TP_STRUCT__entry(
> +		__field(        unsigned long,          nr_taken        )
> +	),
> +
> +	TP_fast_assign(
> +		__entry->nr_taken       = nr_taken;
> +	),
> +
> +	TP_printk("nr_taken=%lu",
> +		__entry->nr_taken)
> +);
> +
>  #endif /* _TRACE_KMEM_H */
>  
>  /* This part must be outside protection */
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index ad93096..eec4099 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -40,6 +40,7 @@
>  #include <linux/memcontrol.h>
>  #include <linux/delayacct.h>
>  #include <linux/sysctl.h>
> +#include <trace/events/kmem.h>
>  
>  #include <asm/tlbflush.h>
>  #include <asm/div64.h>
> @@ -1322,7 +1323,9 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>  	__count_zone_vm_events(PGREFILL, zone, pgscanned);
>  
>  	if (nr_taken == 0)
> -		goto done;
> +		trace_mm_vmscan_nr_taken_zero(nr_taken);
> +	else
> +		trace_mm_vmscan_nr_taken_nonzero(nr_taken);
>  
>  	reclaim_stat->recent_scanned[file] += nr_taken;
>  	if (file)
> @@ -1388,7 +1391,6 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>  							nr_rotated);
>  	__mod_zone_page_state(zone, NR_INACTIVE_ANON + file * LRU_FILE,
>  							nr_deactivated);
> -done:
>  	spin_unlock_irq(&zone->lru_lock);
>  }
> 
> ----PATCH END---
>  
> /usr/src/mmotm-0903# perf stat --repeat 5 -e kmem:mm_vmscan_nr_taken_zero \
>  -e kmem:mm_vmscan_nr_taken_nonzero hackbench 100
> 
> Running with 100*40 (== 4000) tasks.
> Time: 41.599
> Running with 100*40 (== 4000) tasks.
> Time: 80.192
> Running with 100*40 (== 4000) tasks.
> Time: 26.451
> Running with 100*40 (== 4000) tasks.
> Time: 65.428
> Running with 100*40 (== 4000) tasks.
> Time: 30.054
> 
>  Performance counter stats for 'hackbench 100' (5 runs):
> 
>           10330  kmem:mm_vmscan_nr_taken_zero   ( +-  11.732% )
>            2601  kmem:mm_vmscan_nr_taken_nonzero   ( +-  10.876% )
> 
>    49.509328260  seconds time elapsed   ( +-   0.934% )
> 
> Sampling of nr_taken_zero is way bigger than sampling of nr_taken_nonzero 
> in the 5 hackbench runs. I thought the sampling result would be the 
> opposite. 
> 
> Maybe I get it all wrong :-).

You're right. the experiment said so. 
But hackbench performs fork-bomb test 
so that it makes corner case, I think.
Such a case shows the your patch is good.
But that case is rare. 

The thing remained is to test your patch
in normal case. so you need to test hackbench with 
smaller parameters to make for the number of task 
to fit your memory size but does happen reclaim.

> 
> 
> > 
> > > I don't know if the tracing code is capable of maintaining the counters
> > > for you.  Perhaps you _do_ need nr_taken_zeros.  In which case you want
> > > 
> > > 	if (nr_taken == 0) {
> > > 		nr_taken_zeros++;
> > > 		trace_mm_vmscan_isolate_pages_zero(nr_taken_zeros);
> > > 	} else {
> > > 		nr_taken_nonzeros++;
> > > 		trace_mm_vmscan_isolate_pages_nonzero(nr_taken_nonzeros);
> > > 	}
> > > 
> > > which is awkward.  Mel will know.
> > > 
> > 
> > I am not aware of a way of maintaining counters within tracing. The difficulty
> > is figuring out when that event should be recorded. Worse, the count is
> > being aggregated for all processes that enter this path so there is noise
> > in the value that cannot be filtered out. An aggregated counter like this
> > makes sense when reporting to /proc or for discovering via kgdb, but less
> > suitable for tracing.
> > 
> > > > > 
> > > > > The way I used to do stuff like this is:
> > > > > 
> > > > > int akpm1;
> > > > > int akpm2;
> > > > > 
> > > > > 	...
> > > > > 	if (nr_taken)
> > > > > 		akpm1++;
> > > > > 	else
> > > > > 		akpm2++;
> > > > > 
> > > > > then inspect the values of akpm1 and akpm2 in the running kernel using kgdb.
> > > 
> > > That's looking more attractive ;)
> > > 
> > 
> > It's simplier but it's global in nature so it's harder to figure out how much
> > of the counter is for the target workload and how much of it is everything
> > else. The main advantage of using the two tracepoints is that you know exactly
> > how many of the events were due to hackbench and not the rest of the system.
> > 
> > -- 
> > Mel Gorman
> > Part-time Phd Student                          Linux Technology Center
> > University of Limerick                         IBM Dublin Software Lab
> > 
> > 
> 
> Vincent Li
> Biomedical Research Center
> University of British Columbia


-- 
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Wed, 9 Sep 2009, Minchan Kim wrote:

>
> You're right. the experiment said so.
> But hackbench performs fork-bomb test
> so that it makes corner case, I think.
> Such a case shows the your patch is good.
> But that case is rare.
>
> The thing remained is to test your patch
> in normal case. so you need to test hackbench with
> smaller parameters to make for the number of task
> to fit your memory size but does happen reclaim.
>

Hi Kim,

I finally got some time to rerun the perf test and press Alt + SysRq 
+ M the same time  on a freshly start computer.

I run the perf with repeat only 1 instead of 5, so run hackbench 
with number 100 does not cause my system stall, the system  is still quite 
responsive during the test, I assume that is normal situation, not fork 
bomb case?

In general, it seems nr_taken_zero does happen in normal page reclaim 
situation, but it is also true that nr_taken_zero does not happen from 
time to time.


###1 run

root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
kmem:mm_vmscan_nr_taken_nonzero hackbench 80
Running with 80*40 (== 3200) tasks.
Time: 4.912

  Performance counter stats for 'hackbench 80':

               0  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
               0  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec

     5.286915156  seconds time elapsed


[   45.290044] SysRq : Show Memory
[   45.291132] active_anon:3283 inactive_anon:0 isolated_anon:0
[   45.291133]  active_file:2538 inactive_file:7964 isolated_file:0

###2 run

root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
kmem:mm_vmscan_nr_taken_nonzero hackbench 90
Running with 90*40 (== 3600) tasks.
Time: 12.548

  Performance counter stats for 'hackbench 90':

              76  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
             361  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec

    12.980680642  seconds time elapsed

[  324.098169] SysRq : Show Memory
[  324.099261] active_anon:3793 inactive_anon:1635 isolated_anon:590
[  324.099262]  active_file:1334 inactive_file:4262 isolated_file:0

###3 run

root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
kmem:mm_vmscan_nr_taken_nonzero hackbench 100
Running with 100*40 (== 4000) tasks.
Time: 47.296

  Performance counter stats for 'hackbench 100':

               0  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
            1064  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec

    47.765099490  seconds time elapsed

[  454.130625] SysRq : Show Memory
[  454.131718] active_anon:8375 inactive_anon:10350 isolated_anon:10285
[  454.131720]  active_file:1675 inactive_file:7148 isolated_file:30

###4 run

root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
kmem:mm_vmscan_nr_taken_nonzero hackbench 80
Running with 80*40 (== 3200) tasks.
Time: 4.790

  Performance counter stats for 'hackbench 80':

               0  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
               0  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec

     5.210933885  seconds time elapsed

[  599.514166] SysRq : Show Memory
[  599.515263] active_anon:27830 inactive_anon:114 isolated_anon:0
[  599.515264]  active_file:1195 inactive_file:3284 isolated_file:0

###5 run

root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
kmem:mm_vmscan_nr_taken_nonzero hackbench 90
Running with 90*40 (== 3600) tasks.
Time: 5.836

  Performance counter stats for 'hackbench 90':

               0  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
               0  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec

     6.258902896  seconds time elapsed

[  753.201247] SysRq : Show Memory
[  753.202346] active_anon:37091 inactive_anon:114 isolated_anon:0
[  753.202348]  active_file:1211 inactive_file:3314 isolated_file:0

###6 run

root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
kmem:mm_vmscan_nr_taken_nonzero hackbench 100
Running with 100*40 (== 4000) tasks.
Time: 6.445

  Performance counter stats for 'hackbench 100':

               0  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
               0  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec

     6.920834955  seconds time elapsed

[  836.228395] SysRq : Show Memory
[  836.229487] active_anon:30157 inactive_anon:114 isolated_anon:0
[  836.229488]  active_file:1217 inactive_file:3338 isolated_file:0

###7 run

root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
kmem:mm_vmscan_nr_taken_nonzero hackbench 120
Running with 120*40 (== 4800) tasks.
Time: 66.182

  Performance counter stats for 'hackbench 120':

            3307  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
            1218  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec

    66.767057051  seconds time elapsed

[  927.855061] SysRq : Show Memory
[  927.856156] active_anon:11320 inactive_anon:11962 isolated_anon:11879
[  927.856157]  active_file:1220 inactive_file:3253 isolated_file:0

###8 run
root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
kmem:mm_vmscan_nr_taken_nonzero hackbench 110
Running with 110*40 (== 4400) tasks.
Time: 47.128

  Performance counter stats for 'hackbench 110':

               6  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
             934  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec

    47.657109224  seconds time elapsed

[ 1058.031490] SysRq : Show Memory
[ 1058.032573] active_anon:15351 inactive_anon:245 isolated_anon:23350
[ 1058.032574]  active_file:2112 inactive_file:5036 isolated_file:0

###9 run

root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
kmem:mm_vmscan_nr_taken_nonzero hackbench 100
Running with 100*40 (== 4000) tasks.
Time: 14.223

  Performance counter stats for 'hackbench 100':

               9  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
             382  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec

    14.773145947  seconds time elapsed

[ 1242.620748] SysRq : Show Memory
[ 1242.621843] active_anon:5926 inactive_anon:3066 isolated_anon:788
[ 1242.621844]  active_file:1297 inactive_file:3145 isolated_file:0

###10 run

root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
kmem:mm_vmscan_nr_taken_nonzero hackbench 110
Running with 110*40 (== 4400) tasks.
Time: 39.346

  Performance counter stats for 'hackbench 110':

             367  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
             810  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec

    39.880113992  seconds time elapsed

[ 1346.694702] SysRq : Show Memory
[ 1346.695797] active_anon:12729 inactive_anon:6726 isolated_anon:3804
[ 1346.695798]  active_file:1311 inactive_file:3141 isolated_file:0

Thanks,

Vincent

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Thu, 15 Oct 2009, Vincent Li wrote:

> 
> 
> On Wed, 9 Sep 2009, Minchan Kim wrote:
> 
> > 
> > You're right. the experiment said so.
> > But hackbench performs fork-bomb test
> > so that it makes corner case, I think.
> > Such a case shows the your patch is good.
> > But that case is rare.
> > 
> > The thing remained is to test your patch
> > in normal case. so you need to test hackbench with
> > smaller parameters to make for the number of task
> > to fit your memory size but does happen reclaim.
> > 
> 
> Hi Kim,
> 
> I finally got some time to rerun the perf test and press Alt + SysRq + M the
> same time  on a freshly start computer.
> 
> I run the perf with repeat only 1 instead of 5, so run hackbench with number
> 100 does not cause my system stall, the system  is still quite responsive
> during the test, I assume that is normal situation, not fork bomb case?
> 
> In general, it seems nr_taken_zero does happen in normal page reclaim
> situation, but it is also true that nr_taken_zero does not happen from time to
> time.
> 

Oh, The nr_taken_zero/nonzero event tracing test patch is based on 
2.6.32-rc4 as below:

---
diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
index eaf46bd..a94daa0 100644
--- a/include/trace/events/kmem.h
+++ b/include/trace/events/kmem.h
@@ -388,6 +388,42 @@ TRACE_EVENT(mm_page_alloc_extfrag,
 		__entry->alloc_migratetype == __entry->fallback_migratetype)
 );
 
+TRACE_EVENT(mm_vmscan_nr_taken_zero,
+
+	TP_PROTO(unsigned long nr_taken),
+
+	TP_ARGS(nr_taken),
+
+	TP_STRUCT__entry(
+		__field(        unsigned long,          nr_taken        )
+	),
+
+	TP_fast_assign(
+		__entry->nr_taken       = nr_taken;
+	),
+
+	TP_printk("nr_taken=%lu",
+		__entry->nr_taken)
+);
+
+TRACE_EVENT(mm_vmscan_nr_taken_nonzero,
+
+	TP_PROTO(unsigned long nr_taken),
+
+	TP_ARGS(nr_taken),
+
+	TP_STRUCT__entry(
+		__field(        unsigned long,          nr_taken        )
+	),
+
+	TP_fast_assign(
+		__entry->nr_taken       = nr_taken;
+	),
+
+	TP_printk("nr_taken=%lu",
+		__entry->nr_taken)
+);
+
 #endif /* _TRACE_KMEM_H */
 
 /* This part must be outside protection */
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 64e4388..36e7fe2 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1326,6 +1326,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
 	reclaim_stat->recent_scanned[file] += nr_taken;
 
 	__count_zone_vm_events(PGREFILL, zone, pgscanned);
+
+	if (nr_taken == 0)
+		trace_mm_vmscan_nr_taken_zero(nr_taken);
+	else
+		trace_mm_vmscan_nr_taken_nonzero(nr_taken);
+
 	if (file)
 		__mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
 	else


Regards,

Vincent

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Minchan Kim]

Hi, Vicent. 
First of all, Thanks for your effort. :)

On Thu, 15 Oct 2009 15:47:07 -0700 (PDT)
Vincent Li <root@brc.ubc.ca> wrote:

> 
> 
> On Wed, 9 Sep 2009, Minchan Kim wrote:
> 
> >
> > You're right. the experiment said so.
> > But hackbench performs fork-bomb test
> > so that it makes corner case, I think.
> > Such a case shows the your patch is good.
> > But that case is rare.
> >
> > The thing remained is to test your patch
> > in normal case. so you need to test hackbench with
> > smaller parameters to make for the number of task
> > to fit your memory size but does happen reclaim.
> >
> 
> Hi Kim,
> 
> I finally got some time to rerun the perf test and press Alt + SysRq 
> + M the same time  on a freshly start computer.

Your sysrq would catch mem info at random time during hackbench execution.
So, it wouldn't have a consistency. but Your data said somethings. 

> 
> I run the perf with repeat only 1 instead of 5, so run hackbench 
> with number 100 does not cause my system stall, the system  is still quite 
> responsive during the test, I assume that is normal situation, not fork 
> bomb case?

Hackbench make many process in short time so kernel allocates many anon pages
for processes. So we call it 'fork bomb'. 

> 
> In general, it seems nr_taken_zero does happen in normal page reclaim 
> situation, but it is also true that nr_taken_zero does not happen from 
> time to time.
> 
> ###1 run
> 
> root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
> kmem:mm_vmscan_nr_taken_nonzero hackbench 80
> Running with 80*40 (== 3200) tasks.
> Time: 4.912
> 
>   Performance counter stats for 'hackbench 80':
> 
>                0  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
>                0  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec
> 
>      5.286915156  seconds time elapsed
> 
> 
> [   45.290044] SysRq : Show Memory
> [   45.291132] active_anon:3283 inactive_anon:0 isolated_anon:0
> [   45.291133]  active_file:2538 inactive_file:7964 isolated_file:0
> 
> ###2 run
> 
> root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
> kmem:mm_vmscan_nr_taken_nonzero hackbench 90
> Running with 90*40 (== 3600) tasks.
> Time: 12.548
> 
>   Performance counter stats for 'hackbench 90':
> 
>               76  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
>              361  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec
> 
>     12.980680642  seconds time elapsed
> 
> [  324.098169] SysRq : Show Memory
> [  324.099261] active_anon:3793 inactive_anon:1635 isolated_anon:590
> [  324.099262]  active_file:1334 inactive_file:4262 isolated_file:0

isolated_anon said us there are many processes which need reclaim 
in anon list in your system. So it would be a situation as fork bomb. 
But, it's not heavy. 

> ###3 run
> 
> root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
> kmem:mm_vmscan_nr_taken_nonzero hackbench 100
> Running with 100*40 (== 4000) tasks.
> Time: 47.296
> 
>   Performance counter stats for 'hackbench 100':
> 
>                0  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
>             1064  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec
> 
>     47.765099490  seconds time elapsed
> 
> [  454.130625] SysRq : Show Memory
> [  454.131718] active_anon:8375 inactive_anon:10350 isolated_anon:10285
> [  454.131720]  active_file:1675 inactive_file:7148 isolated_file:30

It's so heavy. isolated anon is bigger than active_anon. 
Nontheless, nr_taken_zero count is zero. 
perhaps, VM would select good pages in anon list.
It's good. 

> 
> ###4 run
> 
> root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
> kmem:mm_vmscan_nr_taken_nonzero hackbench 80
> Running with 80*40 (== 3200) tasks.
> Time: 4.790
> 
>   Performance counter stats for 'hackbench 80':
> 
>                0  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
>                0  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec
> 
>      5.210933885  seconds time elapsed
> 
> [  599.514166] SysRq : Show Memory
> [  599.515263] active_anon:27830 inactive_anon:114 isolated_anon:0
> [  599.515264]  active_file:1195 inactive_file:3284 isolated_file:0
> 
> ###5 run
> 
> root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
> kmem:mm_vmscan_nr_taken_nonzero hackbench 90
> Running with 90*40 (== 3600) tasks.
> Time: 5.836
> 
>   Performance counter stats for 'hackbench 90':
> 
>                0  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
>                0  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec
> 
>      6.258902896  seconds time elapsed
> 
> [  753.201247] SysRq : Show Memory
> [  753.202346] active_anon:37091 inactive_anon:114 isolated_anon:0
> [  753.202348]  active_file:1211 inactive_file:3314 isolated_file:0
> 
> ###6 run
> 
> root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
> kmem:mm_vmscan_nr_taken_nonzero hackbench 100
> Running with 100*40 (== 4000) tasks.
> Time: 6.445
> 
>   Performance counter stats for 'hackbench 100':
> 
>                0  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
>                0  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec
> 
>      6.920834955  seconds time elapsed
> 
> [  836.228395] SysRq : Show Memory
> [  836.229487] active_anon:30157 inactive_anon:114 isolated_anon:0
> [  836.229488]  active_file:1217 inactive_file:3338 isolated_file:0
> 
> ###7 run
> 
> root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
> kmem:mm_vmscan_nr_taken_nonzero hackbench 120
> Running with 120*40 (== 4800) tasks.
> Time: 66.182
> 
>   Performance counter stats for 'hackbench 120':
> 
>             3307  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
>             1218  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec
> 
>     66.767057051  seconds time elapsed
> 
> [  927.855061] SysRq : Show Memory
> [  927.856156] active_anon:11320 inactive_anon:11962 isolated_anon:11879
> [  927.856157]  active_file:1220 inactive_file:3253 isolated_file:0

It's so heavy, too. This case is good for proving your concept. 
But as your data said, it's rare case. 

> 
> ###8 run
> root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
> kmem:mm_vmscan_nr_taken_nonzero hackbench 110
> Running with 110*40 (== 4400) tasks.
> Time: 47.128
> 
>   Performance counter stats for 'hackbench 110':
> 
>                6  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
>              934  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec
> 
>     47.657109224  seconds time elapsed
> 
> [ 1058.031490] SysRq : Show Memory
> [ 1058.032573] active_anon:15351 inactive_anon:245 isolated_anon:23350
> [ 1058.032574]  active_file:2112 inactive_file:5036 isolated_file:0
> 
> ###9 run
> 
> root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
> kmem:mm_vmscan_nr_taken_nonzero hackbench 100
> Running with 100*40 (== 4000) tasks.
> Time: 14.223
> 
>   Performance counter stats for 'hackbench 100':
> 
>                9  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
>              382  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec
> 
>     14.773145947  seconds time elapsed
> 
> [ 1242.620748] SysRq : Show Memory
> [ 1242.621843] active_anon:5926 inactive_anon:3066 isolated_anon:788
> [ 1242.621844]  active_file:1297 inactive_file:3145 isolated_file:0
> 
> ###10 run
> 
> root@kernalhack:~# perf stat --repeat 1 -e kmem:mm_vmscan_nr_taken_zero -e 
> kmem:mm_vmscan_nr_taken_nonzero hackbench 110
> Running with 110*40 (== 4400) tasks.
> Time: 39.346
> 
>   Performance counter stats for 'hackbench 110':
> 
>              367  kmem:mm_vmscan_nr_taken_zero #      0.000 M/sec
>              810  kmem:mm_vmscan_nr_taken_nonzero #      0.000 M/sec
> 
>     39.880113992  seconds time elapsed
> 
> [ 1346.694702] SysRq : Show Memory
> [ 1346.695797] active_anon:12729 inactive_anon:6726 isolated_anon:3804
> [ 1346.695798]  active_file:1311 inactive_file:3141 isolated_file:0
> 
> Thanks,
> 
> Vincent

But as your data said, on usual case, nr_taken_zero count is much less 
than non_zero. so we could lost benefit in normal case due to compare
insturction although it's trivial. 

I have no objection in this patch since overhead is not so big.
But I am not sure what other guys think about it. 

How about adding unlikely following as ?

+
+       if (unlikely(nr_taken == 0))
+               goto done;

-- 
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Wu Fengguang]

On Fri, Oct 16, 2009 at 10:10:41AM +0800, Minchan Kim wrote:
> Hi, Vicent. 
> First of all, Thanks for your effort. :)
 
That's pretty serious efforts ;)

> But as your data said, on usual case, nr_taken_zero count is much less 
> than non_zero. so we could lost benefit in normal case due to compare
> insturction although it's trivial. 
> 
> I have no objection in this patch since overhead is not so big.
> But I am not sure what other guys think about it. 
> 
> How about adding unlikely following as ?
> 
> +
> +       if (unlikely(nr_taken == 0))
> +               goto done;

I would prefer to just remove it - to make the code simple :)

Thanks,
Fengguang

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[KOSAKI Motohiro]

> On Fri, Oct 16, 2009 at 10:10:41AM +0800, Minchan Kim wrote:
> > Hi, Vicent. 
> > First of all, Thanks for your effort. :)
>  
> That's pretty serious efforts ;)
> 
> > But as your data said, on usual case, nr_taken_zero count is much less 
> > than non_zero. so we could lost benefit in normal case due to compare
> > insturction although it's trivial. 
> > 
> > I have no objection in this patch since overhead is not so big.
> > But I am not sure what other guys think about it. 
> > 
> > How about adding unlikely following as ?
> > 
> > +
> > +       if (unlikely(nr_taken == 0))
> > +               goto done;
> 
> I would prefer to just remove it - to make the code simple :)

+1 me.

Thank you, Vincent. Your effort was pretty clear and good.
but your mesurement data didn't persuade us.




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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Fri, 16 Oct 2009, KOSAKI Motohiro wrote:

> > I would prefer to just remove it - to make the code simple :)
> 
> +1 me.
> 
> Thank you, Vincent. Your effort was pretty clear and good.
> but your mesurement data didn't persuade us.

That is all right :). While it takes time, but I enjoy it.

Thank you all for the reviewing. 

Andrew: Would you please drop the mm-vsmcan-check-shrink_active_list-sc-isolate_pages-return-value.patch 
patch from mmotm? Thank you for your time.

Thanks,

Vincent

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[KOSAKI Motohiro]

> > On Fri, Oct 16, 2009 at 10:10:41AM +0800, Minchan Kim wrote:
> > > Hi, Vicent. 
> > > First of all, Thanks for your effort. :)
> >  
> > That's pretty serious efforts ;)
> > 
> > > But as your data said, on usual case, nr_taken_zero count is much less 
> > > than non_zero. so we could lost benefit in normal case due to compare
> > > insturction although it's trivial. 
> > > 
> > > I have no objection in this patch since overhead is not so big.
> > > But I am not sure what other guys think about it. 
> > > 
> > > How about adding unlikely following as ?
> > > 
> > > +
> > > +       if (unlikely(nr_taken == 0))
> > > +               goto done;
> > 
> > I would prefer to just remove it - to make the code simple :)
> 
> +1 me.
> 
> Thank you, Vincent. Your effort was pretty clear and good.
> but your mesurement data didn't persuade us.

This patch still exist in current mmotm.

Andrew, can you please drop mm-vsmcan-check-shrink_active_list-sc-isolate_pages-return-value.patch?
Or do you have any remain reason.




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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Mel Gorman]

On Tue, Sep 08, 2009 at 03:39:59PM -0700, Vincent Li wrote:
> On Tue, 8 Sep 2009, Mel Gorman wrote:
> 
> > On Fri, Sep 04, 2009 at 04:53:05PM -0700, Andrew Morton wrote:
> > > On Fri, 4 Sep 2009 14:39:32 -0700 (PDT)
> > > Vincent Li <macli@brc.ubc.ca> wrote:
> > > 
> > > Well you want to count two things: 1: how many times nr_taken==0 and 2:
> > > how many times nr_taken!=0.
> > > 
> > 
> > Indeed. I'm not aware of the specifics that led to this patch, but minimally
> > one would be interested in the exact value of nr_taken as it can be used to
> > answer more than one question.
> > 
> > > > Then I got test result with:
> > > > 
> > > > root@kernelhack:/usr/src/mmotm-0903# perf  stat --repeat 5  -e \ 
> > > > kmem:mm_vmscan_isolate_pages hackbench 100
> > > > 
> > > > Running with 100*40 (== 4000) tasks.
> > > > Time: 52.736
> > > > Running with 100*40 (== 4000) tasks.
> > > > Time: 64.982
> > > > Running with 100*40 (== 4000) tasks.
> > > > Time: 56.866
> > > > Running with 100*40 (== 4000) tasks.
> > > > Time: 37.137
> > > > Running with 100*40 (== 4000) tasks.
> > > > Time: 48.415
> > > > 
> > > >  Performance counter stats for 'hackbench 100' (5 runs):
> > > > 
> > > >           14189  kmem:mm_vmscan_isolate_pages   ( +-   9.084% )
> > > > 
> > > >    52.680621973  seconds time elapsed   ( +-   0.689% )
> > > > 
> > > > Is the testing patch written write? I don't understand what the number 
> > > > 14189 means? Does it make any sense?
> > > 
> > 
> > Broadly speaking
> > 
> > "For each of the 5 runs of hackbench, there were 14189 times the
> > kmem:mm_vmscan_isolate_pages was sampled  +/- 9.084%"
> > 
> > Without knowing how many times nr_taken_zero was positive, it's
> > difficult to tell whether 14189 is common or not.
> > 
> > > I don't think you need nr_taken_zeros at all.  You'd want something like
> > > 
> > > 	if (nr_taken == 0)
> > > 		trace_mm_vmscan_nr_taken_zero();
> > > 	else
> > > 		trace_mm_vmscan_nr_taken_nonzero();
> > > 
> > > which would pointlessly generate a huge stream of events which would
> > > have to be added up downstream, which is dumb.
> > > 
> > 
> > Dumb it might be, but perf acts as that aggregator. For the purposes of
> > debugging, it would be fine although it would not be a very suitable pair
> > of events to merge to mainline. A more sensible trace point for mainline
> > would record what nr_taken was so a higher-level tool could answer the zero
> > vs non-zero question or optionally do things like figure out how many pages
> > were being taken of the lists and being put back.
> > 
> > For this question though, use the two tracepoints with no additional parameters
> > and have perf how many times each event occurred.
> 
> Thank you for the explaintion. I am not sure I follow your discussion 
> correctly, no additional parameters means something like:
> 
> TRACE_EVENT(mm_vmscan_nr_taken_zero,
>        TP_PROTO( ),
>        TP_ARGS( ),
>        TP_STRUCT__entry( ),
>        TP_fast_assign( ),
>        TP_printk( )
> );

My bad, I was expecting something like

TRACE_EVENT(mm_vmscan_nr_taken_zero,
	TP_PROTO(void),
	TP_ARGS(),
	TP_printk("nr_taken_zero");
);

to work in the same way it does for DECLARE_TRACE but that is not the
case.

> ? which looks strange to me and does not compile. I guess that is not what 
> you mean.
> 

No, it's what I meant all right. As you were not using the value of
nr_taken, the information was redundant to store in the trace record.

> I ended up with a following patch:
> 
> ----PATCH BEGIN---
> 
> ---
> diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h
> index eaf46bd..1f9e7bf 100644
> --- a/include/trace/events/kmem.h
> +++ b/include/trace/events/kmem.h
> @@ -388,6 +388,42 @@ TRACE_EVENT(mm_page_alloc_extfrag,
>  		__entry->alloc_migratetype == __entry->fallback_migratetype)
>  );
>  
> +TRACE_EVENT(mm_vmscan_nr_taken_zero,
> +
> +	TP_PROTO(unsigned long nr_taken),
> +
> +	TP_ARGS(nr_taken),
> +
> +	TP_STRUCT__entry(
> +		__field(        unsigned long,          nr_taken        )
> +	),
> +
> +	TP_fast_assign(
> +		__entry->nr_taken       = nr_taken;
> +	),
> +
> +	TP_printk("nr_taken=%lu",
> +	__entry->nr_taken)
> +);
> +
> +TRACE_EVENT(mm_vmscan_nr_taken_nonzero,
> +
> +	TP_PROTO(unsigned long nr_taken),
> +
> +	TP_ARGS(nr_taken),
> +
> +	TP_STRUCT__entry(
> +		__field(        unsigned long,          nr_taken        )
> +	),
> +
> +	TP_fast_assign(
> +		__entry->nr_taken       = nr_taken;
> +	),
> +
> +	TP_printk("nr_taken=%lu",
> +		__entry->nr_taken)
> +);
> +
>  #endif /* _TRACE_KMEM_H */
>  
>  /* This part must be outside protection */
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index ad93096..eec4099 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -40,6 +40,7 @@
>  #include <linux/memcontrol.h>
>  #include <linux/delayacct.h>
>  #include <linux/sysctl.h>
> +#include <trace/events/kmem.h>
>  
>  #include <asm/tlbflush.h>
>  #include <asm/div64.h>
> @@ -1322,7 +1323,9 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>  	__count_zone_vm_events(PGREFILL, zone, pgscanned);
>  
>  	if (nr_taken == 0)
> -		goto done;
> +		trace_mm_vmscan_nr_taken_zero(nr_taken);
> +	else
> +		trace_mm_vmscan_nr_taken_nonzero(nr_taken);
>  
>  	reclaim_stat->recent_scanned[file] += nr_taken;
>  	if (file)
> @@ -1388,7 +1391,6 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>  							nr_rotated);
>  	__mod_zone_page_state(zone, NR_INACTIVE_ANON + file * LRU_FILE,
>  							nr_deactivated);
> -done:
>  	spin_unlock_irq(&zone->lru_lock);
>  }
> 
> ----PATCH END---
>  
> /usr/src/mmotm-0903# perf stat --repeat 5 -e kmem:mm_vmscan_nr_taken_zero \
>  -e kmem:mm_vmscan_nr_taken_nonzero hackbench 100
> 
> Running with 100*40 (== 4000) tasks.
> Time: 41.599
> Running with 100*40 (== 4000) tasks.
> Time: 80.192
> Running with 100*40 (== 4000) tasks.
> Time: 26.451
> Running with 100*40 (== 4000) tasks.
> Time: 65.428
> Running with 100*40 (== 4000) tasks.
> Time: 30.054
> 
>  Performance counter stats for 'hackbench 100' (5 runs):
> 
>           10330  kmem:mm_vmscan_nr_taken_zero   ( +-  11.732% )
>            2601  kmem:mm_vmscan_nr_taken_nonzero   ( +-  10.876% )
> 
>    49.509328260  seconds time elapsed   ( +-   0.934% )
> 
> Sampling of nr_taken_zero is way bigger than sampling of nr_taken_nonzero 
> in the 5 hackbench runs. I thought the sampling result would be the 
> opposite. 
> 
> Maybe I get it all wrong :-).
> 

As pointed out in another mail, the remaining question would be if this
situation is specific to a fork-bomb situation like hackbench or whether
it happens in reclaim generally.

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

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Minchan Kim]

On Fri, Sep 4, 2009 at 6:06 AM, Andrew Morton<akpm@linux-foundation.org> wrote:
> On Wed,  2 Sep 2009 16:49:25 -0700
> Vincent Li <macli@brc.ubc.ca> wrote:
>
>> If we can't isolate pages from LRU list, we don't have to account page movement, either.
>> Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
>>
>> This patch removes unnecessary overhead of page accounting
>> and locking in shrink_active_list as follow-up work of commit 5343daceec.
>>
>> Signed-off-by: Vincent Li <macli@brc.ubc.ca>
>> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
>> Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
>> Acked-by: Rik van Riel <riel@redhat.com>
>>
>> ---
>>  mm/vmscan.c |    9 +++++++--
>>  1 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 460a6f7..2d1c846 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>>       if (scanning_global_lru(sc)) {
>>               zone->pages_scanned += pgscanned;
>>       }
>> -     reclaim_stat->recent_scanned[file] += nr_taken;
>> -
>>       __count_zone_vm_events(PGREFILL, zone, pgscanned);
>> +
>> +     if (nr_taken == 0)
>> +             goto done;
>> +
>> +     reclaim_stat->recent_scanned[file] += nr_taken;
>>       if (file)
>>               __mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
>>       else
>> @@ -1383,6 +1386,8 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
>>       __mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
>>       __mod_zone_page_state(zone, LRU_ACTIVE + file * LRU_FILE, nr_rotated);
>>       __mod_zone_page_state(zone, LRU_BASE + file * LRU_FILE, nr_deactivated);
>> +
>> +done:
>>       spin_unlock_irq(&zone->lru_lock);
>>  }
>
> How do we know this patch is a net gain?
>
> IOW, with what frequency is `nr_taken' zero here?

I think It's not so simple.

In fact, the probability of (nr_taken == 0)
would be very low in active list.

If we verify the benefit, we have to measure trade-off between
loss of compare instruction in most case and
gain of avoiding unnecessary overheads in rare case through
micro-benchmark. I don't know which benchmark can do it.

but if we can know the number of frequent and it's very low,
we can add 'unlikely(if (nr_taken==0))' at least, I think.


-- 
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Andrew Morton]

On Fri, 4 Sep 2009 10:37:17 +0900 Minchan Kim <minchan.kim@gmail.com> wrote:

> On Fri, Sep 4, 2009 at 6:06 AM, Andrew Morton<akpm@linux-foundation.org> wrote:
> > On Wed, __2 Sep 2009 16:49:25 -0700
> > Vincent Li <macli@brc.ubc.ca> wrote:
> >
> >> If we can't isolate pages from LRU list, we don't have to account page movement, either.
> >> Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> >>
> >> This patch removes unnecessary overhead of page accounting
> >> and locking in shrink_active_list as follow-up work of commit 5343daceec.
> >>
> >> Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> >> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> >> Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> >> Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> >> Acked-by: Rik van Riel <riel@redhat.com>
> >>
> >> ---
> >> __mm/vmscan.c | __ __9 +++++++--
> >> __1 files changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/mm/vmscan.c b/mm/vmscan.c
> >> index 460a6f7..2d1c846 100644
> >> --- a/mm/vmscan.c
> >> +++ b/mm/vmscan.c
> >> @@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> >> __ __ __ if (scanning_global_lru(sc)) {
> >> __ __ __ __ __ __ __ zone->pages_scanned += pgscanned;

Someone's email client is replacing 0x09 with 0xa0, dammit.

> >
> > IOW, with what frequency is `nr_taken' zero here?
> 
> I think It's not so simple.
> 
> In fact, the probability of (nr_taken == 0)
> would be very low in active list.
> 
> If we verify the benefit, we have to measure trade-off between
> loss of compare instruction in most case and
> gain of avoiding unnecessary overheads in rare case through
> micro-benchmark. I don't know which benchmark can do it.
> 
> but if we can know the number of frequent and it's very low,
> we can add 'unlikely(if (nr_taken==0))' at least, I think.

But the test-n-branch is not the only cost of this change.

The more worrisome effect is that we've added a rarely-taken long
branch which skips over a whole lot of code.  There's an appreciable
risk that we'll later add code to this function in the expectation that
the new code is always executed.  During testing, the code is executed
sufficiently often for the bug to not be noticed.  So we ship the buggy
code.

IOW, there is a maintainability cost as well.

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Thu, 3 Sep 2009, Andrew Morton wrote:

> On Fri, 4 Sep 2009 10:37:17 +0900 Minchan Kim <minchan.kim@gmail.com> wrote:
> 
> > On Fri, Sep 4, 2009 at 6:06 AM, Andrew Morton<akpm@linux-foundation.org> wrote:
> > > On Wed, __2 Sep 2009 16:49:25 -0700
> > > Vincent Li <macli@brc.ubc.ca> wrote:
> > >
> > >> If we can't isolate pages from LRU list, we don't have to account page movement, either.
> > >> Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> > >>
> > >> This patch removes unnecessary overhead of page accounting
> > >> and locking in shrink_active_list as follow-up work of commit 5343daceec.
> > >>
> > >> Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> > >> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> > >> Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > >> Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> > >> Acked-by: Rik van Riel <riel@redhat.com>
> > >>
> > >> ---
> > >> __mm/vmscan.c | __ __9 +++++++--
> > >> __1 files changed, 7 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/mm/vmscan.c b/mm/vmscan.c
> > >> index 460a6f7..2d1c846 100644
> > >> --- a/mm/vmscan.c
> > >> +++ b/mm/vmscan.c
> > >> @@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> > >> __ __ __ if (scanning_global_lru(sc)) {
> > >> __ __ __ __ __ __ __ zone->pages_scanned += pgscanned;
> 
> Someone's email client is replacing 0x09 with 0xa0, dammit.

I am using alpine 2.0, I got:

 [ Sending Preferences ]
      [X]  Do Not Send Flowed Text                                               
      [ ]  Downgrade Multipart to Text                                           
      [X]  Enable 8bit ESMTP Negotiation    (default)
      [ ]  Strip Whitespace Before Sending                                       
 
And Documentation/email-clients.txt have:

Config options:
- quell-flowed-text is needed for recent versions
- the "no-strip-whitespace-before-send" option is needed

Am I the one to blame? Should I uncheck the 'Do Not Send Flowed Text'? I 
am sorry if it is my fault.

Vincent

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Thu, 3 Sep 2009, Vincent Li wrote:

> On Thu, 3 Sep 2009, Andrew Morton wrote:
> 
> > On Fri, 4 Sep 2009 10:37:17 +0900 Minchan Kim <minchan.kim@gmail.com> wrote:
> > 
> > > On Fri, Sep 4, 2009 at 6:06 AM, Andrew Morton<akpm@linux-foundation.org> wrote:
> > > > On Wed, __2 Sep 2009 16:49:25 -0700
> > > > Vincent Li <macli@brc.ubc.ca> wrote:
> > > >
> > > >> If we can't isolate pages from LRU list, we don't have to account page movement, either.
> > > >> Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> > > >>
> > > >> This patch removes unnecessary overhead of page accounting
> > > >> and locking in shrink_active_list as follow-up work of commit 5343daceec.
> > > >>
> > > >> Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> > > >> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> > > >> Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > > >> Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> > > >> Acked-by: Rik van Riel <riel@redhat.com>
> > > >>
> > > >> ---
> > > >> __mm/vmscan.c | __ __9 +++++++--
> > > >> __1 files changed, 7 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > >> index 460a6f7..2d1c846 100644
> > > >> --- a/mm/vmscan.c
> > > >> +++ b/mm/vmscan.c
> > > >> @@ -1319,9 +1319,12 @@ static void shrink_active_list(unsigned long nr_pages, struct zone *zone,
> > > >> __ __ __ if (scanning_global_lru(sc)) {
> > > >> __ __ __ __ __ __ __ zone->pages_scanned += pgscanned;
> > 
> > Someone's email client is replacing 0x09 with 0xa0, dammit.
> 
> I am using alpine 2.0, I got:
> 
>  [ Sending Preferences ]
>       [X]  Do Not Send Flowed Text                                               
>       [ ]  Downgrade Multipart to Text                                           
>       [X]  Enable 8bit ESMTP Negotiation    (default)
>       [ ]  Strip Whitespace Before Sending                                       
>  
> And Documentation/email-clients.txt have:
> 
> Config options:
> - quell-flowed-text is needed for recent versions
> - the "no-strip-whitespace-before-send" option is needed
> 
> Am I the one to blame? Should I uncheck the 'Do Not Send Flowed Text'? I 
> am sorry if it is my fault.

Ah, I quoted the pine Config options, the alpine config options from 
Documentation/email-clients.txt should be:

Config options:
In the "Sending Preferences" section:

- "Do Not Send Flowed Text" must be enabled
- "Strip Whitespace Before Sending" must be disabled

and my alpine did follow the recommendations as above showed.

I used 'git send-email' to send out the original patch.

Sorry again for the noise. 

Vincent Li
Biomedical Research Center
University of British Columbia

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[KOSAKI Motohiro]

> >  [ Sending Preferences ]
> >       [X]  Do Not Send Flowed Text                                               
> >       [ ]  Downgrade Multipart to Text                                           
> >       [X]  Enable 8bit ESMTP Negotiation    (default)
> >       [ ]  Strip Whitespace Before Sending                                       
> >  
> > And Documentation/email-clients.txt have:
> > 
> > Config options:
> > - quell-flowed-text is needed for recent versions
> > - the "no-strip-whitespace-before-send" option is needed
> > 
> > Am I the one to blame? Should I uncheck the 'Do Not Send Flowed Text'? I 
> > am sorry if it is my fault.
> 
> Ah, I quoted the pine Config options, the alpine config options from 
> Documentation/email-clients.txt should be:
> 
> Config options:
> In the "Sending Preferences" section:
> 
> - "Do Not Send Flowed Text" must be enabled
> - "Strip Whitespace Before Sending" must be disabled

Can you please make email-clients.txt fixing patch too? :-)



> 
> and my alpine did follow the recommendations as above showed.
> 
> I used 'git send-email' to send out the original patch.


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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Mon, 7 Sep 2009, KOSAKI Motohiro wrote:

> > >  [ Sending Preferences ]
> > >       [X]  Do Not Send Flowed Text                                               
> > >       [ ]  Downgrade Multipart to Text                                           
> > >       [X]  Enable 8bit ESMTP Negotiation    (default)
> > >       [ ]  Strip Whitespace Before Sending                                       
> > >  
> > > And Documentation/email-clients.txt have:
> > > 
> > > Config options:
> > > - quell-flowed-text is needed for recent versions
> > > - the "no-strip-whitespace-before-send" option is needed
> > > 
> > > Am I the one to blame? Should I uncheck the 'Do Not Send Flowed Text'? I 
> > > am sorry if it is my fault.
> > 
> > Ah, I quoted the pine Config options, the alpine config options from 
> > Documentation/email-clients.txt should be:
> > 
> > Config options:
> > In the "Sending Preferences" section:
> > 
> > - "Do Not Send Flowed Text" must be enabled
> > - "Strip Whitespace Before Sending" must be disabled
> 
> Can you please make email-clients.txt fixing patch too? :-)

Sorry my poor written English make you confused.:-). The two config 
options for alpine are already in email-clients.txt and I followed the existing config options 
recommendation. I am not sure if my alpine is the faulty email client. Is 
there still something missing with alpine? 

> 
> 
> > 
> > and my alpine did follow the recommendations as above showed.
> > 
> > I used 'git send-email' to send out the original patch.
> 
> 
> 
> 

Vincent Li
Biomedical Research Center
University of British Columbia

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[KOSAKI Motohiro]

> On Mon, 7 Sep 2009, KOSAKI Motohiro wrote:
> 
> > > >  [ Sending Preferences ]
> > > >       [X]  Do Not Send Flowed Text                                               
> > > >       [ ]  Downgrade Multipart to Text                                           
> > > >       [X]  Enable 8bit ESMTP Negotiation    (default)
> > > >       [ ]  Strip Whitespace Before Sending                                       
> > > >  
> > > > And Documentation/email-clients.txt have:
> > > > 
> > > > Config options:
> > > > - quell-flowed-text is needed for recent versions
> > > > - the "no-strip-whitespace-before-send" option is needed
> > > > 
> > > > Am I the one to blame? Should I uncheck the 'Do Not Send Flowed Text'? I 
> > > > am sorry if it is my fault.
> > > 
> > > Ah, I quoted the pine Config options, the alpine config options from 
> > > Documentation/email-clients.txt should be:
> > > 
> > > Config options:
> > > In the "Sending Preferences" section:
> > > 
> > > - "Do Not Send Flowed Text" must be enabled
> > > - "Strip Whitespace Before Sending" must be disabled
> > 
> > Can you please make email-clients.txt fixing patch too? :-)
> 
> Sorry my poor written English make you confused.:-). The two config 
> options for alpine are already in email-clients.txt and I followed the existing config options 
> recommendation. 

I see. sorry my misunderstood.
thanks :)

> I am not sure if my alpine is the faulty email client. Is 
> there still something missing with alpine? 



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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Johannes Weiner]

On Tue, Sep 08, 2009 at 11:32:45AM -0700, Vincent Li wrote:
> On Mon, 7 Sep 2009, KOSAKI Motohiro wrote:
> 
> > > >  [ Sending Preferences ]
> > > >       [X]  Do Not Send Flowed Text                                               
> > > >       [ ]  Downgrade Multipart to Text                                           
> > > >       [X]  Enable 8bit ESMTP Negotiation    (default)
> > > >       [ ]  Strip Whitespace Before Sending                                       
> > > >  
> > > > And Documentation/email-clients.txt have:
> > > > 
> > > > Config options:
> > > > - quell-flowed-text is needed for recent versions
> > > > - the "no-strip-whitespace-before-send" option is needed
> > > > 
> > > > Am I the one to blame? Should I uncheck the 'Do Not Send Flowed Text'? I 
> > > > am sorry if it is my fault.
> > > 
> > > Ah, I quoted the pine Config options, the alpine config options from 
> > > Documentation/email-clients.txt should be:
> > > 
> > > Config options:
> > > In the "Sending Preferences" section:
> > > 
> > > - "Do Not Send Flowed Text" must be enabled
> > > - "Strip Whitespace Before Sending" must be disabled
> > 
> > Can you please make email-clients.txt fixing patch too? :-)
> 
> Sorry my poor written English make you confused.:-). The two config 
> options for alpine are already in email-clients.txt and I followed the existing config options 
> recommendation. I am not sure if my alpine is the faulty email client. Is 
> there still something missing with alpine? 

It seems it was Minchan's mail
<28c262360909031837j4e1a9214if6070d02cb4fde04@mail.gmail.com> that
replaced ascii spacing with some utf8 spacing characters.

It is arguable whether this conversion was sensible but a bit sad
that, apparently, by mid 2009 still not every email client is able to
cope. :/

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Minchan Kim]

On Wed, Sep 9, 2009 at 9:04 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> On Tue, Sep 08, 2009 at 11:32:45AM -0700, Vincent Li wrote:
>> On Mon, 7 Sep 2009, KOSAKI Motohiro wrote:
>>
>> > > >  [ Sending Preferences ]
>> > > >       [X]  Do Not Send Flowed Text
>> > > >       [ ]  Downgrade Multipart to Text
>> > > >       [X]  Enable 8bit ESMTP Negotiation    (default)
>> > > >       [ ]  Strip Whitespace Before Sending
>> > > >
>> > > > And Documentation/email-clients.txt have:
>> > > >
>> > > > Config options:
>> > > > - quell-flowed-text is needed for recent versions
>> > > > - the "no-strip-whitespace-before-send" option is needed
>> > > >
>> > > > Am I the one to blame? Should I uncheck the 'Do Not Send Flowed Text'? I
>> > > > am sorry if it is my fault.
>> > >
>> > > Ah, I quoted the pine Config options, the alpine config options from
>> > > Documentation/email-clients.txt should be:
>> > >
>> > > Config options:
>> > > In the "Sending Preferences" section:
>> > >
>> > > - "Do Not Send Flowed Text" must be enabled
>> > > - "Strip Whitespace Before Sending" must be disabled
>> >
>> > Can you please make email-clients.txt fixing patch too? :-)
>>
>> Sorry my poor written English make you confused.:-). The two config
>> options for alpine are already in email-clients.txt and I followed the existing config options
>> recommendation. I am not sure if my alpine is the faulty email client. Is
>> there still something missing with alpine?
>
> It seems it was Minchan's mail
> <28c262360909031837j4e1a9214if6070d02cb4fde04@mail.gmail.com> that
> replaced ascii spacing with some utf8 spacing characters.

Sigh.

I often use two mail client.
One of them is goolge web mail.
I guess that's mail was sent by google web mail which
changed ascii to utf8.
But I am not sure that's because I have been used it by ascii option.
Anyway, Sorry for noising.

Now I use google web mail and I hope this mail is going to send
properly. ;-(


> It is arguable whether this conversion was sensible but a bit sad
> that, apparently, by mid 2009 still not every email client is able to
> cope. :/
>



-- 
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Andrew Morton]

On Wed,  2 Sep 2009 16:49:25 -0700
Vincent Li <macli@brc.ubc.ca> wrote:

> If we can't isolate pages from LRU list, we don't have to account page movement, either.
> Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> 
> This patch removes unnecessary overhead of page accounting
> and locking in shrink_active_list as follow-up work of commit 5343daceec.
> 

I didn't merge this.  It's still unclear to me that the benefit of the
patch exceeds the (small) maintainability cost.

Did we end up getting any usable data on the frequency of `nr_taken == 0'?




From: Vincent Li <macli@brc.ubc.ca>

If we can't isolate pages from LRU list, we don't have to account page
movement, either.  Already, in commit 5343daceec, KOSAKI did it about
shrink_inactive_list.

This patch removes unnecessary overhead of page accounting and locking in
shrink_active_list as follow-up work of commit 5343daceec.

Signed-off-by: Vincent Li <macli@brc.ubc.ca>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
Acked-by: Rik van Riel <riel@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/vmscan.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff -puN mm/vmscan.c~mm-vsmcan-check-shrink_active_list-sc-isolate_pages-return-value mm/vmscan.c
--- a/mm/vmscan.c~mm-vsmcan-check-shrink_active_list-sc-isolate_pages-return-value
+++ a/mm/vmscan.c
@@ -1323,9 +1323,12 @@ static void shrink_active_list(unsigned 
 	if (scanning_global_lru(sc)) {
 		zone->pages_scanned += pgscanned;
 	}
-	reclaim_stat->recent_scanned[file] += nr_taken;
-
 	__count_zone_vm_events(PGREFILL, zone, pgscanned);
+
+	if (nr_taken == 0)
+		goto done;
+
+	reclaim_stat->recent_scanned[file] += nr_taken;
 	if (file)
 		__mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
 	else
@@ -1383,6 +1386,7 @@ static void shrink_active_list(unsigned 
 	move_active_pages_to_lru(zone, &l_inactive,
 						LRU_BASE   + file * LRU_FILE);
 	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
+done:
 	spin_unlock_irq(&zone->lru_lock);
 }
 
_

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

Re: [RESEND][PATCH V1] mm/vsmcan: check shrink_active_list() sc->isolate_pages() return value.

[Vincent Li]

On Tue, 22 Sep 2009, Andrew Morton wrote:

> On Wed,  2 Sep 2009 16:49:25 -0700
> Vincent Li <macli@brc.ubc.ca> wrote:
> 
> > If we can't isolate pages from LRU list, we don't have to account page movement, either.
> > Already, in commit 5343daceec, KOSAKI did it about shrink_inactive_list.
> > 
> > This patch removes unnecessary overhead of page accounting
> > and locking in shrink_active_list as follow-up work of commit 5343daceec.
> > 
> 
> I didn't merge this.  It's still unclear to me that the benefit of the
> patch exceeds the (small) maintainability cost.
> 
> Did we end up getting any usable data on the frequency of `nr_taken == 0'?

Sorry not to able to follow up on this in timely manner (busy at $work 
and family event), I will try to follow Mel's event tracing suggestions to 
get some frequency data of 'nr_taken == 0' in the next few weeks. I 
suspect 'nr_taken == 0' would be very few in normal situation.

> 
> 
> 
> 
> From: Vincent Li <macli@brc.ubc.ca>
> 
> If we can't isolate pages from LRU list, we don't have to account page
> movement, either.  Already, in commit 5343daceec, KOSAKI did it about
> shrink_inactive_list.
> 
> This patch removes unnecessary overhead of page accounting and locking in
> shrink_active_list as follow-up work of commit 5343daceec.
> 
> Signed-off-by: Vincent Li <macli@brc.ubc.ca>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
> Acked-by: Rik van Riel <riel@redhat.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/vmscan.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff -puN mm/vmscan.c~mm-vsmcan-check-shrink_active_list-sc-isolate_pages-return-value mm/vmscan.c
> --- a/mm/vmscan.c~mm-vsmcan-check-shrink_active_list-sc-isolate_pages-return-value
> +++ a/mm/vmscan.c
> @@ -1323,9 +1323,12 @@ static void shrink_active_list(unsigned 
>  	if (scanning_global_lru(sc)) {
>  		zone->pages_scanned += pgscanned;
>  	}
> -	reclaim_stat->recent_scanned[file] += nr_taken;
> -
>  	__count_zone_vm_events(PGREFILL, zone, pgscanned);
> +
> +	if (nr_taken == 0)
> +		goto done;
> +
> +	reclaim_stat->recent_scanned[file] += nr_taken;
>  	if (file)
>  		__mod_zone_page_state(zone, NR_ACTIVE_FILE, -nr_taken);
>  	else
> @@ -1383,6 +1386,7 @@ static void shrink_active_list(unsigned 
>  	move_active_pages_to_lru(zone, &l_inactive,
>  						LRU_BASE   + file * LRU_FILE);
>  	__mod_zone_page_state(zone, NR_ISOLATED_ANON + file, -nr_taken);
> +done:
>  	spin_unlock_irq(&zone->lru_lock);
>  }
>  
> _
> 
> 
> 

Vincent Li
Biomedical Research Center
University of British Columbia

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