[PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Huang Shijie]

In the real tests, there are maybe many times the cc->nr_migratepages is zero,
but isolate_migratepages() returns ISOLATE_SUCCESS.

Memory in our mx6q board:
	2G memory, 8192 pages per page block

We use the following command to test in two types system loads:
	#echo 1 > /proc/sys/vm/compact_memory

Test Result:
	[1] little load(login in the ubuntu):
		all the scanned pageblocks	: 79
		pageblocks which get no pages	: 46

		The ratio of `get no pages` pageblock is 58.2%.

	[2] heavy load(start thunderbird, firefox, ..etc):
		all the scanned pageblocks	: 89
		pageblocks which get no pages	: 36

		The ratio of `get no pages` pageblock is 40.4%.

In order to get better performance, we should check the number of the
really isolated pages. And do the optimazition for this case.

Also fix the confused comments(from Mel Gorman).

Tested this patch in MX6Q board.

Signed-off-by: Huang Shijie <b32955@freescale.com>
Acked-by: Mel Gorman <mgorman@suse.de>
---
 mm/compaction.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index f4f514d..41d1b72a 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -246,8 +246,8 @@ static bool too_many_isolated(struct zone *zone)
 /* possible outcome of isolate_migratepages */
 typedef enum {
 	ISOLATE_ABORT,		/* Abort compaction now */
-	ISOLATE_NONE,		/* No pages isolated, continue scanning */
-	ISOLATE_SUCCESS,	/* Pages isolated, migrate */
+	ISOLATE_NONE,		/* No pages scanned, consider next pageblock*/
+	ISOLATE_SUCCESS,	/* Pages scanned and maybe isolated, migrate */
 } isolate_migrate_t;
 
 /*
@@ -542,7 +542,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
 
 	while ((ret = compact_finished(zone, cc)) == COMPACT_CONTINUE) {
 		unsigned long nr_migrate, nr_remaining;
-		int err;
+		int err = 0;
 
 		switch (isolate_migratepages(zone, cc)) {
 		case ISOLATE_ABORT:
@@ -554,17 +554,21 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
 			;
 		}
 
-		nr_migrate = cc->nr_migratepages;
-		err = migrate_pages(&cc->migratepages, compaction_alloc,
-				(unsigned long)cc, false,
-				cc->sync);
-		update_nr_listpages(cc);
-		nr_remaining = cc->nr_migratepages;
+		nr_migrate = nr_remaining = cc->nr_migratepages;
+		if (nr_migrate) {
+			err = migrate_pages(&cc->migratepages, compaction_alloc,
+					(unsigned long)cc, false,
+					cc->sync);
+			update_nr_listpages(cc);
+			nr_remaining = cc->nr_migratepages;
+			count_vm_events(COMPACTPAGES,
+					nr_migrate - nr_remaining);
+			if (nr_remaining)
+				count_vm_events(COMPACTPAGEFAILED,
+						nr_remaining);
+		}
 
 		count_vm_event(COMPACTBLOCKS);
-		count_vm_events(COMPACTPAGES, nr_migrate - nr_remaining);
-		if (nr_remaining)
-			count_vm_events(COMPACTPAGEFAILED, nr_remaining);
 		trace_mm_compaction_migratepages(nr_migrate - nr_remaining,
 						nr_remaining);
 
-- 
1.7.3.2


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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Minchan Kim]

On Thu, Jan 12, 2012 at 01:47:02PM +0800, Huang Shijie wrote:
> In the real tests, there are maybe many times the cc->nr_migratepages is zero,
> but isolate_migratepages() returns ISOLATE_SUCCESS.
> 
> Memory in our mx6q board:
> 	2G memory, 8192 pages per page block
> 
> We use the following command to test in two types system loads:
> 	#echo 1 > /proc/sys/vm/compact_memory
> 
> Test Result:
> 	[1] little load(login in the ubuntu):
> 		all the scanned pageblocks	: 79
> 		pageblocks which get no pages	: 46
> 
> 		The ratio of `get no pages` pageblock is 58.2%.
> 
> 	[2] heavy load(start thunderbird, firefox, ..etc):
> 		all the scanned pageblocks	: 89
> 		pageblocks which get no pages	: 36
> 
> 		The ratio of `get no pages` pageblock is 40.4%.
> 
> In order to get better performance, we should check the number of the
> really isolated pages. And do the optimazition for this case.
> 
> Also fix the confused comments(from Mel Gorman).
> 
> Tested this patch in MX6Q board.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> Acked-by: Mel Gorman <mgorman@suse.de>
> ---
>  mm/compaction.c |   28 ++++++++++++++++------------
>  1 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index f4f514d..41d1b72a 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -246,8 +246,8 @@ static bool too_many_isolated(struct zone *zone)
>  /* possible outcome of isolate_migratepages */
>  typedef enum {
>  	ISOLATE_ABORT,		/* Abort compaction now */
> -	ISOLATE_NONE,		/* No pages isolated, continue scanning */
> -	ISOLATE_SUCCESS,	/* Pages isolated, migrate */
> +	ISOLATE_NONE,		/* No pages scanned, consider next pageblock*/
> +	ISOLATE_SUCCESS,	/* Pages scanned and maybe isolated, migrate */
>  } isolate_migrate_t;
>  

Hmm, I don't like this change.
ISOLATE_NONE mean "we don't isolate any page at all"
ISOLATE_SUCCESS mean "We isolaetssome pages"
It's very clear but you are changing semantic slighly.

How about this?

--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -376,7 +376,7 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
 
        trace_mm_compaction_isolate_migratepages(nr_scanned, nr_isolated);
 
