[PATCH] mm/page_owner.c: remove redudant drain_all_pages

[PATCH] mm/page_owner.c: remove redudant drain_all_pages

[Zhenhua Huang]

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>
---
 mm/page_owner.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/page_owner.c b/mm/page_owner.c
index 90023f9..54f3e03 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -524,8 +524,6 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	while (!pfn_valid(pfn) && (pfn & (MAX_ORDER_NR_PAGES - 1)) != 0)
 		pfn++;
 
-	drain_all_pages(NULL);
-
 	/* Find an allocated page */
 	for (; pfn < max_pfn; pfn++) {
 		/*
-- 
2.7.4

Re: [PATCH] mm/page_owner.c: remove redudant drain_all_pages

[Mel Gorman]

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.

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;

	Acked-by: Mel Gorman <mgorman@techsingularity.net>

But just in case -- Joonsoo, can you clarify why drain_all_pages was
originally called?

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/page_owner.c: remove redudant drain_all_pages

[Zhenhua Huang]

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