Re: [PATCH] mm/compaction: count pages and stop correctly during page isolation.

[Zi Yan <ziy@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

On 30 Oct 2020, at 10:50, Vlastimil Babka wrote:

> On 10/29/20 9:04 PM, Zi Yan wrote:
>> From: Zi Yan <ziy@nvidia.com>
>>
>> In isolate_migratepages_block, when cc->alloc_contig is true, we are
>> able to isolate compound pages, nr_migratepages and nr_isolated did not
>> count compound pages correctly, causing us to isolate more pages than we
>> thought. Use thp_nr_pages to count pages. Otherwise, we might be trapped
>> in too_many_isolated while loop, since the actual isolated pages can go
>> up to COMPACT_CLUSTER_MAX*512=16384, where COMPACT_CLUSTER_MAX is 32,
>> since we stop isolation after cc->nr_migratepages reaches to
>> COMPACT_CLUSTER_MAX.
>
> I wonder if a better fix would be to adjust the too_many_isolated() check so that if we have non-zero cc->nr_migratepages, we bail out from further isolation and migrate what we have immediately, instead of looping.

I just tested your fix and it works too. The difference is that with
your fix alloc_contig_range will fail quickly without killing the user
application mlocking THPs in the CMA region (for more context, please
see my other email to Andrew explaining how to reproduce in userspace),
whereas my fix will oom the user application and make alloc_contig_range
successful at the end.

Anyway, I will add your fix below and send v2:

diff --git a/mm/compaction.c b/mm/compaction.c
index ee1f8439369e..8fa11637ccfd 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -817,6 +817,9 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
         * delay for some time until fewer pages are isolated
         */
        while (unlikely(too_many_isolated(pgdat))) {
+               /* stop isolation if there are still pages not migrated */
+               if (cc->nr_migratepages)
+                       return 0;
                /* async migration should just abort */
                if (cc->mode == MIGRATE_ASYNC)
                        return 0;

>
> Because I can also imagine a hypothetical situation where multiple threads in parallel cause too_many_isolated() to be true, and will all loop there forever. The proposed fix should prevent such situation as well, AFAICT.

Yes. oom01 from ltp tests the multi-threaded situation and my fix works there too.


—
Best Regards,
Yan Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]