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