On Wed, Jan 25, 2012 at 10:55:05AM -0500, Rik van Riel wrote:
> On 01/25/2012 10:41 AM, Mel Gorman wrote:
>
> >>--- 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@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>