[PATCH mmotm] mm: delete __ClearPageWaiters()

[PATCH mmotm] mm: delete __ClearPageWaiters()

[Hugh Dickins]

The PG_waiters bit is not included in PAGE_FLAGS_CHECK_AT_FREE, and
vmscan.c's free_unref_page_list() callers rely on that not to generate
bad_page() alerts.  So __page_cache_release() and release_pages() (and
the presumably copy-and-pasted put_zone_device_private_or_public_page())
are redundant and misleading to make a special point of clearing it (as
the "__" implies, it could only safely be used on the freeing path).

Delete __ClearPageWaiters().  Remark on this in one of the "possible"
comments in wake_up_page_bit(), and delete the superfluous comments.

Signed-off-by: Hugh Dickins <hughd@google.com>
---
We've used this since 2018, and I see Yu Zhao posted similar in 2020:
https://lore.kernel.org/linux-mm/20200818184704.3625199-3-yuzhao@google.com/
I couldn't join in at that time, but think its reception was over-cautious.

 include/linux/page-flags.h |  2 +-
 mm/filemap.c               | 22 +++++++---------------
 mm/memremap.c              |  2 --
 mm/swap.c                  |  4 ----
 4 files changed, 8 insertions(+), 22 deletions(-)

--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -481,7 +481,7 @@ static unsigned long *folio_flags(struct folio *folio, unsigned n)
 	TESTSETFLAG_FALSE(uname, lname) TESTCLEARFLAG_FALSE(uname, lname)
 
 __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)
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1179,24 +1179,16 @@ static void folio_wake_bit(struct folio *folio, int bit_nr)
 	}
 
 	/*
-	 * It is possible for other pages to have collided on the waitqueue
-	 * hash, so in that case check for a page match. That prevents a long-
-	 * term waiter
+	 * It's possible to miss clearing waiters here, when we woke our page
+	 * waiters, but the hashed waitqueue has waiters for other pages on it.
 	 *
-	 * It is still possible to miss a case here, when we woke page waiters
-	 * and removed them from the waitqueue, but there are still other
-	 * page waiters.
+	 * That's okay, it's a rare case. The next waker will clear it. Or,
+	 * it might be left set until the page is freed: when it's masked off
+	 * with others in PAGE_FLAGS_CHECK_AT_PREP, by free_pages_prepare().
 	 */
-	if (!waitqueue_active(q) || !key.page_match) {
+	if (!waitqueue_active(q) || !key.page_match)
 		folio_clear_waiters(folio);
-		/*
-		 * It's possible to miss clearing Waiters here, when we woke
-		 * our page waiters, but the hashed waitqueue has waiters for
-		 * other pages on it.
-		 *
-		 * That's okay, it's a rare case. The next waker will clear it.
-		 */
-	}
+
 	spin_unlock_irqrestore(&q->lock, flags);
 }
 
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -487,8 +487,6 @@ void free_zone_device_page(struct page *page)
 	if (WARN_ON_ONCE(!page->pgmap->ops || !page->pgmap->ops->page_free))
 		return;
 
-	__ClearPageWaiters(page);
-
 	mem_cgroup_uncharge(page_folio(page));
 
 	/*
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -97,7 +97,6 @@ static void __page_cache_release(struct page *page)
 		mod_zone_page_state(page_zone(page), NR_MLOCK, -nr_pages);
 		count_vm_events(UNEVICTABLE_PGCLEARED, nr_pages);
 	}
-	__ClearPageWaiters(page);
 }
 
 static void __put_single_page(struct page *page)
@@ -152,7 +151,6 @@ void put_pages_list(struct list_head *pages)
 			continue;
 		}
 		/* Cannot be PageLRU because it's passed to us using the lru */
-		__ClearPageWaiters(page);
 	}
 
 	free_unref_page_list(pages);
@@ -966,8 +964,6 @@ void release_pages(struct page **pages, int nr)
 			count_vm_event(UNEVICTABLE_PGCLEARED);
 		}
 
