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

[Mel Gorman <mgorman@suse.de>]

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>