-       return ISOLATE_SUCCESS;
+       return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;
 }
 
 /*
@@ -542,6 +542,8 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
                unsigned long nr_migrate, nr_remaining;
                int err;
 
+               count_vm_event(COMPACTBLOCKS);
+
                switch (isolate_migratepages(zone, cc)) {
                case ISOLATE_ABORT:
                        ret = COMPACT_PARTIAL;
@@ -559,7 +561,6 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
                update_nr_listpages(cc);
                nr_remaining = cc->nr_migratepages;
 
-               count_vm_event(COMPACTBLOCKS);
                count_vm_events(COMPACTPAGES, nr_migrate - nr_remaining);
                if (nr_remaining)
                        count_vm_events(COMPACTPAGEFAILED, nr_remaining);

This patch's side effect is that it accounts COMPACTBLOCK although isolation is cancel by signal
but I think it's very rare and doesn't give big effect for statistics of compaciton.


>  /*
> @@ -542,7 +542,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
>  
>  	while ((ret = compact_finished(zone, cc)) == COMPACT_CONTINUE) {
>  		unsigned long nr_migrate, nr_remaining;
> -		int err;
> +		int err = 0;
>  
>  		switch (isolate_migratepages(zone, cc)) {
>  		case ISOLATE_ABORT:
> @@ -554,17 +554,21 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
>  			;
>  		}
>  
> -		nr_migrate = cc->nr_migratepages;
> -		err = migrate_pages(&cc->migratepages, compaction_alloc,
> -				(unsigned long)cc, false,
> -				cc->sync);
> -		update_nr_listpages(cc);
> -		nr_remaining = cc->nr_migratepages;
> +		nr_migrate = nr_remaining = cc->nr_migratepages;
> +		if (nr_migrate) {
> +			err = migrate_pages(&cc->migratepages, compaction_alloc,
> +					(unsigned long)cc, false,
> +					cc->sync);
> +			update_nr_listpages(cc);
> +			nr_remaining = cc->nr_migratepages;
> +			count_vm_events(COMPACTPAGES,
> +					nr_migrate - nr_remaining);
> +			if (nr_remaining)
> +				count_vm_events(COMPACTPAGEFAILED,
> +						nr_remaining);
> +		}
>  
>  		count_vm_event(COMPACTBLOCKS);
> -		count_vm_events(COMPACTPAGES, nr_migrate - nr_remaining);
> -		if (nr_remaining)
> -			count_vm_events(COMPACTPAGEFAILED, nr_remaining);
>  		trace_mm_compaction_migratepages(nr_migrate - nr_remaining,
>  						nr_remaining);
>  
> -- 
> 1.7.3.2
> 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Huang Shijie]

Hi,
> On Thu, Jan 12, 2012 at 01:47:02PM +0800, Huang Shijie wrote:
>> In the real tests, there are maybe many times the cc->nr_migratepages is zero,
>> but isolate_migratepages() returns ISOLATE_SUCCESS.
>>
>> Memory in our mx6q board:
>> 	2G memory, 8192 pages per page block
>>
>> We use the following command to test in two types system loads:
>> 	#echo 1>  /proc/sys/vm/compact_memory
>>
>> Test Result:
>> 	[1] little load(login in the ubuntu):
>> 		all the scanned pageblocks	: 79
>> 		pageblocks which get no pages	: 46
>>
>> 		The ratio of `get no pages` pageblock is 58.2%.
>>
>> 	[2] heavy load(start thunderbird, firefox, ..etc):
>> 		all the scanned pageblocks	: 89
>> 		pageblocks which get no pages	: 36
>>
>> 		The ratio of `get no pages` pageblock is 40.4%.
>>
>> In order to get better performance, we should check the number of the
>> really isolated pages. And do the optimazition for this case.
>>
>> Also fix the confused comments(from Mel Gorman).
>>
>> Tested this patch in MX6Q board.
>>
>> Signed-off-by: Huang Shijie<b32955@freescale.com>
>> Acked-by: Mel Gorman<mgorman@suse.de>
>> ---
>>   mm/compaction.c |   28 ++++++++++++++++------------
>>   1 files changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index f4f514d..41d1b72a 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -246,8 +246,8 @@ static bool too_many_isolated(struct zone *zone)
>>   /* possible outcome of isolate_migratepages */
>>   typedef enum {
>>   	ISOLATE_ABORT,		/* Abort compaction now */
>> -	ISOLATE_NONE,		/* No pages isolated, continue scanning */
>> -	ISOLATE_SUCCESS,	/* Pages isolated, migrate */
>> +	ISOLATE_NONE,		/* No pages scanned, consider next pageblock*/
>> +	ISOLATE_SUCCESS,	/* Pages scanned and maybe isolated, migrate */
>>   } isolate_migrate_t;
>>
> Hmm, I don't like this change.
> ISOLATE_NONE mean "we don't isolate any page at all"
> ISOLATE_SUCCESS mean "We isolaetssome pages"
> It's very clear but you are changing semantic slighly.
I think Mel Gorman's new explain is more proper.
> How about this?
>
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -376,7 +376,7 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
>
>          trace_mm_compaction_isolate_migratepages(nr_scanned, nr_isolated);
>
> -       return ISOLATE_SUCCESS;
> +       return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;
>   }
>
>   /*
> @@ -542,6 +542,8 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
>                  unsigned long nr_migrate, nr_remaining;
>                  int err;
>
> +               count_vm_event(COMPACTBLOCKS);
not right.
the isolate_migratepage may returns ISOLATE_NONE. We should not account 
this case.

Best Regards
Huang Shijie

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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Minchan Kim]

On Thu, Jan 12, 2012 at 04:26:04PM +0800, Huang Shijie wrote:
> Hi,
> >On Thu, Jan 12, 2012 at 01:47:02PM +0800, Huang Shijie wrote:
> >>In the real tests, there are maybe many times the cc->nr_migratepages is zero,
> >>but isolate_migratepages() returns ISOLATE_SUCCESS.
> >>
> >>Memory in our mx6q board:
> >>	2G memory, 8192 pages per page block
> >>
> >>We use the following command to test in two types system loads:
> >>	#echo 1>  /proc/sys/vm/compact_memory
> >>
> >>Test Result:
> >>	[1] little load(login in the ubuntu):
> >>		all the scanned pageblocks	: 79
> >>		pageblocks which get no pages	: 46
> >>
> >>		The ratio of `get no pages` pageblock is 58.2%.
> >>
> >>	[2] heavy load(start thunderbird, firefox, ..etc):
> >>		all the scanned pageblocks	: 89
> >>		pageblocks which get no pages	: 36
> >>
> >>		The ratio of `get no pages` pageblock is 40.4%.
> >>
> >>In order to get better performance, we should check the number of the
> >>really isolated pages. And do the optimazition for this case.
> >>
> >>Also fix the confused comments(from Mel Gorman).
> >>
> >>Tested this patch in MX6Q board.
> >>
> >>Signed-off-by: Huang Shijie<b32955@freescale.com>
> >>Acked-by: Mel Gorman<mgorman@suse.de>
> >>---
> >>  mm/compaction.c |   28 ++++++++++++++++------------
> >>  1 files changed, 16 insertions(+), 12 deletions(-)
> >>
> >>diff --git a/mm/compaction.c b/mm/compaction.c
> >>index f4f514d..41d1b72a 100644
> >>--- a/mm/compaction.c
> >>+++ b/mm/compaction.c
> >>@@ -246,8 +246,8 @@ static bool too_many_isolated(struct zone *zone)
> >>  /* possible outcome of isolate_migratepages */
> >>  typedef enum {
> >>  	ISOLATE_ABORT,		/* Abort compaction now */
> >>-	ISOLATE_NONE,		/* No pages isolated, continue scanning */
> >>-	ISOLATE_SUCCESS,	/* Pages isolated, migrate */
> >>+	ISOLATE_NONE,		/* No pages scanned, consider next pageblock*/
> >>+	ISOLATE_SUCCESS,	/* Pages scanned and maybe isolated, migrate */
> >>  } isolate_migrate_t;
> >>
> >Hmm, I don't like this change.
> >ISOLATE_NONE mean "we don't isolate any page at all"
> >ISOLATE_SUCCESS mean "We isolaetssome pages"
> >It's very clear but you are changing semantic slighly.
> I think Mel Gorman's new explain is more proper.

