Re: [PATCH 7/8] mm: compaction: Use the LRU to get a hint on where compaction should start

[Mel Gorman <mel@csn.ul.ie>]

On Thu, Nov 18, 2010 at 07:46:59PM +0100, Andrea Arcangeli wrote:
> On Wed, Nov 17, 2010 at 04:22:48PM +0000, Mel Gorman wrote:
> > +	if (!cc->migrate_pfn)
> > +		cc->migrate_pfn = zone->zone_start_pfn;
> 
> wouldn't it remove a branch if the caller always set migrate_pfn?
> 

If try_to_compact_pages() used it, it would migrate old pages and then
later enter direct reclaim where it reclaimed the pages it just
migrated. It was double work. That said, I neglected to check if
migration affects the age of pages. Offhand, I think it does so the
problem wouldn't apply so it was flawed reasoning.

> > +	if (!cc->free_pfn) {
> > +		cc->free_pfn = zone->zone_start_pfn + zone->spanned_pages;
> > +		cc->free_pfn &= ~(pageblock_nr_pages-1);
> > +	}
> 
> Who sets free_pfn to zero? Previously this was always initialized.
> 

Initialised on stack.

> > @@ -523,7 +539,23 @@ unsigned long reclaimcompact_zone_order(struct zone *zone,
> >  	INIT_LIST_HEAD(&cc.freepages);
> >  	INIT_LIST_HEAD(&cc.migratepages);
> >  
> > -	return compact_zone(zone, &cc);
> > +	/* Get a hint on where to start compacting from the LRU */
> > +	anon_page = lru_to_page(&zone->lru[LRU_BASE + LRU_INACTIVE_ANON].list);
> > +	file_page = lru_to_page(&zone->lru[LRU_BASE + LRU_INACTIVE_FILE].list);
> > +	cc.migrate_pfn = min(page_to_pfn(anon_page), page_to_pfn(file_page));
> > +	cc.migrate_pfn = ALIGN(cc.migrate_pfn, pageblock_nr_pages);
> > +	start_migrate_pfn = cc.migrate_pfn;
> > +
> > +	ret = compact_zone(zone, &cc);
> > +
> > +	/* Restart migration from the start of zone if the hint did not work */
> > +	if (!zone_watermark_ok(zone, cc.order, low_wmark_pages(zone), 0, 0)) {
> > +		cc.migrate_pfn = 0;
> > +		cc.abort_migrate_pfn = start_migrate_pfn;
> > +		ret = compact_zone(zone, &cc);
> > +	}
> > +
> 
> I doubt it works ok if the list is empty... Maybe it's safer to
> validate the migrate_pfn against the zone pfn start/end before
> setting it in the migrate_pfn.
> 

You're right, this could be unsafe.

> Interesting this heuristic slowed down the benchmark, it should lead
> to the exact opposite thanks to saving some cpu. So I guess maybe it's
> not worth it. I see it increases the ratio of compaction of a tiny
> bit, but if a tiny bit of better compaction comes at the expenses of
> an increased runtime I don't like it and I'd drop it... It's not
> making enough difference, further we could extend it to check the
> "second" page in the list and so on... so we can just go blind. All it
> matters is that we use a clock algorithm and I guess this screwes it
> and this is why it leads to increased time.
> 

The variation was within the noise but yes, maybe this is not such a great
idea and the figures are not very compelling. I'm going to drop it from the
series.

Thanks

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab