Messages in this thread | | | Date | Fri, 19 Nov 2010 11:08:08 +0000 | From | Mel Gorman <> | Subject | Re: [PATCH 7/8] mm: compaction: Use the LRU to get a hint on where compaction should start |
| |
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
| |