I didn't read it but I think current shape is good to me and
I don't think why we need such change.

> >How about this?
> >
> >--- a/mm/compaction.c
> >+++ b/mm/compaction.c
> >@@ -376,7 +376,7 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
> >
> >         trace_mm_compaction_isolate_migratepages(nr_scanned, nr_isolated);
> >
> >-       return ISOLATE_SUCCESS;
> >+       return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;
> >  }
> >
> >  /*
> >@@ -542,6 +542,8 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
> >                 unsigned long nr_migrate, nr_remaining;
> >                 int err;
> >
> >+               count_vm_event(COMPACTBLOCKS);
> not right.
> the isolate_migratepage may returns ISOLATE_NONE. We should not
> account this case.

It depends on how we handle COMPACTBLOCKS.
I think COMPACTBLOCK mean "trial" of compaction so although we can't isolate any page at all, we have to
accout it with "trial of compaction".
And in your patch, although nr_migrate is zero, you account it, too.
And we have been accounted it until now.

> 
> Best Regards
> Huang Shijie
> 

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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Huang Shijie]

Hi,
> It depends on how we handle COMPACTBLOCKS.
> I think COMPACTBLOCK mean "trial" of compaction so although we can't isolate any page at all, we have to
> accout it with "trial of compaction".
> And in your patch, although nr_migrate is zero, you account it, too.
> And we have been accounted it until now.
ok, Wait for Mel Gorman for the FINAL explanation.

BR
Huang Shijie



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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Mel Gorman]

On Thu, Jan 12, 2012 at 05:03:11PM +0900, Minchan Kim wrote:
> On Thu, Jan 12, 2012 at 01:47:02PM +0800, Huang Shijie wrote:
> > In the real tests, there are maybe many times the cc->nr_migratepages is zero,
> > but isolate_migratepages() returns ISOLATE_SUCCESS.
> > 
> > Memory in our mx6q board:
> > 	2G memory, 8192 pages per page block
> > 
> > We use the following command to test in two types system loads:
> > 	#echo 1 > /proc/sys/vm/compact_memory
> > 
> > Test Result:
> > 	[1] little load(login in the ubuntu):
> > 		all the scanned pageblocks	: 79
> > 		pageblocks which get no pages	: 46
> > 
> > 		The ratio of `get no pages` pageblock is 58.2%.
> > 
> > 	[2] heavy load(start thunderbird, firefox, ..etc):
> > 		all the scanned pageblocks	: 89
> > 		pageblocks which get no pages	: 36
> > 
> > 		The ratio of `get no pages` pageblock is 40.4%.
> > 
> > In order to get better performance, we should check the number of the
> > really isolated pages. And do the optimazition for this case.
> > 
> > Also fix the confused comments(from Mel Gorman).
> > 
> > Tested this patch in MX6Q board.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > Acked-by: Mel Gorman <mgorman@suse.de>
> > ---
> >  mm/compaction.c |   28 ++++++++++++++++------------
> >  1 files changed, 16 insertions(+), 12 deletions(-)
> > 
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index f4f514d..41d1b72a 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -246,8 +246,8 @@ static bool too_many_isolated(struct zone *zone)
> >  /* possible outcome of isolate_migratepages */
> >  typedef enum {
> >  	ISOLATE_ABORT,		/* Abort compaction now */
> > -	ISOLATE_NONE,		/* No pages isolated, continue scanning */
> > -	ISOLATE_SUCCESS,	/* Pages isolated, migrate */
> > +	ISOLATE_NONE,		/* No pages scanned, consider next pageblock*/
> > +	ISOLATE_SUCCESS,	/* Pages scanned and maybe isolated, migrate */
> >  } isolate_migrate_t;
> >  
> 
> Hmm, I don't like this change.
> ISOLATE_NONE mean "we don't isolate any page at all"
> ISOLATE_SUCCESS mean "We isolaetssome pages"
> It's very clear but you are changing semantic slighly.
> 

