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

Vlastimil Babka
2016-10-07T05:27:52Z

On 10/04/2016 10:12 AM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> 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
> workkloads.
>
> 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@suse.com>
> ---
>
> Hi,
> 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.
>
> Thoughts?
>
> 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)
> return COMPACT_SKIPPED;
>
> trace_mm_compaction_try_to_compact_pages(order, gfp_mask, prio);
>