Messages in this thread | | | Date | Thu, 8 Sep 2022 17:18:25 +0800 | Subject | Re: [PATCH] mm/page_owner.c: remove redudant drain_all_pages | From | 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
| |