-		__ClearPageWaiters(page);
-
 		list_add(&page->lru, &pages_to_free);
 	}
 	if (lruvec)

Re: [PATCH mmotm] mm: delete __ClearPageWaiters()

[Yu Zhao]

On Wed, Mar 2, 2022 at 6:56 PM Hugh Dickins <hughd@google.com> wrote:
>
> The PG_waiters bit is not included in PAGE_FLAGS_CHECK_AT_FREE, and
> vmscan.c's free_unref_page_list() callers rely on that not to generate
> bad_page() alerts.  So __page_cache_release() and release_pages() (and
> the presumably copy-and-pasted put_zone_device_private_or_public_page())
> are redundant and misleading to make a special point of clearing it (as
> the "__" implies, it could only safely be used on the freeing path).
>
> Delete __ClearPageWaiters().  Remark on this in one of the "possible"
> comments in wake_up_page_bit(), and delete the superfluous comments.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> We've used this since 2018, and I see Yu Zhao posted similar in 2020:
> https://lore.kernel.org/linux-mm/20200818184704.3625199-3-yuzhao@google.com/
> I couldn't join in at that time, but think its reception was over-cautious.

Indeed.

Tested-by: Yu Zhao <yuzhao@google.com>

Re: [PATCH mmotm] mm: delete __ClearPageWaiters()

[David Hildenbrand]

On 03.03.22 02:56, Hugh Dickins wrote:
> The PG_waiters bit is not included in PAGE_FLAGS_CHECK_AT_FREE, and
> vmscan.c's free_unref_page_list() callers rely on that not to generate
> bad_page() alerts.  So __page_cache_release() and release_pages() (and
> the presumably copy-and-pasted put_zone_device_private_or_public_page())
> are redundant and misleading to make a special point of clearing it (as
> the "__" implies, it could only safely be used on the freeing path).
> 
> Delete __ClearPageWaiters().  Remark on this in one of the "possible"
> comments in wake_up_page_bit(), and delete the superfluous comments.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> We've used this since 2018, and I see Yu Zhao posted similar in 2020:
> https://lore.kernel.org/linux-mm/20200818184704.3625199-3-yuzhao@google.com/
> I couldn't join in at that time, but think its reception was over-cautious.
> 
>  include/linux/page-flags.h |  2 +-
>  mm/filemap.c               | 22 +++++++---------------
>  mm/memremap.c              |  2 --
>  mm/swap.c                  |  4 ----
>  4 files changed, 8 insertions(+), 22 deletions(-)
> 
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -481,7 +481,7 @@ static unsigned long *folio_flags(struct folio *folio, unsigned n)
>  	TESTSETFLAG_FALSE(uname, lname) TESTCLEARFLAG_FALSE(uname, lname)
>  
>  __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)
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1179,24 +1179,16 @@ static void folio_wake_bit(struct folio *folio, int bit_nr)
>  	}
>  
>  	/*
> -	 * It is possible for other pages to have collided on the waitqueue
> -	 * hash, so in that case check for a page match. That prevents a long-
> -	 * term waiter
> +	 * It's possible to miss clearing waiters here, when we woke our page
> +	 * waiters, but the hashed waitqueue has waiters for other pages on it.
>  	 *
> -	 * It is still possible to miss a case here, when we woke page waiters
> -	 * and removed them from the waitqueue, but there are still other
> -	 * page waiters.
> +	 * That's okay, it's a rare case. The next waker will clear it. Or,
> +	 * it might be left set until the page is freed: when it's masked off
> +	 * with others in PAGE_FLAGS_CHECK_AT_PREP, by free_pages_prepare().
>  	 */

Does that also apply to ZONE_DEVICE pages via free_zone_device_page()?


-- 
Thanks,

David / dhildenb

Re: [PATCH mmotm] mm: delete __ClearPageWaiters()

[Hugh Dickins]

