On Wed, Jan 25, 2012 at 10:55:05AM -0500, Rik van Riel wrote:

On 01/25/2012 10:41 AM, Mel Gorman wrote:

quoted

quoted

--- a/mm/compaction.c
+++ b/mm/compaction.c

@@ -673,9 +673,18 @@ static int __compact_pgdat(pg_data_t *pgdat, struct compact_control *cc)
 		INIT_LIST_HEAD(&cc->freepages);
 		INIT_LIST_HEAD(&cc->migratepages);

-		if (cc->order<  0 || !compaction_deferred(zone))
+		if (cc->order<  0 || !compaction_deferred(zone, cc->order))
 			compact_zone(zone, cc);

+		if (cc->order>  0) {
+			int ok = zone_watermark_ok(zone, cc->order,
+						low_wmark_pages(zone), 0, 0);
+			if (ok&&  cc->order>  zone->compact_order_failed)
+				zone->compact_order_failed = cc->order + 1;
+			else if (!ok&&  cc->sync)
+				defer_compaction(zone, cc->order);
+		}
+

That needs a comment. I think what you're trying to do is reset
compat_order_failed once compaction is successful.

The "!ok&&  cc->sync" check may be broken. __compact_pgdat() is
called from kswapd, not direct compaction so cc->sync will not be true.

The problem with doing that is that we would be deferring
synchronous compaction (by allocators), just because
asynchronous compaction from kswapd failed...

I should have been clear. I was not suggesting that we defer compaction
here for !cc->sync. I was pointing out that the code as-is is dead.

That is the reason the code is like it is above.  And
indeed, it will not defer compaction from this code path
right now.

Ok, that was my understanding, I just wanted to be sure I understood
your intentions.

Then again, neither does async compaction from page
allocators defer compaction - only sync compaction does.

Yep, this is on purpose.

If it turns out we need a separate compaction deferral
for async compaction, we can always introduce that later,
and this code will be ready for it.

Ok, I can accept that.

If you prefer, I can replace the whole "else if" bit with
a big fat comment explaining why we cannot currently
defer compaction from this point.

Explaining that it is dead code for kswapd would also do.

If you do replace the code, add a WARN_ON(cc->sync) in case the
assumption changes in the future so defer_compaction() gets thought
about properly.

-- 
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>