Presumably __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.
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(-)
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 6be1aa559b1e..dba80a2bdfba 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -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)
diff --git a/mm/filemap.c b/mm/filemap.c
index 1aaea26556cc..75240c7ef73f 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -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);
diff --git a/mm/memremap.c b/mm/memremap.c
index 3a06eb91cb59..a9d02ffaf9e3 100644
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -451,8 +451,6 @@ void free_devmap_managed_page(struct page *page)
return;
}
- __ClearPageWaiters(page);
-
mem_cgroup_uncharge(page);
/*
diff --git a/mm/swap.c b/mm/swap.c
index 999a84dbe12c..40bf20a75278 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -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
On Tue, Aug 18, 2020 at 11:47 AM Yu Zhao <[email protected]> wrote:
>
> Presumably __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.
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.
>
> 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(-)
>
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 6be1aa559b1e..dba80a2bdfba 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -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)
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1aaea26556cc..75240c7ef73f 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -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);
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 3a06eb91cb59..a9d02ffaf9e3 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -451,8 +451,6 @@ void free_devmap_managed_page(struct page *page)
> return;
> }
>
> - __ClearPageWaiters(page);
> -
> mem_cgroup_uncharge(page);
>
> /*
> diff --git a/mm/swap.c b/mm/swap.c
> index 999a84dbe12c..40bf20a75278 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -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
>
>
On Wed, Aug 19, 2020 at 04:06:32PM -0700, Yang Shi wrote:
> On Tue, Aug 18, 2020 at 11:47 AM Yu Zhao <[email protected]> wrote:
> >
> > Presumably __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.
>
> 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.
On Wed, Aug 19, 2020 at 4:39 PM Yu Zhao <[email protected]> wrote:
>
> On Wed, Aug 19, 2020 at 04:06:32PM -0700, Yang Shi wrote:
> > On Tue, Aug 18, 2020 at 11:47 AM Yu Zhao <[email protected]> wrote:
> > >
> > > Presumably __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.
> >
> > 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.
Oh, yes, you are right. Now I'm wondering why we have the waiter bit
cleared at the first place.
On Tue 18-08-20 12:47:04, Yu Zhao wrote:
> 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.
> 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.
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.
> 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(-)
>
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 6be1aa559b1e..dba80a2bdfba 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -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)
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1aaea26556cc..75240c7ef73f 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -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);
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 3a06eb91cb59..a9d02ffaf9e3 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -451,8 +451,6 @@ void free_devmap_managed_page(struct page *page)
> return;
> }
>
> - __ClearPageWaiters(page);
> -
> mem_cgroup_uncharge(page);
>
> /*
> diff --git a/mm/swap.c b/mm/swap.c
> index 999a84dbe12c..40bf20a75278 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -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
>
--
Michal Hocko
SUSE Labs
On Thu, Aug 20, 2020 at 08:18:27AM +0200, Michal Hocko wrote:
> On Tue 18-08-20 12:47:04, Yu Zhao wrote:
> > 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.
>
> > 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.
>
> 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.