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

["Xu, Yanfei" <yanfei.xu@windriver.com>]

On 1/12/21 10:31 PM, Vlastimil Babka wrote:
> 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.
> 
Ah， got it. Compaction doesn't need these buddy pages during isolating， 
so it doesn't need to mark them MIGRATE_ISOLATE. THANK YOU.

>> 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
>>
>