Re: [RFC PATCH] mm, compaction: allow compaction for GFP_NOFS requests

From: Vlastimil Babka
Date: Fri Oct 07 2016 - 01:28:28 EST

On 10/04/2016 10:12 AM, Michal Hocko wrote:
From: Michal Hocko <mhocko@xxxxxxxx>

compaction has been disabled for GFP_NOFS and GFP_NOIO requests since
the direct compaction was introduced by 56de7263fcf3 ("mm: compaction:
direct compact when a high-order allocation fails"). The main reason
is that the migration of page cache pages might recurse back to fs/io
layer and we could potentially deadlock. This is overly conservative
because all the anonymous memory is migrateable in the GFP_NOFS context
just fine. This might be a large portion of the memory in many/most

Remove the GFP_NOFS restriction and make sure that we skip all fs pages
(those with a mapping) while isolating pages to be migrated. We cannot
consider clean fs pages because they might need a metadata update so
only isolate pages without any mapping for nofs requests.

The effect of this patch will be probably very limited in many/most
workloads because higher order GFP_NOFS requests are quite rare,
although different configurations might lead to very different results
as GFP_NOFS usage is rather unleashed (e.g. I had hard time to trigger
any with my setup). But still there shouldn't be any strong reason to
completely back off and do nothing in that context. In the worst case
we just skip parts of the block with fs pages. This might be still
sufficient to make a progress for small orders.

Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>

I am sending this as an RFC because I am not completely sure this a) is
really worth it and b) it is 100% correct. I couldn't find any problems
when staring into the code but as mentioned in the changelog I wasn't
really able to trigger high order GFP_NOFS requests in my setup.


mm/compaction.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index badb92bf14b4..07254a73ee32 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -834,6 +834,13 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
page_count(page) > page_mapcount(page))
goto isolate_fail;

+ /*
+ * Only allow to migrate anonymous pages in GFP_NOFS context
+ * because those do not depend on fs locks.
+ */
+ if (!(cc->gfp_mask & __GFP_FS) && page_mapping(page))
+ goto isolate_fail;

Unless page can acquire a page_mapping between this check and migration, I don't see a problem with allowing this.

But make sure you don't break kcompactd and manual compaction from /proc, as they don't currently set cc->gfp_mask. Looks like until now it was only used to determine direct compactor's migratetype which is irrelevant in those contexts.

/* If we already hold the lock, we can skip some rechecking */
if (!locked) {
locked = compact_trylock_irqsave(zone_lru_lock(zone),
@@ -1696,14 +1703,16 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
unsigned int alloc_flags, const struct alloc_context *ac,
enum compact_priority prio)
- int may_enter_fs = gfp_mask & __GFP_FS;
int may_perform_io = gfp_mask & __GFP_IO;
struct zoneref *z;
struct zone *zone;
enum compact_result rc = COMPACT_SKIPPED;

- /* Check if the GFP flags allow compaction */
- if (!may_enter_fs || !may_perform_io)
+ /*
+ * Check if the GFP flags allow compaction - GFP_NOIO is really
+ * tricky context because the migration might require IO and
+ */
+ if (!may_perform_io)

trace_mm_compaction_try_to_compact_pages(order, gfp_mask, prio);