Re: [PATCH 4/9] mm/compaction: simplify pfn iteration in isolate_freepages_range

[Kemeng Shi]

on 8/15/2023 6:07 PM, Baolin Wang wrote:
> 
> 
> On 8/15/2023 5:32 PM, Kemeng Shi wrote:
>>
>>
>> on 8/15/2023 4:38 PM, Baolin Wang wrote:
>>>
>>>
>>> On 8/5/2023 7:07 PM, Kemeng Shi wrote:
>>>> We call isolate_freepages_block in strict mode, continuous pages in
>>>> pageblock will be isolated if isolate_freepages_block successed.
>>>> Then pfn + isolated will point to start of next pageblock to scan
>>>> no matter how many pageblocks are isolated in isolate_freepages_block.
>>>> Use pfn + isolated as start of next pageblock to scan to simplify the
>>>> iteration.
>>>
>>> IIUC, the isolate_freepages_block() can isolate high-order free pages, which means the pfn + isolated can be larger than the block_end_pfn. So in your patch, the 'block_start_pfn' and 'block_end_pfn' can be in different pageblocks, that will break pageblock_pfn_to_page().
>>>
>> In for update statement, we always update block_start_pfn to pfn and
> 
> I mean, you changed to:
> 1) pfn += isolated;
> 2) block_start_pfn = pfn;
> 3) block_end_pfn = pfn + pageblock_nr_pages;
> 
> But in 1) pfn + isolated can go outside of the currnet pageblock if isolating a high-order page, for example, located in the middle of the next pageblock. So that the block_start_pfn can point to the middle of the next pageblock, not the start position. Meanwhile after 3), the block_end_pfn can point another pageblock. Or I missed something else?
> 
Ah, I miss to explain this in changelog.
In case we could we have buddy page with order higher than pageblock:
1. page in buddy page is aligned with it's order
2. order of page is higher than pageblock order
Then page is aligned with pageblock order. So pfn of page and isolated pages
count are both aligned pageblock order. So pfn + isolated is pageblock order
aligned.
>> update block_end_pfn to pfn + pageblock_nr_pages. So they should point
>> to the same pageblock. I guess you missed the change to update of
>> block_end_pfn. :)
>>>>
>>>> Signed-off-by: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx>
>>>> ---
>>>>    mm/compaction.c | 14 ++------------
>>>>    1 file changed, 2 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>>> index 684f6e6cd8bc..8d7d38073d30 100644
>>>> --- a/mm/compaction.c
>>>> +++ b/mm/compaction.c
>>>> @@ -733,21 +733,11 @@ isolate_freepages_range(struct compact_control *cc,
>>>>        block_end_pfn = pageblock_end_pfn(pfn);
>>>>          for (; pfn < end_pfn; pfn += isolated,
>>>> -                block_start_pfn = block_end_pfn,
>>>> -                block_end_pfn += pageblock_nr_pages) {
>>>> +                block_start_pfn = pfn,
>>>> +                block_end_pfn = pfn + pageblock_nr_pages) {
>>>>            /* Protect pfn from changing by isolate_freepages_block */
>>>>            unsigned long isolate_start_pfn = pfn;
>>>>    -        /*
>>>> -         * pfn could pass the block_end_pfn if isolated freepage
>>>> -         * is more than pageblock order. In this case, we adjust
>>>> -         * scanning range to right one.
>>>> -         */
>>>> -        if (pfn >= block_end_pfn) {
>>>> -            block_start_pfn = pageblock_start_pfn(pfn);
>>>> -            block_end_pfn = pageblock_end_pfn(pfn);
>>>> -        }
>>>> -
>>>>            block_end_pfn = min(block_end_pfn, end_pfn);
>>>>              if (!pageblock_pfn_to_page(block_start_pfn,
>>>
>