Messages in this thread | | | Date | Tue, 29 Jul 2014 11:12:50 +0200 | From | Vlastimil Babka <> | Subject | Re: [PATCH v5 02/14] mm, compaction: defer each zone individually instead of preferred zone |
| |
On 07/29/2014 08:38 AM, Joonsoo Kim wrote: >> /* Return values for compact_zone() and try_to_compact_pages() */ >> +/* compaction didn't start as it was deferred due to past failures */ >> +#define COMPACT_DEFERRED 0 >> /* compaction didn't start as it was not possible or direct reclaim was more suitable */ >> -#define COMPACT_SKIPPED 0 >> +#define COMPACT_SKIPPED 1 > > Hello, > > This change makes some users of compaction_suitable() failed > unintentionally, because they assume that COMPACT_SKIPPED is 0. > Please fix them according to this change.
Oops, good catch. Thanks!
>> @@ -2324,27 +2327,31 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order, >> order, zonelist, high_zoneidx, >> alloc_flags & ~ALLOC_NO_WATERMARKS, >> preferred_zone, classzone_idx, migratetype); >> + >> if (page) { >> - preferred_zone->compact_blockskip_flush = false; >> - compaction_defer_reset(preferred_zone, order, true); >> + struct zone *zone = page_zone(page); >> + >> + zone->compact_blockskip_flush = false; >> + compaction_defer_reset(zone, order, true); >> count_vm_event(COMPACTSUCCESS); >> return page; >> } >> >> /* >> + * last_compact_zone is where try_to_compact_pages thought >> + * allocation should succeed, so it did not defer compaction. >> + * But now we know that it didn't succeed, so we do the defer. >> + */ >> + if (last_compact_zone && mode != MIGRATE_ASYNC) >> + defer_compaction(last_compact_zone, order); > > I still don't understand why defer_compaction() is needed here. > defer_compaction() is intended for not struggling doing compaction on > the zone where we already have tried compaction and found that it > isn't suitable for compaction. Allocation failure doesn't tell us > that we have tried compaction for all the zone range so we shouldn't > make a decision here to defer compaction on this zone carelessly.
OK I can remove that, it should make the code nicer anyway. I also agree with the argument "for all the zone range" and I also realized that it's not (both before and after this patch) really the case. I planned to fix that in the future, but I can probably do it now. The plan is to call defer_compaction() only when compaction returned COMPACT_COMPLETE (and not COMPACT_PARTIAL) as it means the whole zone was scanned. Otherwise there will be bias towards the beginning of the zone in the migration scanner - compaction will be deferred half-way and then cached pfn's might be reset when it restarts, and the rest of the zone won't be scanned at all.
> Thanks. >
| |