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

[Mel Gorman <mgorman@suse.de>]

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>