On Thu, 3 Mar 2022, David Hildenbrand wrote:
> On 03.03.22 02:56, Hugh Dickins wrote:
> > The PG_waiters bit is not included in PAGE_FLAGS_CHECK_AT_FREE, and
> > vmscan.c's free_unref_page_list() callers rely on that not to generate
> > bad_page() alerts.  So __page_cache_release() and release_pages() (and
> > the presumably copy-and-pasted put_zone_device_private_or_public_page())

Hah, I'm showing my age there, or the patch's age: it's been rebranded
frequently since then, with linux-next calling it free_zone_device_page(),
as you kindly point out.  How long before it's free_zone_device_folio()?

> > are redundant and misleading to make a special point of clearing it (as
> > the "__" implies, it could only safely be used on the freeing path).
> > 
> > Delete __ClearPageWaiters().  Remark on this in one of the "possible"
> > comments in wake_up_page_bit(), and delete the superfluous comments.
> > 
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > ---
> > We've used this since 2018, and I see Yu Zhao posted similar in 2020:
> > https://lore.kernel.org/linux-mm/20200818184704.3625199-3-yuzhao@google.com/
> > I couldn't join in at that time, but think its reception was over-cautious.
> > 
> >  include/linux/page-flags.h |  2 +-
> >  mm/filemap.c               | 22 +++++++---------------
> >  mm/memremap.c              |  2 --
> >  mm/swap.c                  |  4 ----
> >  4 files changed, 8 insertions(+), 22 deletions(-)
> > 
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1179,24 +1179,16 @@ static void folio_wake_bit(struct folio *folio, int bit_nr)
> >  	}
> >  
> >  	/*
> > -	 * It is possible for other pages to have collided on the waitqueue
> > -	 * hash, so in that case check for a page match. That prevents a long-
> > -	 * term waiter
> > +	 * It's possible to miss clearing waiters here, when we woke our page
> > +	 * waiters, but the hashed waitqueue has waiters for other pages on it.
> >  	 *
> > -	 * It is still possible to miss a case here, when we woke page waiters
> > -	 * and removed them from the waitqueue, but there are still other
> > -	 * page waiters.
> > +	 * That's okay, it's a rare case. The next waker will clear it. Or,
> > +	 * it might be left set until the page is freed: when it's masked off
> > +	 * with others in PAGE_FLAGS_CHECK_AT_PREP, by free_pages_prepare().
> >  	 */
> 
> Does that also apply to ZONE_DEVICE pages via free_zone_device_page()?

I'm sure you could tell me a lot more about ZONE_DEVICE pages than I
could ever tell you.  But, if they don't ever reach the main page freer,
then they're in the same category as other pages not freed until reboot:
any clearing of left-behind PG_waiters will be done by the next waker,
not by reaching free_pages_prepare().  Does that really require special
mention of ZONE_DEVICE pages here?  Would I do better just to remove
the comment on PAGE_FLAGS_CHECK_AT_PREP being one of the clearers?

