| Message ID | 1662537673-9392-1-git-send-email-quic_zhenhuah@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/page_owner.c: remove redudant drain_all_pages | expand |
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?
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
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++) { /*
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(-)