linux-kernel - Re: [PATCH] mm/page_owner.c: remove redudant drain_all

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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f83f6106-ccc8-35d7-d275-acbc02b95d35@quicinc.com>
Date:   Thu, 8 Sep 2022 17:18:25 +0800
From:   Zhenhua Huang <quic_zhenhuah@...cinc.com>
To:     Mel Gorman <mgorman@...hsingularity.net>
CC:     <akpm@...ux-foundation.org>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>, Joonsoo Kim <js1304@...il.com>
Subject: Re: [PATCH] mm/page_owner.c: remove redudant drain_all_pages



On 2022/9/8 16:40, Mel Gorman wrote:
> On Wed, Sep 07, 2022 at 04:01:13PM +0800, Zhenhua Huang wrote:
>> Page owner info of pages in pcp list have already been reset:
>> 	free_unref_page
>> 		-> free_unref_page_prepare
>> 			-> free_pcp_prepare
>> 				-> free_pages_prepare which do page owner
>> 				reset
>> 		-> free_unref_page_commit which add pages into pcp list
>> It can also be confirmed from dump that page owner info of pcp pages are
>> correct. Hence there is no more need to drain when reading.
>>
>> Signed-off-by: Zhenhua Huang <quic_zhenhuah@...cinc.com>
> 
> This is subtle because there is no comment explaining why drain_all_pages
> is called and git history does not help. I agree that the page owner
> information has already been reset and has been since the very beginning
> but I do not think that is *why* drain_all_pages is called here.

Thanks Mel, to be honest I've checked git history and also didn't find 
the author's purpose for this call.

> 
> After the drain_all_pages, there is a fairly standard PFN walker with this
> in it;
> 
>          /* Find an allocated page */
>          for (; pfn < max_pfn; pfn++) {
> 	....
>                  page = pfn_to_page(pfn);
>                  if (PageBuddy(page)) {
>                          unsigned long freepage_order = buddy_order_unsafe(page);
> 
>                          if (freepage_order < MAX_ORDER)
>                                  pfn += (1UL << freepage_order) - 1;
>                          continue;
>                  }	
> 	....
>          }
> 
> The PFN walker is trying to skip free pages efficiently and PCP pages
> are not buddy pages so the order is unknown. The order *can* be known but
> it's risky to try detecting it. I suspect the drain_all_pages is called
> to move PCP pages to the buddy list so they can identified as buddy pages
> and skipped and has nothing to do with resetting the page owner.
> 
> If that is correct then I think it is overkill to drain the PCP lists
> to marginally improve the efficiency of the PFN walker and the drain is
> subject to a race. Just because the PCP lists are drained does not mean
> a new PCP page will be added during the PFN walk. Furthermore, PCP pages
> get skipped because PAGE_EXT_OWNER_ALLOCATED is cleared so it's not about
> scan safety. The drain is a guaranteed expensive operation that is unlikely
> to be offset by a slight increase in efficiently of the PFN walker when
> skipping free pages so the drain_all_pages should be dropped. I believe
> the patch itself is correct but the changelog needs to be changed.
> 
> With a changelog stating that the patch is removing an expensive and
> unnecessary operation as PCP pages are safely skipped;

Sure.
I will stat your thoughts in changelog.

> 
> 	Acked-by: Mel Gorman <mgorman@...hsingularity.net>
> 
> But just in case -- Joonsoo, can you clarify why drain_all_pages was
> originally called?
> 

Thanks,
Zhenhua