Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f83f6106-ccc8-35d7-d275-acbc02b95d35@quicinc.com>
Date:   Thu, 8 Sep 2022 17:18:25 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Subject: Re: [PATCH] mm/page_owner.c: remove redudant drain_all_pages
Content-Language: en-US
To:     Mel Gorman <mgorman@techsingularity.net>
CC:     <akpm@linux-foundation.org>, <linux-mm@kvack.org>,
        <linux-kernel@vger.kernel.org>, Joonsoo Kim <js1304@gmail.com>
References: <1662537673-9392-1-git-send-email-quic_zhenhuah@quicinc.com>
 <20220908084008.tmerssqksyrg3knl@techsingularity.net>
From:   Zhenhua Huang <quic_zhenhuah@quicinc.com>
In-Reply-To: <20220908084008.tmerssqksyrg3knl@techsingularity.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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