[PATCH v2] mm/compaction : fix the wrong return value for isolate_migratepages()

[PATCH v2] mm/compaction : fix the wrong return value for isolate_migratepages()

[Huang Shijie]

When we do not get any migrate page, we should return ISOLATE_NONE.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 mm/compaction.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 0f12cc9..3db8630 100644
--- 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 == 0) ? ISOLATE_NONE : ISOLATE_SUCCESS;
 }
 
 /*
-- 
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 : fix the wrong return value for isolate_migratepages()

[Mel Gorman]

On Sat, Dec 31, 2011 at 05:09:45PM +0800, Huang Shijie wrote:
> When we do not get any migrate page, we should return ISOLATE_NONE.
> 

Why?

Returning ISOLATE_SUCCESS means that we fall through. This means busy
work in migrate_pages(), updating list accounting and the list. It's
wasteful but is it functionally incorrect? What problem did you observe?

If this is simply a performance issue then minimally COMPACTBLOCKS
still needs to be updated, we still want to see the tracepoint etc. To
preserve that, I would suggest as an alternative to leave it returning
ISOLATE_SUCCESS but move


               err = migrate_pages(&cc->migratepages, compaction_alloc,
                                (unsigned long)cc, false,
                                cc->sync ? MIGRATE_SYNC_LIGHT : MIGRATE_ASYNC);
                update_nr_listpages(cc);

inside a if (nr_migrate) check to avoid some overhead.

-- 
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 : fix the wrong return value for isolate_migratepages()

[Huang Shijie]

hi,
> Why?
>
> Returning ISOLATE_SUCCESS means that we fall through. This means busy
> work in migrate_pages(), updating list accounting and the list. It's
> wasteful but is it functionally incorrect? What problem did you observe?
there may are many times the cc->migratepages is zero, but the return 
value is ISOLATE_SUCCESS.


> If this is simply a performance issue then minimally COMPACTBLOCKS
yes, My concern is the performance.

the comment of ISOLATE_NONE makes me confused.  :(

If you think we should update the COMPACTBLOCK in this case, my patch is 
wrong.


> still needs to be updated, we still want to see the tracepoint etc. To
ok.
> preserve that, I would suggest as an alternative to leave it returning
> ISOLATE_SUCCESS but move
>
>
>                 err = migrate_pages(&cc->migratepages, compaction_alloc,
>                                  (unsigned long)cc, false,
>                                  cc->sync ? MIGRATE_SYNC_LIGHT : MIGRATE_ASYNC);
>                  update_nr_listpages(cc);
>
> inside a if (nr_migrate) check to avoid some overhead.
>
thanks.
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 : fix the wrong return value for isolate_migratepages()

[Mel Gorman]

On Thu, Jan 05, 2012 at 06:31:14PM +0800, Huang Shijie wrote:
> >Why?
> >
> >
> >Returning ISOLATE_SUCCESS means that we fall through. This means busy
> >work in migrate_pages(), updating list accounting and the list. It's
> >wasteful but is it functionally incorrect? What problem did you observe?
>
> there may are many times the cc->migratepages is zero, but the
> return value is ISOLATE_SUCCESS.
> 

Ok, this is a reasonable assertion. How often will depend on a large
number of factors.

> >If this is simply a performance issue then minimally COMPACTBLOCKS
>
> yes, My concern is the performance.
> 

For future reference, please explain this in the changelog.

> the comment of ISOLATE_NONE makes me confused.  :(
> 

I see your confusion. The main difference between ISOLATE_NONE and
ISOLATE_SUCCESS is that scanning within the pageblock took place even
if no pages were isolated by the scan. Maybe it would be easier if
your patch clarified the meaning of the return values. Something like;

typedef enum {
        ISOLATE_ABORT,          /* Abort compaction now */
        ISOLATE_NONE,           /* No pages scanned, consider next pageblock */
        ISOLATE_SUCCESS,        /* Pages scanned and maybe isolated, migrate */
} isolate_migrate_t;

and then check if pages were really isolated on ISOLATE_SUCCESS?


> If you think we should update the COMPACTBLOCK in this case, my
> patch is wrong.
> 

I think the overhead is unnecessary and worth fixing but because the
scanning took place, the COMPACTBLOCK counter should be bumped and
the tracepoint triggered.

> >still needs to be updated, we still want to see the tracepoint etc. To
>
> ok.

Thanks.

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