On Fri, Jan 13, 2012 at 10:31:12AM +0800, Huang Shijie wrote: > >>> /* > >>>+ * Watermarks for order-0 must be met for compaction. > >>>+ * During the migration, copies of pages need to be > >>>+ * allocated and for a short time, so the footprint is higher. > >>> * order == -1 is expected when compacting via > >>>- * /proc/sys/vm/compact_memory > >>>+ * /proc/sys/vm/compact_memory. > >>> */ > >>>- if (order == -1) > >>>- return COMPACT_CONTINUE; > >>>+ watermark = low_wmark_pages(zone) + > >>>+ ((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL<< order)); > >>> > >>>- /* > >>>- * Watermarks for order-0 must be met for compaction. Note the 2UL. > >>>- * This is because during migration, copies of pages need to be > >>>- * allocated and for a short time, the footprint is higher > >>>- */ > >>>- watermark = low_wmark_pages(zone) + (2UL<< order); > >>> if (!zone_watermark_ok(zone, 0, watermark, 0, 0)) > >>> return COMPACT_SKIPPED; > >>> > >>>+ if (order == -1) > >>>+ return COMPACT_CONTINUE; > >>>+ > >>> /* > >>> * fragmentation index determines if allocation failures are due to > >>> * low memory or external fragmentation > >>Is this patch meaningless? > >>I really think this patch is useful when the zone is nearly full. > >> > >Code wise the patch is fine. One reason why it fell off my radar is > >because you mangled the comments for no apparent reason. Specifically, > >after your patch is applied the code looks like this > > > > /* > > * Watermarks for order-0 must be met for compaction. > > * During the migration, copies of pages need to be > > * allocated and for a short time, so the footprint is higher. > > * order == -1 is expected when compacting via > > * /proc/sys/vm/compact_memory. > > */ > > watermark = low_wmark_pages(zone) + > > ((order == -1) ? (COMPACT_CLUSTER_MAX * 2) : (2UL<< order)); > "order == -1" first appears here. > > if (!zone_watermark_ok(zone, 0, watermark, 0, 0)) > > return COMPACT_SKIPPED; > > > > if (order == -1) > > return COMPACT_CONTINUE; > > > >The comment about "order == -1" is no longer with the code it refers > If I keep the comment here, someone may wonder why the `order == -1` > firstly appears above. > > I just want to keep the comment where it firstly appears. Don't you > think it's right? > Bah, I'm an idiot. When I glanced at this first, I missed that you altered the watermark check as well. When I said "Code wise the patch is fine", I was wrong. Compaction works in units of pageblocks and the watermark check is necessary. Reducing it to COMPACT_CLUSTER_MAX*2 leads to the possibility of compaction via /proc causing livelocks in low memory situations depending on the value of min_free_kbytes. -- Mel Gorman SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </a>