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@xxxxxxxxxxxxx>
> Acked-by: Mel Gorman <mgorman@xxxxxxx>
> ---
> 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@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </a>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </a>