(I had to do a bit of research before answering: temporarily confused
about the role of PG_waiters, I worried that removing copy-and-pasted
__ClearPageWaiters from free_zone_device_page() might risk gradually
clogging up the hash queues with spuriously waited pages; no, nonsense,
it's just a matter of how efficient the next folio_unlock() will be.)

Hugh

Re: [PATCH mmotm] mm: delete __ClearPageWaiters()

[Matthew Wilcox]

On Thu, Mar 03, 2022 at 02:28:46PM -0800, Hugh Dickins wrote:
> On Thu, 3 Mar 2022, David Hildenbrand wrote:
> > On 03.03.22 02:56, Hugh Dickins wrote:
> > > The PG_waiters bit is not included in PAGE_FLAGS_CHECK_AT_FREE, and
> > > vmscan.c's free_unref_page_list() callers rely on that not to generate
> > > bad_page() alerts.  So __page_cache_release() and release_pages() (and
> > > the presumably copy-and-pasted put_zone_device_private_or_public_page())
> 
> Hah, I'm showing my age there, or the patch's age: it's been rebranded
> frequently since then, with linux-next calling it free_zone_device_page(),
> as you kindly point out.  How long before it's free_zone_device_folio()?

Probably not a serious question, but within the next year, I expect.
I have a prototype patch to do the entire page freeing path, but it
wasn't a priority for this merge window.

Re: [PATCH mmotm] mm: delete __ClearPageWaiters()

[David Hildenbrand]

On 03.03.22 23:28, Hugh Dickins wrote:
> On Thu, 3 Mar 2022, David Hildenbrand wrote:
>> On 03.03.22 02:56, Hugh Dickins wrote:
>>> The PG_waiters bit is not included in PAGE_FLAGS_CHECK_AT_FREE, and
>>> vmscan.c's free_unref_page_list() callers rely on that not to generate
>>> bad_page() alerts.  So __page_cache_release() and release_pages() (and
>>> the presumably copy-and-pasted put_zone_device_private_or_public_page())
> 
> Hah, I'm showing my age there, or the patch's age: it's been rebranded
> frequently since then, with linux-next calling it free_zone_device_page(),
> as you kindly point out.  How long before it's free_zone_device_folio()?

:)

> 
>>> are redundant and misleading to make a special point of clearing it (as
>>> the "__" implies, it could only safely be used on the freeing path).
>>>
>>> Delete __ClearPageWaiters().  Remark on this in one of the "possible"
>>> comments in wake_up_page_bit(), and delete the superfluous comments.
>>>
>>> Signed-off-by: Hugh Dickins <hughd@google.com>
>>> ---
>>> We've used this since 2018, and I see Yu Zhao posted similar in 2020:
>>> https://lore.kernel.org/linux-mm/20200818184704.3625199-3-yuzhao@google.com/
>>> I couldn't join in at that time, but think its reception was over-cautious.
>>>
>>>  include/linux/page-flags.h |  2 +-
>>>  mm/filemap.c               | 22 +++++++---------------
>>>  mm/memremap.c              |  2 --
>>>  mm/swap.c                  |  4 ----
>>>  4 files changed, 8 insertions(+), 22 deletions(-)
>>>
>>> --- a/mm/filemap.c
>>> +++ b/mm/filemap.c
>>> @@ -1179,24 +1179,16 @@ static void folio_wake_bit(struct folio *folio, int bit_nr)
>>>  	}
>>>  
>>>  	/*
>>> -	 * It is possible for other pages to have collided on the waitqueue
>>> -	 * hash, so in that case check for a page match. That prevents a long-
>>> -	 * term waiter
>>> +	 * It's possible to miss clearing waiters here, when we woke our page
>>> +	 * waiters, but the hashed waitqueue has waiters for other pages on it.
>>>  	 *
>>> -	 * It is still possible to miss a case here, when we woke page waiters
>>> -	 * and removed them from the waitqueue, but there are still other
>>> -	 * page waiters.
>>> +	 * That's okay, it's a rare case. The next waker will clear it. Or,
>>> +	 * it might be left set until the page is freed: when it's masked off
>>> +	 * with others in PAGE_FLAGS_CHECK_AT_PREP, by free_pages_prepare().
>>>  	 */
>>
>> Does that also apply to ZONE_DEVICE pages via free_zone_device_page()?
> 
> I'm sure you could tell me a lot more about ZONE_DEVICE pages than I
> could ever tell you.  But, if they don't ever reach the main page freer,
> then they're in the same category as other pages not freed until reboot:
> any clearing of left-behind PG_waiters will be done by the next waker,
> not by reaching free_pages_prepare().  Does that really require special
> mention of ZONE_DEVICE pages here?  Would I do better just to remove
> the comment on PAGE_FLAGS_CHECK_AT_PREP being one of the clearers?