That is somewhat the point of his patch - isolate_migratepages()
can return ISOLATE_SUCCESS even though no pages were isolated. Note that
he does not change when ISOLATE_NONE or ISOLATE_SUCCESS gets returned,
he updates the comment to match what the code is actually doing. This
should be visible from the tracepoint. My machine has been up for days
and loaded when I started a process that mapped a large anonymous
region. THP would kick in and I see from the tracepoints excerpts like
this

          malloc-13964 [007] 221636.457022: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
          malloc-13964 [007] 221636.457022: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
          malloc-13964 [007] 221636.457023: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
          malloc-13964 [007] 221636.457023: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
          malloc-13964 [007] 221636.457024: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
          malloc-13964 [007] 221636.457025: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
          malloc-13964 [007] 221636.457025: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
          malloc-13964 [007] 221636.457049: mm_compaction_isolate_migratepages: nr_scanned=512 nr_taken=16
          malloc-13964 [007] 221636.457102: mm_compaction_isolate_migratepages: nr_scanned=512 nr_taken=16
          malloc-13964 [007] 221636.457143: mm_compaction_isolate_migratepages: nr_scanned=512 nr_taken=17
          malloc-13964 [007] 221636.457189: mm_compaction_isolate_migratepages: nr_scanned=433 nr_taken=32
          malloc-13964 [007] 221636.457253: mm_compaction_isolate_migratepages: nr_scanned=205 nr_taken=32
          malloc-13964 [007] 221636.457319: mm_compaction_isolate_migratepages: nr_scanned=389 nr_taken=7

These "nr_scanned=1 nr_taken=0" are during async compaction where the
scanner is skipping over pageblocks that are not MIGRATE_MOVABLE. As the
function only deals in pageblocks, it means the function returns after
only scanning 1 page expecting that compact_zone() will move to the next
block.

> How about this?
> 
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -376,7 +376,7 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
>  
>         trace_mm_compaction_isolate_migratepages(nr_scanned, nr_isolated);
>  
> -       return ISOLATE_SUCCESS;
> +       return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;
>  }
>  
>  /*
> @@ -542,6 +542,8 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
>                 unsigned long nr_migrate, nr_remaining;
>                 int err;
>  
> +               count_vm_event(COMPACTBLOCKS);
> +
>                 switch (isolate_migratepages(zone, cc)) {
>                 case ISOLATE_ABORT:
>                         ret = COMPACT_PARTIAL;
> @@ -559,7 +561,6 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
>                 update_nr_listpages(cc);
>                 nr_remaining = cc->nr_migratepages;
>  
> -               count_vm_event(COMPACTBLOCKS);
>                 count_vm_events(COMPACTPAGES, nr_migrate - nr_remaining);
>                 if (nr_remaining)
>                         count_vm_events(COMPACTPAGEFAILED, nr_remaining);
> 
> This patch's side effect is that it accounts COMPACTBLOCK although isolation is cancel by signal
> but I think it's very rare and doesn't give big effect for statistics of compaciton.
> 

This came up during discussion the last time. My opinion was that
COMPACTBLOCK not being updated was a problem. In the existing code
ISOLATE_NONE returning also means the scan did not take place and
this does not need to be accounted for. However, if we scan the block
and isolate no pages, we still want to account for that. A rapidly
increasing COMPACTBLOCKS while COMPACTPAGES changes very little could
indicate that compaction is doing a lot of busy work without making
any useful progress for example.

It could easily be argued that if we skip over !MIGRATE_MOVABLE
pageblocks then we should not account for that in COMPACTBLOCKS either
because the scanning was minimal. In that case we would change this

                /*
                 * For async migration, also only scan in MOVABLE blocks. Async
                 * migration is optimistic to see if the minimum amount of work
                 * satisfies the allocation
                 */
                pageblock_nr = low_pfn >> pageblock_order;
                if (!cc->sync && last_pageblock_nr != pageblock_nr &&
                                get_pageblock_migratetype(page) != MIGRATE_MOVABLE) {
                        low_pfn += pageblock_nr_pages;
                        low_pfn = ALIGN(low_pfn, pageblock_nr_pages) - 1;
                        last_pageblock_nr = pageblock_nr;
                        continue;
                }

to return ISOLATE_NONE there instead of continue. I would be ok making
that part of this patch to clarify the difference between ISOLATE_NONE
and ISOLATE_SUCCESS and what it means for accounting.

