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

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

[Xu, Yanfei]

Hi Vlastimil,

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

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

[Vlastimil Babka]

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
> 

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

[Xu, Yanfei]

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