On Wed, Nov 17, 2010 at 04:22:44PM +0000, Mel Gorman wrote: > + */ > + if (sc->lumpy_reclaim_mode & LUMPY_MODE_COMPACTION) > + nr_to_scan = max(nr_to_scan, (1UL << sc->order)); Just one nitpick: I'm not sure this is a good idea. We can scan quite some pages and we may do nothing on them. First I guess for symmetry this should be 2UL << sc->oder to match the 2UL << order in the watermark checks in compaction.c (maybe it should be 3UL if something considering the probability at least one page is mapped and won't be freed is quite high). But SWAP_CLUSTER_MAX is only 32 pages.. not even close to 1UL << 9 (hugepage order 9). So I think this can safely be removed... it only makes a difference for the stack with order 2. And for order 2 when we take the spinlocks we can take all 32 pages without screwing the "free" levels in any significant way, considering maybe only 4 pages are really freed in the end, and if all 32 pages are really freed (i.e. all plain clean cache), all that matters to avoid freeing more cache is to stick to compaction next time around (i.e. at the next allocation). And if compaction fails again next time around, then it's all right to shrink 32 more pages even for order 2... In short I'd delete the above chunk and to run the shrinker unmodified as this is a too lowlevel idea, and the only real life effect is to decrease VM scalability for kernel stack allocation a tiny bit, with no benefit whatsoever. It's subtle because the difference it'd makes it so infinitesimal and I can only imagine it's a worsening overall difference. > @@ -1425,6 +1438,9 @@ shrink_inactive_list(unsigned long nr_to_scan, struct zone *zone, > > putback_lru_pages(zone, sc, nr_anon, nr_file, &page_list); > > + if (sc->lumpy_reclaim_mode & LUMPY_MODE_COMPACTION) > + reclaimcompact_zone_order(zone, sc->order, sc->gfp_mask); > + > trace_mm_vmscan_lru_shrink_inactive(zone->zone_pgdat->node_id, > zone_idx(zone), > nr_scanned, nr_reclaimed, I'm worried about this one as the objective here is to increase the amount of free pages, and the loop won't stop until we reach nr_reclaimed >= nr_to_reclaim. I'm afraid it'd lead sometime to be doing an overwork of compaction here for no good. In short there is no feedback check into the loop to verify if this newly introduced compaction work in the shrinker lead us to get the hugepage and stop the loop. It sounds some pretty random compaction invocation here just to run it more frequently. nr_to_reclaim is only 32 anyway. So my suggestion is to remove it and let the shrinker do its thing without interleaving compaction inside the shrinker, without feedback check if the compaction actually succeeded (maybe 100% of free ram is contiguous already), and then try compaction again outside of the shrinker interleaving it with the shrinker as usual if the watermarks aren't satisfied yet after shrinker freed nr_to_reclaim pages. I prefer we keep separated the job of freeing more pages from the job of compacting the single free pages into higher order pages. It's only 32 pages being freed we're talking about here so no need to calling compaction more frequently (if something we should increase nr_to_reclaim to 512 and to call compaction less frequently). If the interleaving of the caller isn't ok then fix it in the caller and also update the nr_to_reclaim, but I think keeping those separated is way cleaner and the mixture is unnecessary. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxxx For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/ Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>