Re: [PATCH] mm/compaction: fix the total_isolated in strict mode

["liuq131@chinatelecom.cn" <liuq131@chinatelecom.cn>]

On 2024/11/14  10:58, baolin.wang@linux.alibaba.com wrote:
>On 2024/11/14 10:10, Qiang Liu wrote:
>> From: Baolin Wang <baolin.wang@linux.alibaba.com>
>> 
>> 
>> On 2024/11/12  17:47, baolin.wang@linux.alibaba.com wrote:
>>> On 2024/11/12 10:16, liuq131@chinatelecom.cn wrote:
>>>> "We assume that the block we are currently processing is distributed 
>>>> as follows:
>>>> 0   1   2                                                            511
>>>> --------------------------------------------------
>>>> |    |    
>>>> |                                                              |
>>>> ---------------------------------------------------
>>>> Index 0 and 1 are both pages with an order of 0.
>>>> Index 2 has a bogus order (let's assume the order is 9).
>>>> When the for loop reaches index 2, it will enter the following code:
>>>> /*
>>>>   * For compound pages such as THP and hugetlbfs, we can save
>>>>   * potentially a lot of iterations if we skip them at once.
>>>>   * The check is racy, but we can consider only valid values
>>>>   * and the only danger is skipping too much.
>>>>   */
>>>> if (PageCompound(page)) {
>>>>      const unsigned int order = compound_order(page);
>>>>      if (blockpfn + (1UL << order) <= end_pfn) {
>>>>          blockpfn += (1UL << order) - 1;
>>>>          page += (1UL << order) - 1;
>>>>          nr_scanned += (1UL << order) - 1;
>>>>      }
>>>>      goto isolate_fail;
>>>> }
>>>>
>>>> After exiting the for loop:
>>>> blockpfn =basepfn+ 2+2^9 = basepfn+514
>>>> endpfn  = basepfn +512
>>>> total_isolated = 2
>>>> nr_scanned = 514
>>>
>>> In your case, the 'blockpfn' will not be updated to 'basepfn+514', 
>>> because 'blockpfn + (1UL << order) > end_pfn', right? And remember the 
>>> 'end_pfn' is the end of the pageblock.
>>>
>>> So I'm still confused about your case. Is this from code inspection?
>> You're right, the situation where blockpfn > end_pfn would not actually 
>> occur here.
>> I encountered this issue in the 4.19 kernel, which did not have this check.
>> I didn't carefully examine this scenario later. Sorry about that.
>
>Never mind:)
>
>> However, when blockpfn == end_pfn, I believe the patch is still applicable,
>> but the git log needs to be updated. Is there still an opportunity to 
>> submit
>> a revised version of the patch?
>
>Of course yes, and please describe your issue clearly in the next 
>verion. However, IIUC when blockpfn == end_pfn in your case, the 
>'total_isolated' is still 0.
>
Indeed, that's the case
        /*
         * Be careful to not go outside of the pageblock.
         */
        if (unlikely(blockpfn > end_pfn))
                blockpfn = end_pfn;
This if statement is redundant

>>>> /*
>>>> * Be careful to not go outside of the pageblock.
>>>> */
>>>> if (unlikely(blockpfn > end_pfn))
>>>> blockpfn = end_pfn;
>>>> So this can happen
>>>>
>>>> /*
>>>>   * If strict isolation is requested by CMA then check that all the
>>>>   * pages requested were isolated. If there were any failures, 0 is
>>>>   * returned and CMA will fail.
>>>>   */
>>>> if (strict && blockpfn < end_pfn)
>>>> total_isolated = 0;
>>>>
>>>> If processed according to the old code, it will not enter the if 
>>>> statement to reset total_isolated, but the correct handling is to 
>>>> reset total_isolated to 0.
>>>
>>> Please do not top-posting:
>>>
>>> "
>>> - Use interleaved ("inline") replies, which makes your response easier 
>>> to read. (i.e. avoid top-posting -- the practice of putting your 
>>> answer above the quoted text you are responding to.) For more details, 
>>> see
>>>   :ref:`Documentation/process/submitting-patches.rst 
>>> <interleaved_replies>`.
>>> "
>>>