In this context we can consider ZONE_DEVICE pages just like any other
pages that, although getting freed, are not returned to the buddy, but
instead are returned to another pool. So PAGE_FLAGS_CHECK_AT_PREP won't
apply and free_pages_prepare() won't apply.

Another example would be hugetlb pages, that are returned to the hugetlb
pool, but not back to the buddy unless the huge page pool is shrunk.


So I feel like the underlying principle here is: we don't *care* if
PG_waiter is cleared when a page gets freed, because it will simply get
cleared by the next waker if it sticks around.

Then, I agree, we can just drop the comment regarding
PAGE_FLAGS_CHECK_AT_PREP and instead have something like


"
That's okay, it's a rare case and the next waker will just clear it.
Note that, depending on the page pool (buddy, ZONE_DEVICE, hugetlb), we
might clear the flag while freeing the page, however, this is not
required for correctness.
"


-- 
Thanks,

David / dhildenb

Re: [PATCH mmotm] mm: delete __ClearPageWaiters()

[Hugh Dickins]

On Fri, 4 Mar 2022, David Hildenbrand wrote:
> 
> In this context we can consider ZONE_DEVICE pages just like any other
> pages that, although getting freed, are not returned to the buddy, but
> instead are returned to another pool. So PAGE_FLAGS_CHECK_AT_PREP won't
> apply and free_pages_prepare() won't apply.
> 
> Another example would be hugetlb pages, that are returned to the hugetlb
> pool, but not back to the buddy unless the huge page pool is shrunk.
> 
> 
> So I feel like the underlying principle here is: we don't *care* if
> PG_waiter is cleared when a page gets freed, because it will simply get
> cleared by the next waker if it sticks around.

I think we were focused on different issues here.  I was focused on
how it was redundant for those places to clear the bit, because it
was going to get cleared anyway just after (in the buddy case).
Whereas you are focused on how it doesn't matter at all whether
it gets cleared when freeing.  Both valid points.

> 
> Then, I agree, we can just drop the comment regarding
> PAGE_FLAGS_CHECK_AT_PREP and instead have something like

Okay, the reference to PAGE_FLAGS_CHECK_AT_PREP in the commit message is
good enough for me, no need to make a point of it in the code comment.

> 
> 
> "
> That's okay, it's a rare case and the next waker will just clear it.
> Note that, depending on the page pool (buddy, ZONE_DEVICE, hugetlb), we
> might clear the flag while freeing the page, however, this is not
> required for correctness.
> "

Okay, v2 coming up: I've taken largely your wording (but not exactly).

Thanks,
Hugh

[PATCH mmotm v2] mm: delete __ClearPageWaiters()

[Hugh Dickins]

The PG_waiters bit is not included in PAGE_FLAGS_CHECK_AT_FREE, and
vmscan.c's free_unref_page_list() callers rely on that not to generate
bad_page() alerts.  So __page_cache_release(), put_pages_list() and
release_pages() (and presumably copy-and-pasted free_zone_device_page())
are redundant and misleading to make a special point of clearing it (as
the "__" implies, it could only safely be used on the freeing path).

Delete __ClearPageWaiters().  Remark on this in one of the "possible"
comments in folio_wake_bit(), and delete the superfluous comments.

Signed-off-by: Hugh Dickins <hughd@google.com>
Tested-by: Yu Zhao <yuzhao@google.com>
---
v2: Add Tested-by from Yu Zhao.
    Update to latest function names in the commit message.
    Reword comment in folio_wake_bit() per David Hildenbrand.

 include/linux/page-flags.h |  2 +-
 mm/filemap.c               | 23 ++++++++---------------
 mm/memremap.c              |  2 --
 mm/swap.c                  |  4 ----
 4 files changed, 9 insertions(+), 22 deletions(-)

