lkml.org 
[lkml]   [2022]   [Sep]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] mm/page_owner.c: remove redudant drain_all_pages
From


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@quicinc.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@techsingularity.net>
>
> But just in case -- Joonsoo, can you clarify why drain_all_pages was
> originally called?
>

Thanks,
Zhenhua

\
 
 \ /
  Last update: 2022-09-08 11:19    [W:0.039 / U:0.764 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site