| Message ID | 20200818184704.3625199-3-yuzhao@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] mm: remove activate_page() from unuse_pte() | expand |
On Tue, Aug 18, 2020 at 11:47 AM Yu Zhao <yuzhao@google.com> 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 <yuzhao@google.com> > --- > 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 <yuzhao@google.com> 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 <yuzhao@google.com> 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 <yuzhao@google.com> 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 <yuzhao@google.com> > --- > 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 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/20161225030030.23219-3-npiggin@gmail.com/ 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.
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)
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 <yuzhao@google.com> --- 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(-)