--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -481,7 +481,7 @@ static unsigned long *folio_flags(struct folio *folio, unsigned n)
 	TESTSETFLAG_FALSE(uname, lname) TESTCLEARFLAG_FALSE(uname, lname)
 
 __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)
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1179,24 +1179,17 @@ static void folio_wake_bit(struct folio *folio, int bit_nr)
 	}
 
 	/*
-	 * It is possible for other pages to have collided on the waitqueue
-	 * hash, so in that case check for a page match. That prevents a long-
-	 * term waiter
+	 * It's possible to miss clearing waiters here, when we woke our page
+	 * waiters, but the hashed waitqueue has waiters for other pages on it.
+	 * That's okay, it's a rare case. The next waker will clear it.
 	 *
-	 * It is still possible to miss a case here, when we woke page waiters
-	 * and removed them from the waitqueue, but there are still other
-	 * page waiters.
+	 * Note that, depending on the page pool (buddy, hugetlb, ZONE_DEVICE,
+	 * other), the flag may be cleared in the course of freeing the page;
+	 * but that is not required for correctness.
 	 */
-	if (!waitqueue_active(q) || !key.page_match) {
+	if (!waitqueue_active(q) || !key.page_match)
 		folio_clear_waiters(folio);
-		/*
-		 * It's possible to miss clearing Waiters here, when we woke
-		 * our page waiters, but the hashed waitqueue has waiters for
-		 * other pages on it.
-		 *
-		 * That's okay, it's a rare case. The next waker will clear it.
-		 */
-	}
+
 	spin_unlock_irqrestore(&q->lock, flags);
 }
 
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -487,8 +487,6 @@ void free_zone_device_page(struct page *page)
 	if (WARN_ON_ONCE(!page->pgmap->ops || !page->pgmap->ops->page_free))
 		return;
 
-	__ClearPageWaiters(page);
-
 	mem_cgroup_uncharge(page_folio(page));
 
 	/*
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -97,7 +97,6 @@ static void __page_cache_release(struct page *page)
 		mod_zone_page_state(page_zone(page), NR_MLOCK, -nr_pages);
 		count_vm_events(UNEVICTABLE_PGCLEARED, nr_pages);
 	}
-	__ClearPageWaiters(page);
 }
 
 static void __put_single_page(struct page *page)
@@ -152,7 +151,6 @@ void put_pages_list(struct list_head *pages)
 			continue;
 		}
 		/* Cannot be PageLRU because it's passed to us using the lru */
-		__ClearPageWaiters(page);
 	}
 
 	free_unref_page_list(pages);
@@ -966,8 +964,6 @@ void release_pages(struct page **pages, int nr)
 			count_vm_event(UNEVICTABLE_PGCLEARED);
 		}
 
-		__ClearPageWaiters(page);
-
 		list_add(&page->lru, &pages_to_free);
 	}
 	if (lruvec)

Re: [PATCH mmotm v2] mm: delete __ClearPageWaiters()

[David Hildenbrand]

On 04.03.22 22:25, Hugh Dickins wrote:
> The PG_waiters bit is not included in PAGE_FLAGS_CHECK_AT_FREE, and
> vmscan.c's free_unref_page_list() callers rely on that not to generate
> bad_page() alerts.  So __page_cache_release(), put_pages_list() and
> release_pages() (and presumably copy-and-pasted free_zone_device_page())
> are redundant and misleading to make a special point of clearing it (as
> the "__" implies, it could only safely be used on the freeing path).
> 
> Delete __ClearPageWaiters().  Remark on this in one of the "possible"
> comments in folio_wake_bit(), and delete the superfluous comments.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Tested-by: Yu Zhao <yuzhao@google.com>

Thanks

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH mmotm v2] mm: delete __ClearPageWaiters()

[Yang Shi]