> 
> >  /*
> > @@ -542,7 +542,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
> >  
> >  	while ((ret = compact_finished(zone, cc)) == COMPACT_CONTINUE) {
> >  		unsigned long nr_migrate, nr_remaining;
> > -		int err;
> > +		int err = 0;
> >  
> >  		switch (isolate_migratepages(zone, cc)) {
> >  		case ISOLATE_ABORT:
> > @@ -554,17 +554,21 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
> >  			;
> >  		}
> >  
> > -		nr_migrate = cc->nr_migratepages;
> > -		err = migrate_pages(&cc->migratepages, compaction_alloc,
> > -				(unsigned long)cc, false,
> > -				cc->sync);
> > -		update_nr_listpages(cc);
> > -		nr_remaining = cc->nr_migratepages;
> > +		nr_migrate = nr_remaining = cc->nr_migratepages;
> > +		if (nr_migrate) {
> > +			err = migrate_pages(&cc->migratepages, compaction_alloc,
> > +					(unsigned long)cc, false,
> > +					cc->sync);
> > +			update_nr_listpages(cc);
> > +			nr_remaining = cc->nr_migratepages;
> > +			count_vm_events(COMPACTPAGES,
> > +					nr_migrate - nr_remaining);
> > +			if (nr_remaining)
> > +				count_vm_events(COMPACTPAGEFAILED,
> > +						nr_remaining);
> > +		}
> >  
> >  		count_vm_event(COMPACTBLOCKS);
> > -		count_vm_events(COMPACTPAGES, nr_migrate - nr_remaining);
> > -		if (nr_remaining)
> > -			count_vm_events(COMPACTPAGEFAILED, nr_remaining);
> >  		trace_mm_compaction_migratepages(nr_migrate - nr_remaining,
> >  						nr_remaining);
> >  

-- 
Mel Gorman
SUSE Labs

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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Minchan Kim]

On Thu, Jan 12, 2012 at 11:48:35AM +0000, Mel Gorman wrote:
> On Thu, Jan 12, 2012 at 05:03:11PM +0900, Minchan Kim wrote:
> > On Thu, Jan 12, 2012 at 01:47:02PM +0800, Huang Shijie wrote:
> > > In the real tests, there are maybe many times the cc->nr_migratepages is zero,
> > > but isolate_migratepages() returns ISOLATE_SUCCESS.
> > > 
> > > Memory in our mx6q board:
> > > 	2G memory, 8192 pages per page block
> > > 
> > > We use the following command to test in two types system loads:
> > > 	#echo 1 > /proc/sys/vm/compact_memory
> > > 
> > > Test Result:
> > > 	[1] little load(login in the ubuntu):
> > > 		all the scanned pageblocks	: 79
> > > 		pageblocks which get no pages	: 46
> > > 
> > > 		The ratio of `get no pages` pageblock is 58.2%.
> > > 
> > > 	[2] heavy load(start thunderbird, firefox, ..etc):
> > > 		all the scanned pageblocks	: 89
> > > 		pageblocks which get no pages	: 36
> > > 
> > > 		The ratio of `get no pages` pageblock is 40.4%.
> > > 
> > > In order to get better performance, we should check the number of the
> > > really isolated pages. And do the optimazition for this case.
> > > 
> > > Also fix the confused comments(from Mel Gorman).
> > > 
> > > Tested this patch in MX6Q board.
> > > 
> > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > > Acked-by: Mel Gorman <mgorman@suse.de>
> > > ---
> > >  mm/compaction.c |   28 ++++++++++++++++------------
> > >  1 files changed, 16 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/mm/compaction.c b/mm/compaction.c
> > > index f4f514d..41d1b72a 100644
> > > --- a/mm/compaction.c
> > > +++ b/mm/compaction.c
> > > @@ -246,8 +246,8 @@ static bool too_many_isolated(struct zone *zone)
> > >  /* possible outcome of isolate_migratepages */
> > >  typedef enum {
> > >  	ISOLATE_ABORT,		/* Abort compaction now */
> > > -	ISOLATE_NONE,		/* No pages isolated, continue scanning */
> > > -	ISOLATE_SUCCESS,	/* Pages isolated, migrate */
> > > +	ISOLATE_NONE,		/* No pages scanned, consider next pageblock*/
> > > +	ISOLATE_SUCCESS,	/* Pages scanned and maybe isolated, migrate */
> > >  } isolate_migrate_t;
> > >  
> > 
> > Hmm, I don't like this change.
> > ISOLATE_NONE mean "we don't isolate any page at all"
> > ISOLATE_SUCCESS mean "We isolaetssome pages"
> > It's very clear but you are changing semantic slighly.
> > 
> 
> That is somewhat the point of his patch - isolate_migratepages()
> can return ISOLATE_SUCCESS even though no pages were isolated. Note that

That's what I don't like part.
Why should we return ISOLATE_SUCESS although we didn't isolate any page?
Of course, comment can say that but I want to clear code itself than comment.

> he does not change when ISOLATE_NONE or ISOLATE_SUCCESS gets returned,
> he updates the comment to match what the code is actually doing. This

I think he code is doing needs fix.

