Re: mm: Question about the race condition of pages in buddy during isolating pages

[Vlastimil Babka <vbabka@suse.cz>]

On 1/12/21 3:24 PM, Xu, Yanfei wrote:
> Hi Vlastimil,

Hi,

> When I inspect the the codes about isolating pages, there are some lines
> from you make me confused. As blow:
> 
> Locate in isolate_migratepages_block()
> mm/compaction.c
> 
>  908                 /*
>  909                  * Skip if free. We read page order here without zone lock
>  910                  * which is generally unsafe, but the race window is small and
>  911                  * the worst thing that can happen is that we skip some
>  912                  * potential isolation targets.
>  913                  */
>  914                 if (PageBuddy(page)) {
>  915                         unsigned long freepage_order =
> buddy_order_unsafe(page);
>  916
>  917                         /*
>  918                          * Without lock, we cannot be sure that what we got is
>  919                          * a valid page order. Consider only values in the
>  920                          * valid order range to prevent low_pfn overflow.
>  921                          */
>  922                         if (freepage_order > 0 && freepage_order < MAX_ORDER))
>  923                                 low_pfn += (1UL << freepage_order) - 1;
>  924                         continue;
>  925                 }
> 
> These lines was isntroduced by the commit 99c0fd5e51c("mm, compaction:
> skip buddy pages by their order in the migrate scanner")
> 
> What I don't understand is that "the samll race window" mentioned in
> comments is which situation. I think before the
> isolate_migratepages_block() function is involved, those pageblocks have
> been marked MIGRATE_ISOLATE by set_migratetype_isolate() in
> start_isolate_page_range(). So the pages of those pageblocks in buddy

AFAIK that's only true when we start in alloc_contig_range(). In compact_zone()
-> isolate_migratepages() we don't use MIGRATE_ISOLATE as that would increase
the compaction cost a lot.

> will not be allocated, then the buddy_order_unsafe() here will get a
> certainly correct order value.
> 
> Could you please tell me what situation it will race with? :)
> 
> 
> 
> Thanks,
> Yanfei
>