On Fri, Mar 4, 2022 at 1:25 PM Hugh Dickins <hughd@google.com> wrote:
>
> The PG_waiters bit is not included in PAGE_FLAGS_CHECK_AT_FREE, and
> vmscan.c's free_unref_page_list() callers rely on that not to generate
> bad_page() alerts.  So __page_cache_release(), put_pages_list() and
> release_pages() (and presumably copy-and-pasted free_zone_device_page())
> are redundant and misleading to make a special point of clearing it (as
> the "__" implies, it could only safely be used on the freeing path).
>
> Delete __ClearPageWaiters().  Remark on this in one of the "possible"
> comments in folio_wake_bit(), and delete the superfluous comments.
>
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Tested-by: Yu Zhao <yuzhao@google.com>
> ---
> v2: Add Tested-by from Yu Zhao.
>     Update to latest function names in the commit message.
>     Reword comment in folio_wake_bit() per David Hildenbrand.

Reviewed-by: Yang Shi <shy828301@gmail.com>

>
>  include/linux/page-flags.h |  2 +-
>  mm/filemap.c               | 23 ++++++++---------------
>  mm/memremap.c              |  2 --
>  mm/swap.c                  |  4 ----
>  4 files changed, 9 insertions(+), 22 deletions(-)
>
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -481,7 +481,7 @@ static unsigned long *folio_flags(struct folio *folio, unsigned n)
>         TESTSETFLAG_FALSE(uname, lname) TESTCLEARFLAG_FALSE(uname, lname)
>
>  __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)
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1179,24 +1179,17 @@ static void folio_wake_bit(struct folio *folio, int bit_nr)
>         }
>
>         /*
> -        * It is possible for other pages to have collided on the waitqueue
> -        * hash, so in that case check for a page match. That prevents a long-
> -        * term waiter
> +        * It's possible to miss clearing waiters here, when we woke our page
> +        * waiters, but the hashed waitqueue has waiters for other pages on it.
> +        * That's okay, it's a rare case. The next waker will clear it.
>          *
> -        * It is still possible to miss a case here, when we woke page waiters
> -        * and removed them from the waitqueue, but there are still other
> -        * page waiters.
> +        * Note that, depending on the page pool (buddy, hugetlb, ZONE_DEVICE,
> +        * other), the flag may be cleared in the course of freeing the page;
> +        * but that is not required for correctness.
>          */
> -       if (!waitqueue_active(q) || !key.page_match) {
> +       if (!waitqueue_active(q) || !key.page_match)
>                 folio_clear_waiters(folio);
> -               /*
> -                * It's possible to miss clearing Waiters here, when we woke
> -                * our page waiters, but the hashed waitqueue has waiters for
> -                * other pages on it.
> -                *
> -                * That's okay, it's a rare case. The next waker will clear it.
> -                */
> -       }
> +
>         spin_unlock_irqrestore(&q->lock, flags);
>  }
>
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -487,8 +487,6 @@ void free_zone_device_page(struct page *page)
>         if (WARN_ON_ONCE(!page->pgmap->ops || !page->pgmap->ops->page_free))
>                 return;
>
> -       __ClearPageWaiters(page);
> -
>         mem_cgroup_uncharge(page_folio(page));
>
>         /*
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -97,7 +97,6 @@ static void __page_cache_release(struct page *page)
>                 mod_zone_page_state(page_zone(page), NR_MLOCK, -nr_pages);
>                 count_vm_events(UNEVICTABLE_PGCLEARED, nr_pages);
>         }
> -       __ClearPageWaiters(page);
>  }
>
>  static void __put_single_page(struct page *page)
> @@ -152,7 +151,6 @@ void put_pages_list(struct list_head *pages)
>                         continue;
>                 }
>                 /* Cannot be PageLRU because it's passed to us using the lru */
> -               __ClearPageWaiters(page);
>         }
>
>         free_unref_page_list(pages);
> @@ -966,8 +964,6 @@ void release_pages(struct page **pages, int nr)
>                         count_vm_event(UNEVICTABLE_PGCLEARED);
>                 }
>
> -               __ClearPageWaiters(page);
> -
>                 list_add(&page->lru, &pages_to_free);
>         }
>         if (lruvec)