> should be visible from the tracepoint. My machine has been up for days
> and loaded when I started a process that mapped a large anonymous
> region. THP would kick in and I see from the tracepoints excerpts like
> this
> 
>           malloc-13964 [007] 221636.457022: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
>           malloc-13964 [007] 221636.457022: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
>           malloc-13964 [007] 221636.457023: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
>           malloc-13964 [007] 221636.457023: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
>           malloc-13964 [007] 221636.457024: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
>           malloc-13964 [007] 221636.457025: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
>           malloc-13964 [007] 221636.457025: mm_compaction_isolate_migratepages: nr_scanned=1 nr_taken=0
>           malloc-13964 [007] 221636.457049: mm_compaction_isolate_migratepages: nr_scanned=512 nr_taken=16
>           malloc-13964 [007] 221636.457102: mm_compaction_isolate_migratepages: nr_scanned=512 nr_taken=16
>           malloc-13964 [007] 221636.457143: mm_compaction_isolate_migratepages: nr_scanned=512 nr_taken=17
>           malloc-13964 [007] 221636.457189: mm_compaction_isolate_migratepages: nr_scanned=433 nr_taken=32
>           malloc-13964 [007] 221636.457253: mm_compaction_isolate_migratepages: nr_scanned=205 nr_taken=32
>           malloc-13964 [007] 221636.457319: mm_compaction_isolate_migratepages: nr_scanned=389 nr_taken=7
> 
> These "nr_scanned=1 nr_taken=0" are during async compaction where the
> scanner is skipping over pageblocks that are not MIGRATE_MOVABLE. As the
> function only deals in pageblocks, it means the function returns after
> only scanning 1 page expecting that compact_zone() will move to the next
> block.
> 
> > How about this?
> > 
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -376,7 +376,7 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
> >  
> >         trace_mm_compaction_isolate_migratepages(nr_scanned, nr_isolated);
> >  
> > -       return ISOLATE_SUCCESS;
> > +       return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;
> >  }
> >  
> >  /*
> > @@ -542,6 +542,8 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
> >                 unsigned long nr_migrate, nr_remaining;
> >                 int err;
> >  
> > +               count_vm_event(COMPACTBLOCKS);
> > +
> >                 switch (isolate_migratepages(zone, cc)) {
> >                 case ISOLATE_ABORT:
> >                         ret = COMPACT_PARTIAL;
> > @@ -559,7 +561,6 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
> >                 update_nr_listpages(cc);
> >                 nr_remaining = cc->nr_migratepages;
> >  
> > -               count_vm_event(COMPACTBLOCKS);
> >                 count_vm_events(COMPACTPAGES, nr_migrate - nr_remaining);
> >                 if (nr_remaining)
> >                         count_vm_events(COMPACTPAGEFAILED, nr_remaining);
> > 
> > This patch's side effect is that it accounts COMPACTBLOCK although isolation is cancel by signal
> > but I think it's very rare and doesn't give big effect for statistics of compaciton.
> > 
> 
> This came up during discussion the last time. My opinion was that
> COMPACTBLOCK not being updated was a problem. In the existing code
> ISOLATE_NONE returning also means the scan did not take place and
> this does not need to be accounted for. However, if we scan the block
> and isolate no pages, we still want to account for that. A rapidly
> increasing COMPACTBLOCKS while COMPACTPAGES changes very little could
> indicate that compaction is doing a lot of busy work without making
> any useful progress for example.

Agree.

> 
> It could easily be argued that if we skip over !MIGRATE_MOVABLE
> pageblocks then we should not account for that in COMPACTBLOCKS either
> because the scanning was minimal. In that case we would change this
> 
>                 /*
>                  * For async migration, also only scan in MOVABLE blocks. Async
>                  * migration is optimistic to see if the minimum amount of work
>                  * satisfies the allocation
>                  */
>                 pageblock_nr = low_pfn >> pageblock_order;
>                 if (!cc->sync && last_pageblock_nr != pageblock_nr &&
>                                 get_pageblock_migratetype(page) != MIGRATE_MOVABLE) {
>                         low_pfn += pageblock_nr_pages;
>                         low_pfn = ALIGN(low_pfn, pageblock_nr_pages) - 1;
>                         last_pageblock_nr = pageblock_nr;
>                         continue;
>                 }
> 
> to return ISOLATE_NONE there instead of continue. I would be ok making
> that part of this patch to clarify the difference between ISOLATE_NONE
> and ISOLATE_SUCCESS and what it means for accounting.

I think simple patch is returning "return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;"
It's very clear and readable, I think.
In this patch, what's the problem you think?

