mm: remove activate_page() from unuse_pte()
Subsystems (5)
PATCH v2 1/3 mm: remove activate_page() from unuse_pte()
We don't initially add anon pages to active lruvec after
commit b518154e59aa ("mm/vmscan: protect the workingset on anonymous LRU"). Remove activate_page() from unuse_pte(), which seems to be missed by the commit. And make the function static while we are at it.
Before the commit, we called lru_cache_add_active_or_unevictable() to add new ksm pages to active lruvec. Therefore, activate_page() wasn't necessary for them in the first place.
--- include/linux/swap.h | 1 - mm/swap.c | 4 ++-- mm/swapfile.c | 5 ----- 3 files changed, 2 insertions(+), 8 deletions(-)
include/linux/swap.h +0 -1
@@ -340,7 +340,6 @@ extern void lru_note_cost_page(struct page *);extern void lru_cache_add (struct page *);extern void lru_add_page_tail (struct page *page,struct page *page_tail,struct lruvec *lruvec,struct list_head *head); -extern void activate_page (struct page *);extern void mark_page_accessed (struct page *);extern void lru_add_drain (void );extern void lru_add_drain_cpu (int cpu);
mm/swap.c +2 -2
@@ -348,7 +348,7 @@ static bool need_activate_page_drain(int cpu)return pagevec_count (&per_cpu (lru_pvecs.activate_page, cpu)) !=0 ; } -void activate_page (struct page *page) +static void activate_page (struct page *page) { page =compound_head (page);if (PageLRU (page) && !PageActive (page) && !PageUnevictable (page)) {
@@ -368,7 +368,7 @@ static inline void activate_page_drain(int cpu) { } -void activate_page (struct page *page) +static void activate_page (struct page *page) {pg_data_t *pgdat =page_pgdat (page);
mm/swapfile.c +0 -5
@@ -1925,11 +1925,6 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,lru_cache_add_inactive_or_unevictable (page, vma); }swap_free (entry); -/* -* Move the page to the active list so it is not -* immediately swapped out again after swapon. -*/ -activate_page (page); out:pte_unmap_unlock (pte, ptl);if (page != swapcache) {
Signature · 1 line
2.28.0.220.ged08abb693-goog
PATCH v2 2/3 mm: remove superfluous __ClearPageActive()
To activate a page, mark_page_accessed() always holds a reference on it. It either gets a new reference when adding a page to lru_pvecs.activate_page or reuses an existing one it previously got when it added a page to lru_pvecs.lru_add. So it doesn't call SetPageActive() on a page that doesn't have any reference left. Therefore, the race is impossible these days (I didn't brother to dig into its history).
For other paths, namely reclaim and migration, a reference count is always held while calling SetPageActive() on a page.
SetPageSlabPfmemalloc() also uses SetPageActive(), but it's irrelevant to LRU pages.
--- mm/memremap.c | 2 -- mm/swap.c | 2 -- 2 files changed, 4 deletions(-)
mm/memremap.c +0 -2
@@ -451,8 +451,6 @@ void free_devmap_managed_page(struct page *page)return ; } -/* Clear Active bit in case of parallel mark_page_accessed */ -__ClearPageActive (page);__ClearPageWaiters (page);mem_cgroup_uncharge (page);
mm/swap.c +0 -2
@@ -900,8 +900,6 @@ void release_pages(struct page **pages, int nr)del_page_from_lru_list (page, lruvec,page_off_lru (page)); } -/* Clear Active bit in case of parallel mark_page_accessed */ -__ClearPageActive (page);__ClearPageWaiters (page);list_add (&page->lru, &pages_to_free);
Signature · 1 line
2.28.0.220.ged08abb693-goog
Re: [PATCH v2 2/3] mm: remove superfluous __ClearPageActive()
Quoted 9 lines · Yu Zhao
To activate a page,
mark_page_accessed()always holds a reference on it. It either gets a new reference when adding a page to lru_pvecs.activate_page or reuses an existing one it previously got when it added a page to lru_pvecs.lru_add. So it doesn't callSetPageActive()on a page that doesn't have any reference left. Therefore, the race is impossible these days (I didn't brother to dig into its history).
Quoted 5 lines
For other paths, namely reclaim and migration, a reference count is always held while calling
SetPageActive()on a page.
SetPageSlabPfmemalloc()also usesSetPageActive(), but it's irrelevant to LRU pages.
Seems fine to me. Reviewed-by: Yang Shi <[email protected]>
Quoted 37 lines
Signed-off-by Yu Zhao <[email protected]>--- mm/memremap.c | 2 -- mm/swap.c | 2 -- 2 files changed, 4 deletions(-)mm/memremap.c +0
-2@@ -451,8 +451,6 @@ void free_devmap_managed_page(struct page *page)return ; } -/* Clear Active bit in case of parallel mark_page_accessed */ -__ClearPageActive (page);__ClearPageWaiters (page);mem_cgroup_uncharge (page);mm/swap.c +0
-3@@ -900,8 +900,6 @@ void release_pages(struct page **pages, int nr)del_page_from_lru_list (page, lruvec,page_off_lru (page)); } -/* Clear Active bit in case of parallel mark_page_accessed */ -__ClearPageActive (page);__ClearPageWaiters (page);list_add (&page->lru, &pages_to_free); --2.28.0.220.ged08abb693-goog
PATCH v2 3/3 mm: remove superfluous __ClearPageWaiters()
In reply to Yu ZhaoPresumably __ClearPageWaiters() was added to follow the previously removed __ClearPageActive() pattern.
Only flags that are in PAGE_FLAGS_CHECK_AT_FREE needs to be properly cleared because otherwise we think there may be some kind of leak. PG_waiters is not one of those flags and leaving the clearing to PAGE_FLAGS_CHECK_AT_PREP is more appropriate.
--- include/linux/page-flags.h | 2 +- mm/filemap.c | 2 ++ mm/memremap.c | 2 -- mm/swap.c | 3 --- 4 files changed, 3 insertions(+), 6 deletions(-)
include/linux/page-flags.h +1 -1
@@ -318,7 +318,7 @@ static inline int TestClearPage##uname(struct page *page) { return 0; } TESTSETFLAG_FALSE(uname) TESTCLEARFLAG_FALSE(uname) __PAGEFLAG(Locked, locked, PF_NO_TAIL) -PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) __CLEARPAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) +PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) PAGEFLAG(Error, error, PF_NO_TAIL) TESTCLEARFLAG(Error, error, PF_NO_TAIL) PAGEFLAG(Referenced, referenced, PF_HEAD) TESTCLEARFLAG(Referenced, referenced, PF_HEAD)
mm/filemap.c +2 -0
@@ -1079,6 +1079,8 @@ static void wake_up_page_bit(struct page *page, int bit_nr)* other pages on it. * * That's okay, it's a rare case. The next waker will clear it. +* Otherwise the bit will be cleared by PAGE_FLAGS_CHECK_AT_PREP +* when the page is being freed. */ }spin_unlock_irqrestore (&q->lock, flags);
mm/memremap.c +0 -2
@@ -451,8 +451,6 @@ void free_devmap_managed_page(struct page *page)return ; } -__ClearPageWaiters (page); -mem_cgroup_uncharge (page);/*
mm/swap.c +0 -3
@@ -90,7 +90,6 @@ static void __page_cache_release(struct page *page)del_page_from_lru_list (page, lruvec,page_off_lru (page));spin_unlock_irqrestore (&pgdat->lru_lock, flags); } -__ClearPageWaiters (page); }static void __put_single_page (struct page *page)
@@ -900,8 +899,6 @@ void release_pages(struct page **pages, int nr)del_page_from_lru_list (page, lruvec,page_off_lru (page)); } -__ClearPageWaiters (page); -list_add (&page->lru, &pages_to_free); }if (locked_pgdat)
Signature · 1 line
2.28.0.220.ged08abb693-goog
Re: [PATCH v2 3/3] mm: remove superfluous __ClearPageWaiters()
Quoted 8 lines · Yu Zhao
Presumably
__ClearPageWaiters()was added to follow the previously removed__ClearPageActive()pattern.Only flags that are in
PAGE_FLAGS_CHECK_AT_FREEneeds to be properly cleared because otherwise we think there may be some kind of leak.PG_waitersis not one of those flags and leaving the clearing toPAGE_FLAGS_CHECK_AT_PREPis more appropriate.
Actually TBH I'm not very keen to this change, it seems the clearing is just moved around and the allocation side pays for that instead of free side.
Quoted 73 lines
Signed-off-by Yu Zhao <[email protected]>--- include/linux/page-flags.h | 2 +- mm/filemap.c | 2 ++ mm/memremap.c | 2 -- mm/swap.c | 3 --- 4 files changed, 3 insertions(+), 6 deletions(-)include/linux/page-flags.h +1
-1@@ -318,7 +318,7 @@ static inline int TestClearPage##uname(struct page *page) { return 0; } TESTSETFLAG_FALSE(uname) TESTCLEARFLAG_FALSE(uname) __PAGEFLAG(Locked, locked, PF_NO_TAIL) -PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) __CLEARPAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) +PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) PAGEFLAG(Error, error, PF_NO_TAIL) TESTCLEARFLAG(Error, error, PF_NO_TAIL) PAGEFLAG(Referenced, referenced, PF_HEAD) TESTCLEARFLAG(Referenced, referenced, PF_HEAD)mm/filemap.c +2
-0@@ -1079,6 +1079,8 @@ static void wake_up_page_bit(struct page *page, int bit_nr)* other pages on it. * * That's okay, it's a rare case. The next waker will clear it. +* Otherwise the bit will be cleared by PAGE_FLAGS_CHECK_AT_PREP +* when the page is being freed. */ }spin_unlock_irqrestore (&q->lock, flags);mm/memremap.c +0
-2@@ -451,8 +451,6 @@ void free_devmap_managed_page(struct page *page)return ; } -__ClearPageWaiters (page); -mem_cgroup_uncharge (page);/* mm/swap.c +0
-4@@ -90,7 +90,6 @@ static void __page_cache_release(struct page *page)del_page_from_lru_list (page, lruvec,page_off_lru (page));spin_unlock_irqrestore (&pgdat->lru_lock, flags); } -__ClearPageWaiters (page); }static void __put_single_page (struct page *page)@@ -900,8 +899,6 @@ void release_pages(struct page **pages, int nr)del_page_from_lru_list (page, lruvec,page_off_lru (page)); } -__ClearPageWaiters (page); -list_add (&page->lru, &pages_to_free); }if (locked_pgdat) --2.28.0.220.ged08abb693-goog
Re: [PATCH v2 3/3] mm: remove superfluous __ClearPageWaiters()
Quoted 5 lines · Yang Shi
Quoted 4 lines · Yu Zhao
Presumably
__ClearPageWaiters()was added to follow the previously removed__ClearPageActive()pattern.
Quoted 8 lines
Quoted 4 lines
Only flags that are in
PAGE_FLAGS_CHECK_AT_FREEneeds to be properly cleared because otherwise we think there may be some kind of leak.PG_waitersis not one of those flags and leaving the clearing toPAGE_FLAGS_CHECK_AT_PREPis more appropriate.Actually TBH I'm not very keen to this change, it seems the clearing is just moved around and the allocation side pays for that instead of free side.
I'll assume you are referring to the overhead from clearing PG_waiters. First of all, there is no overhead -- we should have a serious talk with the hardware team who makes word-size bitwise AND more than one instruction. And the clearing is done in free_pages_prepare(), which has nothing to do with allocations.
Re: [PATCH v2 3/3] mm: remove superfluous __ClearPageWaiters()
Quoted 16 lines · Yu Zhao
Quoted 5 lines · Yang Shi
Quoted 4 lines · Yu Zhao
Presumably
__ClearPageWaiters()was added to follow the previously removed__ClearPageActive()pattern.Quoted 8 lines
Quoted 4 lines
Only flags that are in
PAGE_FLAGS_CHECK_AT_FREEneeds to be properly cleared because otherwise we think there may be some kind of leak.PG_waitersis not one of those flags and leaving the clearing toPAGE_FLAGS_CHECK_AT_PREPis more appropriate.Actually TBH I'm not very keen to this change, it seems the clearing is just moved around and the allocation side pays for that instead of free side.
Quoted 5 lines
I'll assume you are referring to the overhead from clearing
PG_waiters. First of all, there is no overhead -- we should have a serious talk with the hardware team who makes word-size bitwise AND more than one instruction. And the clearing is done infree_pages_prepare(), which has nothing to do with allocations.
Oh, yes, you are right. Now I'm wondering why we have the waiter bit cleared at the first place.
Michal Hocko
reviewerRe: [PATCH v2 3/3] mm: remove superfluous __ClearPageWaiters()
In reply to Yu ZhaoQuoted 2 lines · Yu Zhao
Presumably
__ClearPageWaiters()was added to follow the previously removed__ClearPageActive()pattern.
I do not think so. Please have a look at 62906027091f ("mm: add PageWaiters indicating tasks are waiting for a page bit") and a discussion when the patch has been proposed. Sorry I do not have a link handy but I do remember that the handling was quite subtle.
Quoted 4 lines
Only flags that are in
PAGE_FLAGS_CHECK_AT_FREEneeds to be properly cleared because otherwise we think there may be some kind of leak.PG_waitersis not one of those flags and leaving the clearing toPAGE_FLAGS_CHECK_AT_PREPis more appropriate.
What is the point of this patch in the first place? Page waiters is quite subtle and I wouldn't touch it without having a very good reason.
Quoted 71 lines
Signed-off-by Yu Zhao <[email protected]>--- include/linux/page-flags.h | 2 +- mm/filemap.c | 2 ++ mm/memremap.c | 2 -- mm/swap.c | 3 --- 4 files changed, 3 insertions(+), 6 deletions(-)include/linux/page-flags.h +1
-1@@ -318,7 +318,7 @@ static inline int TestClearPage##uname(struct page *page) { return 0; } TESTSETFLAG_FALSE(uname) TESTCLEARFLAG_FALSE(uname) __PAGEFLAG(Locked, locked, PF_NO_TAIL) -PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) __CLEARPAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) +PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) PAGEFLAG(Error, error, PF_NO_TAIL) TESTCLEARFLAG(Error, error, PF_NO_TAIL) PAGEFLAG(Referenced, referenced, PF_HEAD) TESTCLEARFLAG(Referenced, referenced, PF_HEAD)mm/filemap.c +2
-0@@ -1079,6 +1079,8 @@ static void wake_up_page_bit(struct page *page, int bit_nr)* other pages on it. * * That's okay, it's a rare case. The next waker will clear it. +* Otherwise the bit will be cleared by PAGE_FLAGS_CHECK_AT_PREP +* when the page is being freed. */ }spin_unlock_irqrestore (&q->lock, flags);mm/memremap.c +0
-2@@ -451,8 +451,6 @@ void free_devmap_managed_page(struct page *page)return ; } -__ClearPageWaiters (page); -mem_cgroup_uncharge (page);/* mm/swap.c +0
-3@@ -90,7 +90,6 @@ static void __page_cache_release(struct page *page)del_page_from_lru_list (page, lruvec,page_off_lru (page));spin_unlock_irqrestore (&pgdat->lru_lock, flags); } -__ClearPageWaiters (page); }static void __put_single_page (struct page *page)@@ -900,8 +899,6 @@ void release_pages(struct page **pages, int nr)del_page_from_lru_list (page, lruvec,page_off_lru (page)); } -__ClearPageWaiters (page); -list_add (&page->lru, &pages_to_free); }if (locked_pgdat)Signature · 1 line
2.28.0.220.ged08abb693-goog
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH v2 3/3] mm: remove superfluous __ClearPageWaiters()
Quoted 9 lines · Michal Hocko
Quoted 2 lines · Yu Zhao
Presumably
__ClearPageWaiters()was added to follow the previously removed__ClearPageActive()pattern.I do not think so. Please have a look at
62906027091f("mm: add PageWaiters indicating tasks are waiting for a page bit") and a discussion when the patch has been proposed. Sorry I do not have a link handy but I do remember that the handling was quite subtle.
Quoted 7 lines
Quoted 4 lines
Only flags that are in
PAGE_FLAGS_CHECK_AT_FREEneeds to be properly cleared because otherwise we think there may be some kind of leak.PG_waitersis not one of those flags and leaving the clearing toPAGE_FLAGS_CHECK_AT_PREPis more appropriate.What is the point of this patch in the first place? Page waiters is quite subtle and I wouldn't touch it without having a very good reason.
I appreciate your caution. And I just studied the history [1] (I admit this is something I should have done beforehand), and didn't find any discussion on __ClearPageWaiters() specifically. So I would ask why it was added originally. I was hoping Nicholas could help us.
[1] https://lore.kernel.org/lkml/[email protected]/
Given its triviality, I can't argue how useful this patch is. So I'll go with how evident it is: we are removing __ClearPageWaiters() from paths where pages have no references left -- they can't have any waiters or be on any wait queues.
Quoted 9 lines · Yu Zhao
We don't initially add anon pages to active lruvec after
commitb518154e59aa("mm/vmscan: protect the workingset on anonymous LRU"). Removeactivate_page()fromunuse_pte(), which seems to be missed by the commit. And make the function static while we are at it.Before the commit, we called
lru_cache_add_active_or_unevictable()to add new ksm pages to active lruvec. Therefore,activate_page()wasn't necessary for them in the first place.
Quoted 62 lines
Signed-off-by Yu Zhao <[email protected]>--- include/linux/swap.h | 1 - mm/swap.c | 4 ++-- mm/swapfile.c | 5 ----- 3 files changed, 2 insertions(+), 8 deletions(-)include/linux/swap.h +0
-1@@ -340,7 +340,6 @@ extern void lru_note_cost_page(struct page *);extern void lru_cache_add (struct page *);extern void lru_add_page_tail (struct page *page,struct page *page_tail,struct lruvec *lruvec,struct list_head *head); -extern void activate_page (struct page *);extern void mark_page_accessed (struct page *);extern void lru_add_drain (void );extern void lru_add_drain_cpu (int cpu);mm/swap.c +2
-2@@ -348,7 +348,7 @@ static bool need_activate_page_drain(int cpu)return pagevec_count (&per_cpu (lru_pvecs.activate_page, cpu)) !=0 ; } -void activate_page (struct page *page) +static void activate_page (struct page *page) { page =compound_head (page);if (PageLRU (page) && !PageActive (page) && !PageUnevictable (page)) {@@ -368,7 +368,7 @@ static inline void activate_page_drain(int cpu) { } -void activate_page (struct page *page) +static void activate_page (struct page *page) {pg_data_t *pgdat =page_pgdat (page);mm/swapfile.c +0
-6@@ -1925,11 +1925,6 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,lru_cache_add_inactive_or_unevictable (page, vma); }swap_free (entry); -/* -* Move the page to the active list so it is not -* immediately swapped out again after swapon. -*/ -activate_page (page); out:pte_unmap_unlock (pte, ptl);if (page != swapcache) { --2.28.0.220.ged08abb693-goog