Re: [PATCH v3 2/4] mm: drop stale folio_ref_count()==1 check in do_swap_page reuse logic

Re: [PATCH v3 2/4] mm: drop stale folio_ref_count()==1 check in do_swap_page reuse logic

[Barry Song]

On Thu, Jul 2, 2026 at 4:14 PM David Hildenbrand (Arm) <david@kernel.org> wrote:
>
> On 7/2/26 01:59, Barry Song (Xiaomi) wrote:
> > The "we just allocated them without exposing them to the swapcache"
> > case no longer exists, as Kairui has routed synchronous I/O through
> > the swapcache as well in his series "unify swapin use swap cache and
> > cleanup flags"[1]. As a result, folio_ref_count() should never be 1
> > in this path, since at least two references are held (base ref plus
> > swapcache). Remove the folio_ref_count()==1 check and update the
> > comment accordingly.
>
> Sashiko points out two minor things (one flagged as medium, lol, sure sure).
>
> Here, you can clarify that folio_ref_count()==1 is true for freshly allocated
> pages (due to the KSM check) in which case exclusive=true already.

Thanks. Sashiko's comment below seems valid. While there's no
need to change the code, the changelog should be updated to
reflect it. I'll do that in v4.

"Is the claim that folio_ref_count() should never be 1 in this path accurate?
When ksm_might_need_to_copy() allocates a fresh folio to break KSM sharing,
the new folio bypasses the swap cache:
do_swap_page() {
 ...
 if (!folio_test_ksm(folio)) {
   ...
 if (folio != swapcache) {
    /*
     * We have a fresh page that is not exposed to the
     * swapcache -> certainly exclusive.
     */
  exclusive = true;
  }
  ...
}
In this specific case, the folio refcount would still be exactly 1 when it
reaches this path.
While the code change itself is safe because the KSM copy path explicitly
sets exclusive = true earlier (satisfying the new if (exclusive) check
without needing the refcount check), the rationale in the commit message
seems to overlook this scenario."

>
> >
> > [1] https://lore.kernel.org/all/20251220-swap-table-p2-v5-0-8862a265a033@tencent.com/
> >
> > Acked-by: Usama Arif <usama.arif@linux.dev>
> > Reviewed-by: Kairui Song <kasong@tencent.com>
> > Reviewed-by: Baoquan He <baoquan.he@linux.dev>
> > Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> > Signed-off-by: Barry Song (Xiaomi) <baohua@kernel.org>
> > ---
> >  mm/memory.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 87da78eb1abd..71e9d394816b 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -5046,13 +5046,10 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> >               pte = pte_mkuffd_wp(pte);
> >
> >       /*
> > -      * Same logic as in do_wp_page(); however, optimize for pages that are
> > -      * certainly not shared either because we just allocated them without
> > -      * exposing them to the swapcache or because the swap entry indicates
> > -      * exclusivity.
> > +      * Similar logic as in do_wp_page(); however, optimize for pages that are
> > +      * certainly not because the swap entry indicates exclusivity.
>
> s/not/not shared/
>
> but likely you should just simplify to "... pages that are certainly exclusive."

Thanks. I will fix the doc in v4.

Best Regards
Barry

Re: [PATCH v3 1/4] mm: avoid unnecessary lru drain for wp_can_reuse_anon_folio()

[Barry Song]

On Thu, Jul 2, 2026 at 4:05 PM David Hildenbrand (Arm) <david@kernel.org> wrote:
>
> On 7/2/26 01:59, Barry Song (Xiaomi) wrote:
> > There is a case where `folio_ref_count(folio) == 3` and
> > `!folio_test_swapcache(folio)`. In that case, both
> > `folio_ref_count(folio) > 3` and
> > `folio_ref_count(folio) > 1 + folio_test_swapcache(folio)` evaluate
> > false, causing an unnecessary local LRU drain.
> >
> > During an Ubuntu boot, I observed over 5,000 redundant local LRU
> > drains. For a kernel build with a minimal configuration, I observed
> > more than 20,000 redundant drains.
> >
> > Fix this by checking against: `1 + in_swapcache + in_lrucache`
> > instead of hardcoding `folio_ref_count(folio) > 3`.
> >
> > Suggested-by: David Hildenbrand (Arm) <david@kernel.org>
> > Reviewed-by: Kairui Song <kasong@tencent.com>
> > Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> > Reviewed-by: Baoquan He <baoquan.he@linux.dev>
> > Signed-off-by: Barry Song (Xiaomi) <baohua@kernel.org>
> > ---
> The other folks should probably re-review this patch that changed quite a bit :)
>
>
> Acked-by: David Hildenbrand (Arm) <david@kernel.org>

Thanks!

Hi Baoquan, Shakeel, and Kairui,

Would you like to re-review the patches so I can re-collect your
tags? If not, are you okay with me removing your tags when I
send v4?

The concept hasn't changed since v2, but the code readability has
improved quite a bit based on David's suggestions.

Best Regards
Barry

Re: [PATCH v3 1/4] mm: avoid unnecessary lru drain for wp_can_reuse_anon_folio()

[Baoquan He]

Hi Barry,

On 07/06/26 at 04:18pm, Barry Song wrote:
> On Thu, Jul 2, 2026 at 4:05 PM David Hildenbrand (Arm) <david@kernel.org> wrote:
> >
> > On 7/2/26 01:59, Barry Song (Xiaomi) wrote:
> > > There is a case where `folio_ref_count(folio) == 3` and
> > > `!folio_test_swapcache(folio)`. In that case, both
> > > `folio_ref_count(folio) > 3` and
> > > `folio_ref_count(folio) > 1 + folio_test_swapcache(folio)` evaluate
> > > false, causing an unnecessary local LRU drain.
> > >
> > > During an Ubuntu boot, I observed over 5,000 redundant local LRU
> > > drains. For a kernel build with a minimal configuration, I observed
> > > more than 20,000 redundant drains.
> > >
> > > Fix this by checking against: `1 + in_swapcache + in_lrucache`
> > > instead of hardcoding `folio_ref_count(folio) > 3`.
> > >
> > > Suggested-by: David Hildenbrand (Arm) <david@kernel.org>
> > > Reviewed-by: Kairui Song <kasong@tencent.com>
> > > Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> > > Reviewed-by: Baoquan He <baoquan.he@linux.dev>
> > > Signed-off-by: Barry Song (Xiaomi) <baohua@kernel.org>
> > > ---
> > The other folks should probably re-review this patch that changed quite a bit :)
> >
> >
> > Acked-by: David Hildenbrand (Arm) <david@kernel.org>
> 
> Thanks!
> 
> Hi Baoquan, Shakeel, and Kairui,
> 
> Would you like to re-review the patches so I can re-collect your
> tags? If not, are you okay with me removing your tags when I
> send v4?

Is there change you need to do in a new version? If yes, please remove
my tags, I will review v4. Oterwise, I will check this v3. Thanks.

AFAIK, usually, if patch is updated in a new version, any reviewing tags
from the old version should be dropped unless the change is minor or
trivial.

Thanks
Baoquan

> 
> The concept hasn't changed since v2, but the code readability has
> improved quite a bit based on David's suggestions.
> 
> Best Regards
> Barry

Re: [PATCH v3 1/4] mm: avoid unnecessary lru drain for wp_can_reuse_anon_folio()

[Barry Song]

On Mon, Jul 6, 2026 at 5:13 PM Baoquan He <baoquan.he@linux.dev> wrote:
>
> Hi Barry,
>
> On 07/06/26 at 04:18pm, Barry Song wrote:
> > On Thu, Jul 2, 2026 at 4:05 PM David Hildenbrand (Arm) <david@kernel.org> wrote:
> > >
> > > On 7/2/26 01:59, Barry Song (Xiaomi) wrote:
> > > > There is a case where `folio_ref_count(folio) == 3` and
> > > > `!folio_test_swapcache(folio)`. In that case, both
> > > > `folio_ref_count(folio) > 3` and
> > > > `folio_ref_count(folio) > 1 + folio_test_swapcache(folio)` evaluate
> > > > false, causing an unnecessary local LRU drain.
> > > >
> > > > During an Ubuntu boot, I observed over 5,000 redundant local LRU
> > > > drains. For a kernel build with a minimal configuration, I observed
> > > > more than 20,000 redundant drains.
> > > >
> > > > Fix this by checking against: `1 + in_swapcache + in_lrucache`
> > > > instead of hardcoding `folio_ref_count(folio) > 3`.
> > > >
> > > > Suggested-by: David Hildenbrand (Arm) <david@kernel.org>
> > > > Reviewed-by: Kairui Song <kasong@tencent.com>
> > > > Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> > > > Reviewed-by: Baoquan He <baoquan.he@linux.dev>
> > > > Signed-off-by: Barry Song (Xiaomi) <baohua@kernel.org>
> > > > ---
> > > The other folks should probably re-review this patch that changed quite a bit :)
> > >
> > >
> > > Acked-by: David Hildenbrand (Arm) <david@kernel.org>
> >
> > Thanks!
> >
> > Hi Baoquan, Shakeel, and Kairui,
> >
> > Would you like to re-review the patches so I can re-collect your
> > tags? If not, are you okay with me removing your tags when I
> > send v4?
>
> Is there change you need to do in a new version? If yes, please remove
> my tags, I will review v4. Oterwise, I will check this v3. Thanks.

no change for v4. Please feel free to re-review v3.

>
> AFAIK, usually, if patch is updated in a new version, any reviewing tags
> from the old version should be dropped unless the change is minor or
> trivial.

Thanks for the clarification. make sense to me.

Best Regards
Barry

Re: [PATCH v3 2/4] mm: drop stale folio_ref_count()==1 check in do_swap_page reuse logic

[Barry Song]

On Thu, Jul 2, 2026 at 4:14 PM David Hildenbrand (Arm) <david@kernel.org> wrote:
>
> On 7/2/26 01:59, Barry Song (Xiaomi) wrote:
> > The "we just allocated them without exposing them to the swapcache"
> > case no longer exists, as Kairui has routed synchronous I/O through
> > the swapcache as well in his series "unify swapin use swap cache and
> > cleanup flags"[1]. As a result, folio_ref_count() should never be 1
> > in this path, since at least two references are held (base ref plus
> > swapcache). Remove the folio_ref_count()==1 check and update the
> > comment accordingly.
>
> Sashiko points out two minor things (one flagged as medium, lol, sure sure).
>
> Here, you can clarify that folio_ref_count()==1 is true for freshly allocated
> pages (due to the KSM check) in which case exclusive=true already.

Hi Andrew,

I saw that you've queued this in mm-new. Thanks very much for that!

Would you mind adding the following sentences to the end of the changelog?

The ksm_might_need_to_copy() check may allocate a fresh folio with
folio_ref_count() == 1. Along that path, exclusive has already been
set to true, so the folio can still be reused correctly.

Best Regards
Barry

Re: [PATCH v3 1/4] mm: avoid unnecessary lru drain for wp_can_reuse_anon_folio()

[Baoquan He]

On 07/02/26 at 07:59am, Barry Song (Xiaomi) wrote:
> There is a case where `folio_ref_count(folio) == 3` and
> `!folio_test_swapcache(folio)`. In that case, both
> `folio_ref_count(folio) > 3` and
> `folio_ref_count(folio) > 1 + folio_test_swapcache(folio)` evaluate
> false, causing an unnecessary local LRU drain.
> 
> During an Ubuntu boot, I observed over 5,000 redundant local LRU
> drains. For a kernel build with a minimal configuration, I observed
> more than 20,000 redundant drains.
> 
> Fix this by checking against: `1 + in_swapcache + in_lrucache`
> instead of hardcoding `folio_ref_count(folio) > 3`.
> 
> Suggested-by: David Hildenbrand (Arm) <david@kernel.org>
> Reviewed-by: Kairui Song <kasong@tencent.com>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> Reviewed-by: Baoquan He <baoquan.he@linux.dev>
> Signed-off-by: Barry Song (Xiaomi) <baohua@kernel.org>
> ---
>  mm/memory.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

Rechecked, LGTM,

Reviewed-by: Baoquan He <baoquan.he@linux.dev>

> 
> diff --git a/mm/memory.c b/mm/memory.c
> index ff338c2abe92..87da78eb1abd 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4181,6 +4181,9 @@ static bool __wp_can_reuse_large_anon_folio(struct folio *folio,
>  static bool wp_can_reuse_anon_folio(struct folio *folio,
>  				    struct vm_area_struct *vma)
>  {
> +	const bool in_lru_cache = !folio_test_lru(folio);
> +	const bool in_swapcache = folio_test_swapcache(folio);
> +
>  	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && folio_test_large(folio))
>  		return __wp_can_reuse_large_anon_folio(folio, vma);
>  
> @@ -4191,15 +4194,16 @@ static bool wp_can_reuse_anon_folio(struct folio *folio,
>  	 *
>  	 * KSM doesn't necessarily raise the folio refcount.
>  	 */
> -	if (folio_test_ksm(folio) || folio_ref_count(folio) > 3)
> +	if (folio_test_ksm(folio) ||
> +	    folio_ref_count(folio) > 1 + in_lru_cache + in_swapcache)
>  		return false;
> -	if (!folio_test_lru(folio))
> +	if (in_lru_cache)
>  		/*
>  		 * We cannot easily detect+handle references from
>  		 * remote LRU caches or references to LRU folios.
>  		 */
>  		lru_add_drain();
> -	if (folio_ref_count(folio) > 1 + folio_test_swapcache(folio))
> +	if (folio_ref_count(folio) > 1 + in_swapcache)
>  		return false;
>  	if (!folio_trylock(folio))
>  		return false;
> -- 
> 2.39.3 (Apple Git-146)
>