> 
> > 
> > >  /*
> > > @@ -542,7 +542,7 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
> > >  
> > >  	while ((ret = compact_finished(zone, cc)) == COMPACT_CONTINUE) {
> > >  		unsigned long nr_migrate, nr_remaining;
> > > -		int err;
> > > +		int err = 0;
> > >  
> > >  		switch (isolate_migratepages(zone, cc)) {
> > >  		case ISOLATE_ABORT:
> > > @@ -554,17 +554,21 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
> > >  			;
> > >  		}
> > >  
> > > -		nr_migrate = cc->nr_migratepages;
> > > -		err = migrate_pages(&cc->migratepages, compaction_alloc,
> > > -				(unsigned long)cc, false,
> > > -				cc->sync);
> > > -		update_nr_listpages(cc);
> > > -		nr_remaining = cc->nr_migratepages;
> > > +		nr_migrate = nr_remaining = cc->nr_migratepages;
> > > +		if (nr_migrate) {
> > > +			err = migrate_pages(&cc->migratepages, compaction_alloc,
> > > +					(unsigned long)cc, false,
> > > +					cc->sync);
> > > +			update_nr_listpages(cc);
> > > +			nr_remaining = cc->nr_migratepages;
> > > +			count_vm_events(COMPACTPAGES,
> > > +					nr_migrate - nr_remaining);
> > > +			if (nr_remaining)
> > > +				count_vm_events(COMPACTPAGEFAILED,
> > > +						nr_remaining);
> > > +		}
> > >  
> > >  		count_vm_event(COMPACTBLOCKS);
> > > -		count_vm_events(COMPACTPAGES, nr_migrate - nr_remaining);
> > > -		if (nr_remaining)
> > > -			count_vm_events(COMPACTPAGEFAILED, nr_remaining);
> > >  		trace_mm_compaction_migratepages(nr_migrate - nr_remaining,
> > >  						nr_remaining);
> > >  
> 
> -- 
> Mel Gorman
> SUSE Labs

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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Huang Shijie]

Hi,
> I think simple patch is returning "return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;"
> It's very clear and readable, I think.
> In this patch, what's the problem you think?
>
sorry for the wrong thread, please read the following thread:
http://marc.info/?l=linux-mm&m=132532266130861&w=2

Best Regards
Huang Shijie

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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Minchan Kim]

On Fri, Jan 13, 2012 at 10:35:42AM +0800, Huang Shijie wrote:
> Hi,
> >I think simple patch is returning "return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;"
> >It's very clear and readable, I think.
> >In this patch, what's the problem you think?
> >
> sorry for the wrong thread, please read the following thread:
> http://marc.info/?l=linux-mm&m=132532266130861&w=2

Huang, Thanks for notice that thread.
I read and if I understand correctly, the point is that Mel want to see tracepoint
"trace_mm_compaction_migratepages" and account "count_vm_event(COMPACTBLOCKS);"
My patch does accounting COMPACTBLOCKS so it's not a problem.
The problem is my patch doesn't emit trace of "trace_mm_compaction_migratepages".
But doesn't it matter? When we doesn't isolate any page at all, both argument in
trace_mm_compaction_migratepages are always zero. Is it meaningful tracepoint?
Do we really want it?

> 
> Best Regards
> Huang Shijie
> 

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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Huang Shijie]

> On Fri, Jan 13, 2012 at 10:35:42AM +0800, Huang Shijie wrote:
>> Hi,
>>> I think simple patch is returning "return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;"
>>> It's very clear and readable, I think.
>>> In this patch, what's the problem you think?
>>>
>> sorry for the wrong thread, please read the following thread:
>> http://marc.info/?l=linux-mm&m=132532266130861&w=2
> Huang, Thanks for notice that thread.
> I read and if I understand correctly, the point is that Mel want to see tracepoint
> "trace_mm_compaction_migratepages" and account "count_vm_event(COMPACTBLOCKS);"
> My patch does accounting COMPACTBLOCKS so it's not a problem.
Your patch also accounts the COMPACTBLOCKS In the ISOLATE_NONE and 
ISOLATE_ABOART when :
++++++++++++++++++++++++++++++++++++++++++++++++++++++
     /* Do not cross the free scanner or scan within a memory hole */
     if (end_pfn > cc->free_pfn || !pfn_valid(low_pfn)) {
         cc->migrate_pfn = end_pfn;
         return ISOLATE_NONE;
     }

     /*
      * Ensure that there are not too many pages isolated from the LRU
      * list by either parallel reclaimers or compaction. If there are,
      * delay for some time until fewer pages are isolated
      */
     while (unlikely(too_many_isolated(zone))) {
         /* async migration should just abort */
         if (!cc->sync)
             return ISOLATE_ABORT;

         congestion_wait(BLK_RW_ASYNC, HZ/10);

         if (fatal_signal_pending(current))
             return ISOLATE_ABORT;
     }
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

this not make sense.
> The problem is my patch doesn't emit trace of "trace_mm_compaction_migratepages".
> But doesn't it matter? When we doesn't isolate any page at all, both argument in
> trace_mm_compaction_migratepages are always zero. Is it meaningful tracepoint?
> Do we really want it?
>
IMHO, yes.

For it _DOES_  scan one PAGEBLOCK even we can not get any page from this 
pageblock.
it should trace the scan even the parameters are both zero.

Huang Shijie
>> Best Regards
>> Huang Shijie
>>


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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Minchan Kim]

On Fri, Jan 13, 2012 at 11:31:31AM +0800, Huang Shijie wrote:
> 
> >On Fri, Jan 13, 2012 at 10:35:42AM +0800, Huang Shijie wrote:
> >>Hi,
> >>>I think simple patch is returning "return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;"
> >>>It's very clear and readable, I think.
> >>>In this patch, what's the problem you think?
> >>>
> >>sorry for the wrong thread, please read the following thread:
> >>http://marc.info/?l=linux-mm&m=132532266130861&w=2
> >Huang, Thanks for notice that thread.
> >I read and if I understand correctly, the point is that Mel want to see tracepoint
> >"trace_mm_compaction_migratepages" and account "count_vm_event(COMPACTBLOCKS);"
> >My patch does accounting COMPACTBLOCKS so it's not a problem.
> Your patch also accounts the COMPACTBLOCKS In the ISOLATE_NONE and
> ISOLATE_ABOART when :
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>     /* Do not cross the free scanner or scan within a memory hole */
>     if (end_pfn > cc->free_pfn || !pfn_valid(low_pfn)) {
>         cc->migrate_pfn = end_pfn;
>         return ISOLATE_NONE;
>     }
> 
>     /*
>      * Ensure that there are not too many pages isolated from the LRU
>      * list by either parallel reclaimers or compaction. If there are,
>      * delay for some time until fewer pages are isolated
>      */
>     while (unlikely(too_many_isolated(zone))) {
>         /* async migration should just abort */
>         if (!cc->sync)
>             return ISOLATE_ABORT;
> 
>         congestion_wait(BLK_RW_ASYNC, HZ/10);
> 
>         if (fatal_signal_pending(current))
>             return ISOLATE_ABORT;
>     }
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> this not make sense.
> >The problem is my patch doesn't emit trace of "trace_mm_compaction_migratepages".
> >But doesn't it matter? When we doesn't isolate any page at all, both argument in
> >trace_mm_compaction_migratepages are always zero. Is it meaningful tracepoint?
> >Do we really want it?
> >
> IMHO, yes.
> 
> For it _DOES_  scan one PAGEBLOCK even we can not get any page from
> this pageblock.
> it should trace the scan even the parameters are both zero.

