On 2024/11/12 17:47, baolin.wang@xxxxxxxxxxxxxxxxx wrote:
>On 2024/11/12 10:16, liuq131@xxxxxxxxxxxxxxx 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.
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?
>> /*
>> * 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>`.
>"
