From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.7 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C638EC3F2D7 for ; Thu, 5 Mar 2020 04:41:34 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8AB3F2072D for ; Thu, 5 Mar 2020 04:41:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8AB3F2072D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 209CD6B0003; Wed, 4 Mar 2020 23:41:34 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 192AB6B0005; Wed, 4 Mar 2020 23:41:34 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 05AB16B0007; Wed, 4 Mar 2020 23:41:34 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0170.hostedemail.com [216.40.44.170]) by kanga.kvack.org (Postfix) with ESMTP id DE94C6B0003 for ; Wed, 4 Mar 2020 23:41:33 -0500 (EST) Received: from smtpin05.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 60C6D8248047 for ; Thu, 5 Mar 2020 04:41:33 +0000 (UTC) X-FDA: 76560060066.05.dolls55_2a994c9543b2f X-HE-Tag: dolls55_2a994c9543b2f X-Filterd-Recvd-Size: 8362 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by imf40.hostedemail.com (Postfix) with ESMTP for ; Thu, 5 Mar 2020 04:41:32 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Mar 2020 20:41:31 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,516,1574150400"; d="scan'208";a="240684644" Received: from yhuang-dev.sh.intel.com (HELO yhuang-dev) ([10.239.159.23]) by orsmga003.jf.intel.com with ESMTP; 04 Mar 2020 20:41:27 -0800 From: "Huang\, Ying" To: David Hildenbrand Cc: Andrew Morton , , , Dave Hansen , Mel Gorman , Vlastimil Babka , Zi Yan , Michal Hocko , Peter Zijlstra , Minchan Kim , Johannes Weiner , Hugh Dickins Subject: Re: [PATCH -V2] mm: Add PageLayzyFree() helper functions for MADV_FREE References: <20200304081732.563536-1-ying.huang@intel.com> Date: Thu, 05 Mar 2020 12:41:26 +0800 In-Reply-To: (David Hildenbrand's message of "Wed, 4 Mar 2020 09:28:37 +0100") Message-ID: <87y2sf1ki1.fsf@yhuang-dev.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: David Hildenbrand writes: > On 04.03.20 09:17, Huang, Ying wrote: >> From: Huang Ying >> >> Now PageSwapBacked() is used as the helper function to check whether >> pages have been freed lazily via MADV_FREE. This isn't very obvious. >> So Dave suggested to add PageLazyFree() family helper functions to >> improve the code readability. >> >> Signed-off-by: "Huang, Ying" >> Suggested-by: Dave Hansen >> Cc: David Hildenbrand >> Cc: Mel Gorman >> Cc: Vlastimil Babka >> Cc: Zi Yan >> Cc: Michal Hocko >> Cc: Peter Zijlstra >> Cc: Minchan Kim >> Cc: Johannes Weiner >> Cc: Hugh Dickins >> --- >> >> Changelog: >> >> v2: >> >> - Avoid code bloat via removing VM_BUG_ON_PAGE(), which doesn't exist >> in the original code. Now there is no any text/data/bss size >> change. >> >> - Fix one wrong replacement in try_to_unmap_one(). >> --- >> include/linux/page-flags.h | 25 +++++++++++++++++++++++++ >> mm/rmap.c | 4 ++-- >> mm/swap.c | 11 +++-------- >> mm/vmscan.c | 7 +++---- >> 4 files changed, 33 insertions(+), 14 deletions(-) >> >> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h >> index 49c2697046b9..192b593750d3 100644 >> --- a/include/linux/page-flags.h >> +++ b/include/linux/page-flags.h >> @@ -498,6 +498,31 @@ static __always_inline int PageKsm(struct page *page) >> TESTPAGEFLAG_FALSE(Ksm) >> #endif >> >> +/* >> + * For pages freed lazily via MADV_FREE. lazyfree pages are clean >> + * anonymous pages. They have SwapBacked flag cleared to distinguish >> + * with normal anonymous pages > > "They don't have PG_swapbacked set, to distinguish them from normal > anonymous pages." ? Thanks! >> + */ >> +static __always_inline int __PageLazyFree(struct page *page) >> +{ >> + return !PageSwapBacked(page); >> +} >> + >> +static __always_inline int PageLazyFree(struct page *page) >> +{ >> + return PageAnon(page) && __PageLazyFree(page); >> +} >> + >> +static __always_inline void SetPageLazyFree(struct page *page) >> +{ >> + ClearPageSwapBacked(page); >> +} >> + >> +static __always_inline void ClearPageLazyFree(struct page *page) >> +{ >> + SetPageSwapBacked(page); >> +} >> + >> u64 stable_page_flags(struct page *page); >> >> static inline int PageUptodate(struct page *page) >> diff --git a/mm/rmap.c b/mm/rmap.c >> index 1c02adaa233e..6ec96c8e7826 100644 >> --- a/mm/rmap.c >> +++ b/mm/rmap.c >> @@ -1609,7 +1609,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, >> } >> >> /* MADV_FREE page check */ >> - if (!PageSwapBacked(page)) { >> + if (__PageLazyFree(page)) { >> if (!PageDirty(page)) { >> /* Invalidate as we cleared the pte */ >> mmu_notifier_invalidate_range(mm, >> @@ -1623,7 +1623,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, >> * discarded. Remap the page to page table. >> */ >> set_pte_at(mm, address, pvmw.pte, pteval); >> - SetPageSwapBacked(page); >> + ClearPageLazyFree(page); >> ret = false; >> page_vma_mapped_walk_done(&pvmw); >> break; >> diff --git a/mm/swap.c b/mm/swap.c >> index c1d3ca80ea10..d83f2cd4cdb8 100644 >> --- a/mm/swap.c >> +++ b/mm/swap.c >> @@ -563,7 +563,7 @@ static void lru_deactivate_fn(struct page *page, struct lruvec *lruvec, >> static void lru_lazyfree_fn(struct page *page, struct lruvec *lruvec, >> void *arg) >> { >> - if (PageLRU(page) && PageAnon(page) && PageSwapBacked(page) && >> + if (PageLRU(page) && PageAnon(page) && !__PageLazyFree(page) && > > See my comment below > >> !PageSwapCache(page) && !PageUnevictable(page)) { >> bool active = PageActive(page); >> >> @@ -571,12 +571,7 @@ static void lru_lazyfree_fn(struct page *page, struct lruvec *lruvec, >> LRU_INACTIVE_ANON + active); >> ClearPageActive(page); >> ClearPageReferenced(page); >> - /* >> - * lazyfree pages are clean anonymous pages. They have >> - * SwapBacked flag cleared to distinguish normal anonymous >> - * pages >> - */ >> - ClearPageSwapBacked(page); >> + SetPageLazyFree(page); >> add_page_to_lru_list(page, lruvec, LRU_INACTIVE_FILE); >> >> __count_vm_events(PGLAZYFREE, hpage_nr_pages(page)); >> @@ -678,7 +673,7 @@ void deactivate_page(struct page *page) >> */ >> void mark_page_lazyfree(struct page *page) >> { >> - if (PageLRU(page) && PageAnon(page) && PageSwapBacked(page) && >> + if (PageLRU(page) && PageAnon(page) && !__PageLazyFree(page) && > > See my comment below. > >> !PageSwapCache(page) && !PageUnevictable(page)) { >> struct pagevec *pvec = &get_cpu_var(lru_lazyfree_pvecs); >> >> diff --git a/mm/vmscan.c b/mm/vmscan.c >> index eca49a1c2f68..40bb41ada2d2 100644 >> --- a/mm/vmscan.c >> +++ b/mm/vmscan.c >> @@ -1043,8 +1043,7 @@ static void page_check_dirty_writeback(struct page *page, >> * Anonymous pages are not handled by flushers and must be written >> * from reclaim context. Do not stall reclaim based on them >> */ >> - if (!page_is_file_cache(page) || >> - (PageAnon(page) && !PageSwapBacked(page))) { >> + if (!page_is_file_cache(page) || PageLazyFree(page)) { >> *dirty = false; >> *writeback = false; >> return; >> @@ -1235,7 +1234,7 @@ static unsigned long shrink_page_list(struct list_head *page_list, >> * Try to allocate it some swap space here. >> * Lazyfree page could be freed directly >> */ >> - if (PageAnon(page) && PageSwapBacked(page)) { >> + if (PageAnon(page) && !__PageLazyFree(page)) { > > I don't think this chunk makes this code easier to understand. I'd just > keep this as is. IOW, I prefer PageSwapBacked() over !__PageLazyFree(). > > > In general, I don't think this patch really improves the situation ... > it's only a handful of places where this change slightly makes the code > easier to understand. And there, only slightly ... I'd prefer better > comments instead (e.g., in PageAnon()), documenting what it means for a > anon page to either have PageSwapBacked() set or not. Personally, I still prefer the better named functions than the comments here and there. But I can understand that people may have different flavor. Best Regards, Huang, Ying