Okay. If you want it really, How about this?
Why I insist on is I don't want to change ISOLATE_NONE's semantic.
It's very clear and readable.
We should change code itself instead of semantic of ISOLATE_NONE.

--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -376,7 +376,7 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
 
        trace_mm_compaction_isolate_migratepages(nr_scanned, nr_isolated);
 
-       return ISOLATE_SUCCESS;
+       return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;
 }
 
 /*
@@ -547,6 +547,12 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
                        ret = COMPACT_PARTIAL;
                        goto out;
                case ISOLATE_NONE:
+                       /*
+                        * If we can't isolate pages at all, we want to
+                        * trace, still.
+                        */
+                       count_vm_event(COMPACTBLOCKS);
+                       trace_mm_compaction_migratepages(0, 0);
                        continue;
                case ISOLATE_SUCCESS:
                        ;



> 
> Huang Shijie
> >>Best Regards
> >>Huang Shijie
> >>
> 
> 

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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Mel Gorman]

On Fri, Jan 13, 2012 at 09:50:42AM +0900, Minchan Kim wrote:
> > > Hmm, I don't like this change.
> > > ISOLATE_NONE mean "we don't isolate any page at all"
> > > ISOLATE_SUCCESS mean "We isolaetssome pages"
> > > It's very clear but you are changing semantic slighly.
> > > 
> > 
> > That is somewhat the point of his patch - isolate_migratepages()
> > can return ISOLATE_SUCCESS even though no pages were isolated. Note that
> 
> That's what I don't like part.
> Why should we return ISOLATE_SUCESS although we didn't isolate any page?

Because the scan took place. ISOLATE_NONE is returned when no scanning
took place. ISOLATE_SUCCESS is returned when some scanning took place.
BEcause of async compaction, the scan might only be 1 page but
it's still a scan. It's easy to distinguish using the tracepoint
if necessary.

> Of course, comment can say that but I want to clear code itself than comment.
> 

Yes.

> > <SNIP>
> > 
> > It could easily be argued that if we skip over !MIGRATE_MOVABLE
> > pageblocks then we should not account for that in COMPACTBLOCKS either
> > because the scanning was minimal. In that case we would change this
> > 
> >                 /*
> >                  * For async migration, also only scan in MOVABLE blocks. Async
> >                  * migration is optimistic to see if the minimum amount of work
> >                  * satisfies the allocation
> >                  */
> >                 pageblock_nr = low_pfn >> pageblock_order;
> >                 if (!cc->sync && last_pageblock_nr != pageblock_nr &&
> >                                 get_pageblock_migratetype(page) != MIGRATE_MOVABLE) {
> >                         low_pfn += pageblock_nr_pages;
> >                         low_pfn = ALIGN(low_pfn, pageblock_nr_pages) - 1;
> >                         last_pageblock_nr = pageblock_nr;
> >                         continue;
> >                 }
> > 
> > to return ISOLATE_NONE there instead of continue. I would be ok making
> > that part of this patch to clarify the difference between ISOLATE_NONE
> > and ISOLATE_SUCCESS and what it means for accounting.
> 
> I think simple patch is returning "return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;"
> It's very clear and readable, I think.
> In this patch, what's the problem you think?
> 

The trace point and accounting is missed and that information is useful. 

-- 
Mel Gorman
SUSE Labs

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

Re: [PATCH v2] mm/compaction : do optimazition when the migration scanner gets no page

[Mel Gorman]

On Fri, Jan 13, 2012 at 12:50:37PM +0900, Minchan Kim wrote:
> 
> Okay. If you want it really, How about this?
> Why I insist on is I don't want to change ISOLATE_NONE's semantic.
> It's very clear and readable.
> We should change code itself instead of semantic of ISOLATE_NONE.
> 
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -376,7 +376,7 @@ static isolate_migrate_t isolate_migratepages(struct zone *zone,
>  
>         trace_mm_compaction_isolate_migratepages(nr_scanned, nr_isolated);
>  
> -       return ISOLATE_SUCCESS;
> +       return cc->nr_migratepages ? ISOLATE_SUCCESS : ISOLATE_NONE;
>  }
>  
>  /*
> @@ -547,6 +547,12 @@ static int compact_zone(struct zone *zone, struct compact_control *cc)
>                         ret = COMPACT_PARTIAL;
>                         goto out;
>                 case ISOLATE_NONE:
> +                       /*
> +                        * If we can't isolate pages at all, we want to
> +                        * trace, still.
> +                        */
> +                       count_vm_event(COMPACTBLOCKS);
> +                       trace_mm_compaction_migratepages(0, 0);
>                         continue;
>                 case ISOLATE_SUCCESS:
>                         ;
> 

This will increment COMPACTBLOCKS and trigger the tracepoint even
when no scanning took place. It only happens with the migration and free
scanner meet so once per full compaction cycle which should be a rare
case. That should be fine.

-- 
Mel Gorman
SUSE Labs

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