Re: [PATCH] mm: compaction: skip memory hole rapidly when isolating migratable pages

[David Hildenbrand <david@redhat.com>]

On 12.06.23 12:10, Baolin Wang wrote:
> 
> 
> On 6/12/2023 5:54 PM, David Hildenbrand wrote:
>> On 12.06.23 11:36, Baolin Wang wrote:
>>>
>>>
>>> On 6/12/2023 2:39 PM, Huang, Ying wrote:
>>>> Baolin Wang <baolin.wang@linux.alibaba.com> writes:
>>>>
>>>>> On some machines, the normal zone can have a large memory hole like
>>>>> below memory layout, and we can see the range from 0x100000000 to
>>>>> 0x1800000000 is a hole. So when isolating some migratable pages, the
>>>>> scanner can meet the hole and it will take more time to skip the large
>>>>> hole. From my measurement, I can see the isolation scanner will take
>>>>> 80us ~ 100us to skip the large hole [0x100000000 - 0x1800000000].
>>>>>
>>>>> So adding a new helper to fast search next online memory section
>>>>> to skip the large hole can help to find next suitable pageblock
>>>>> efficiently. With this patch, I can see the large hole scanning only
>>>>> takes < 1us.
>>>>>
>>>>> [    0.000000] Zone ranges:
>>>>> [    0.000000]   DMA      [mem 0x0000000040000000-0x00000000ffffffff]
>>>>> [    0.000000]   DMA32    empty
>>>>> [    0.000000]   Normal   [mem 0x0000000100000000-0x0000001fa7ffffff]
>>>>> [    0.000000] Movable zone start for each node
>>>>> [    0.000000] Early memory node ranges
>>>>> [    0.000000]   node   0: [mem 0x0000000040000000-0x0000000fffffffff]
>>>>> [    0.000000]   node   0: [mem 0x0000001800000000-0x0000001fa3c7ffff]
>>>>> [    0.000000]   node   0: [mem 0x0000001fa3c80000-0x0000001fa3ffffff]
>>>>> [    0.000000]   node   0: [mem 0x0000001fa4000000-0x0000001fa402ffff]
>>>>> [    0.000000]   node   0: [mem 0x0000001fa4030000-0x0000001fa40effff]
>>>>> [    0.000000]   node   0: [mem 0x0000001fa40f0000-0x0000001fa73cffff]
>>>>> [    0.000000]   node   0: [mem 0x0000001fa73d0000-0x0000001fa745ffff]
>>>>> [    0.000000]   node   0: [mem 0x0000001fa7460000-0x0000001fa746ffff]
>>>>> [    0.000000]   node   0: [mem 0x0000001fa7470000-0x0000001fa758ffff]
>>>>> [    0.000000]   node   0: [mem 0x0000001fa7590000-0x0000001fa7ffffff]
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>>> ---
>>>>>     include/linux/mmzone.h | 10 ++++++++++
>>>>>     mm/compaction.c        | 23 ++++++++++++++++++++++-
>>>>>     2 files changed, 32 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
>>>>> index 5a7ada0413da..87e6c535d895 100644
>>>>> --- a/include/linux/mmzone.h
>>>>> +++ b/include/linux/mmzone.h
>>>>> @@ -2000,6 +2000,16 @@ static inline unsigned long
>>>>> next_present_section_nr(unsigned long section_nr)
>>>>>         return -1;
>>>>>     }
>>>>> +static inline unsigned long next_online_section_nr(unsigned long
>>>>> section_nr)
>>>>> +{
>>>>> +    while (++section_nr <= __highest_present_section_nr) {
>>>>> +        if (online_section_nr(section_nr))
>>>>> +            return section_nr;
>>>>> +    }
>>>>> +
>>>>> +    return -1UL;
>>>>> +}
>>>>> +
>>>>>     /*
>>>>>      * These are _only_ used during initialisation, therefore they
>>>>>      * can use __initdata ...  They could have names to indicate
>>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>>> index 3398ef3a55fe..3a55fdd20c49 100644
>>>>> --- a/mm/compaction.c
>>>>> +++ b/mm/compaction.c
>>>>> @@ -229,6 +229,21 @@ static void reset_cached_positions(struct zone
>>>>> *zone)
>>>>>                     pageblock_start_pfn(zone_end_pfn(zone) - 1);
>>>>>     }
>>>>> +static unsigned long skip_hole_pageblock(unsigned long start_pfn)
>>>>> +{
>>>>> +    unsigned long next_online_nr;
>>>>> +    unsigned long start_nr = pfn_to_section_nr(start_pfn);
>>>>> +
>>>>> +    if (online_section_nr(start_nr))
>>>>> +        return -1UL;
>>>>
>>>> Define a macro for the maigic "-1UL"?  Which is used for multiple times
>>>> in the patch.
>>>
>>> I am struggling to find a readable macro for these '-1UL', since the
>>> '-1UL' in next_online_section_nr() indicates that it can not find an
>>> online section. However the '-1' in skip_hole_pageblock() indicates that
>>> it can not find an online pfn.
>>
>> Maybe something like
>>
>> #define SECTION_NR_INVALID -1UL
> 
> Actually we already have a NR_MEM_SECTIONS macro, which means it is an
> invalid section if the section nr >= NR_MEM_SECTIONS. So using
> NR_MEM_SECTIONS seems more suitable?

Indeed, that would also work!

-- 
Cheers,

David / dhildenb