linux-kernel - Re: [PATCH] mm/compaction: fix the total

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <2503b955-79a9-4d21-9a25-34a6c33e688d@linux.alibaba.com>
Date: Thu, 14 Nov 2024 09:55:17 +0800
From: Qiang Liu <liuq131@...natelecom.cn>
To: liuq131@...natelecom.cn,
	akpm <akpm@...ux-foundation.org>
Cc: linux-mm <linux-mm@...ck.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Baolin Wang <baolin.wang@...ux.alibaba.com>
Subject: Re: [PATCH] mm/compaction: fix the total_isolated in strict mode

From: Baolin Wang <baolin.wang@...ux.alibaba.com>



On 2024/11/12 10:16, liuq131@...natelecom.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?

> /*
> * 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>`.
"