Re: [PATCH v5 1/5] mm/vmscan: introduce folio_activate_locked() helper

[Zhang Peng <zippermonkey@icloud.com>]

On Mon, Aug 10, 2026 at 4:36 PM Barry Song <baohua@kernel.org> wrote:
>
> Reviewed-by: Barry Song <baohua@kernel.org>

Thanks for the review!

> > + * Prepare a locked folio to be kept active rather than reclaimed.
> > + * Reclaims its swap slot if it will not be swapped, then marks it
>
> I'm not quite sure whether this should be "if". Because, it seems
> to always be true up to this point. BTW, if we really want to use
> "if", shouldn't we use it to check whether swap is full?

You're right, the wording was misleading: the condition that actually
guards folio_free_swap() is the swapcache/swap-full/mlocked test right
below it, not anything about whether the folio "will be swapped".

Rather than try to restate that in prose, I dropped the paragraph and
left a single line, since the code below is already explicit:

	/* Activate an isolated, locked folio and account the activation. */
	static void folio_activate_locked(struct folio *folio,
			struct reclaim_stat *stat)

One thing I should flag, since it is not just a comment change: the
VM_BUG_ON_FOLIO(folio_test_active(folio)) that used to sit at the
activate_locked label is now a VM_WARN_ON_ONCE_FOLIO(), so a caller
that gets this wrong is reported rather than taking the machine down.
Both are CONFIG_DEBUG_VM-only, and no non-debug behaviour changes, but
it is a deliberate change rather than a pure move, and the changelog
now says so.

I kept your Reviewed-by on that basis - please let me know if you'd
rather I dropped it, or if you'd prefer the BUG_ON left alone.

This patch is now part of a smaller cleanup-only series, see my reply
on 5/5.

Thanks
Zhang Peng

From nobody Sun Sep 20 00:00:00 2026
From: Zhang Peng <zippermonkey@icloud.com>
To: Barry Song <baohua@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	David Hildenbrand <david@kernel.org>,
	Lorenzo Stoakes <ljs@kernel.org>,
	Vlastimil Babka <vbabka@kernel.org>,
	Mike Rapoport <rppt@kernel.org>,
	Suren Baghdasaryan <surenb@google.com>,
	Michal Hocko <mhocko@suse.com>,
	Michal Hocko <mhocko@kernel.org>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Shakeel Butt <shakeel.butt@linux.dev>,
	Axel Rasmussen <axelrasmussen@google.com>,
	Yuanchu Xie <yuanchu@google.com>,
	Wei Xu <weixugc@google.com>,
	Qi Zheng <qi.zheng@linux.dev>,
	"Liam R. Howlett" <liam@infradead.org>,
	Kairui Song <kasong@tencent.com>,
	Zhang Peng <bruzzhang@tencent.com>,
	linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 2/5] mm/vmscan: extract folio_free() from shrink_folio_list()
Date: Sun, 20 Sep 2026 14:14:37 +0800
References: <20260720-batch-tlb-flush-v5-0-db943a0d0d6b@icloud.com>
 <20260720-batch-tlb-flush-v5-2-db943a0d0d6b@icloud.com>
 <CAGsJ_4ykbJKVqEv6uGNKZaQhtV1UxxV-MWt7D1jx6SpdMN+cqA@mail.gmail.com>
In-Reply-To: <CAGsJ_4ykbJKVqEv6uGNKZaQhtV1UxxV-MWt7D1jx6SpdMN+cqA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On Fri, Aug 14, 2026 at 5:40 AM Barry Song <baohua@kernel.org> wrote:
>
> Could we avoid hiding the activate semantics inside
> folio_try_reclaim_free()? It makes the logic harder to read and
> can be confusing.
>
> Could we pull this out so that the three possible outcomes are
> explicit?
>
> 1. activate
> 2. keep
> 3. free
[...]
> I mean, this is confusing because an activated folio ends up in the
> "keep" path. Can we make the activation semantics explicit at the
> outer level?

Agreed, and thanks - the bool return was the root of it. The helper
now returns exactly the three outcomes you listed, and the caller,
not the helper, decides what to do with each:

	enum folio_reclaim_result {
		FOLIO_RECLAIM_KEEP,
		FOLIO_RECLAIM_ACTIVATE,
		FOLIO_RECLAIM_SUCCESS,
	};

	switch (folio_try_reclaim_free(folio, &free_folios, sc,
				       &nr_reclaimed)) {
	case FOLIO_RECLAIM_ACTIVATE:
		goto activate_locked;
	case FOLIO_RECLAIM_KEEP:
		goto keep_locked;
	case FOLIO_RECLAIM_SUCCESS:
		continue;
	}

So there is no longer a folio_activate_locked() call inside the
helper at all, and an activated folio no longer disappears into the
"keep" path.

The patch has been respun on that basis and posted in a cleanup-only
series, see my reply on 5/5.

Thanks
Zhang Peng

From nobody Sun Sep 20 00:00:00 2026
From: Zhang Peng <zippermonkey@icloud.com>
To: Barry Song <baohua@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	David Hildenbrand <david@kernel.org>,
	Lorenzo Stoakes <ljs@kernel.org>,
	Vlastimil Babka <vbabka@kernel.org>,
	Mike Rapoport <rppt@kernel.org>,
	Suren Baghdasaryan <surenb@google.com>,
	Michal Hocko <mhocko@suse.com>,
	Michal Hocko <mhocko@kernel.org>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Shakeel Butt <shakeel.butt@linux.dev>,
	Axel Rasmussen <axelrasmussen@google.com>,
	Yuanchu Xie <yuanchu@google.com>,
	Wei Xu <weixugc@google.com>,
	Qi Zheng <qi.zheng@linux.dev>,
	"Liam R. Howlett" <liam@infradead.org>,
	Kairui Song <kasong@tencent.com>,
	Zhang Peng <bruzzhang@tencent.com>,
	linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 3/5] mm/vmscan: extract pageout_one() from shrink_folio_list()
Date: Sun, 20 Sep 2026 14:16:52 +0800
References: <20260720-batch-tlb-flush-v5-0-db943a0d0d6b@icloud.com>
 <20260720-batch-tlb-flush-v5-3-db943a0d0d6b@icloud.com>
 <CAGsJ_4zOfR3Rn5tk_RcOWUDmREO1JyRwXA15Vr3aeUs=+ybMMA@mail.gmail.com>
In-Reply-To: <CAGsJ_4zOfR3Rn5tk_RcOWUDmREO1JyRwXA15Vr3aeUs=+ybMMA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On Fri, Aug 14, 2026 at 5:49 AM Barry Song <baohua@kernel.org> wrote:
>
> Also, this patch looks basically good, just like the previous one.
> Could we also avoid hiding the activation semantics in the inner
> function? It would be clearer to make the activation semantics
> explicit at the outer level, so readers don't have to dig into a
> deep internal function to realize that a folio may take the
> activation path.
>
> In LRU, we have two distinct possibilities: activate a folio or just
> keep it. This is an important semantic distinction in the LRU logic.
> Hiding the activation decision so deep in an inner function makes
> that semantic much less obvious.

Done, same treatment as 2/5 - the helper reports the decision and the
caller acts on it:

	enum folio_pageout_result {
		FOLIO_PAGEOUT_KEEP_LOCKED,
		FOLIO_PAGEOUT_KEEP_UNLOCKED,
		FOLIO_PAGEOUT_ACTIVATE,
		FOLIO_PAGEOUT_FREE,	/* folio is locked, hand it to
					   folio_try_reclaim_free() */
	};

	switch (folio_try_pageout(folio, sc, &ctx, folio_list)) {
	case FOLIO_PAGEOUT_ACTIVATE:
		goto activate_locked;
	case FOLIO_PAGEOUT_KEEP_LOCKED:
		goto keep_locked;
	case FOLIO_PAGEOUT_KEEP_UNLOCKED:
		goto keep;
	case FOLIO_PAGEOUT_FREE:
		break;	/* folio is locked; try to free it below */
	}

Two things beyond what you asked for, both because pageout() has more
outcomes than the freeing path does:

 - "keep" is split into KEEP_LOCKED and KEEP_UNLOCKED. pageout() can
   return with the folio either still locked or already unlocked, and
   previously the caller had to know which internal branch it came
   from to pick between the keep_locked and keep labels. Now the
   result says so.

 - FOLIO_PAGEOUT_FREE keeps the "and now try to free it" step at the
   outer level too, instead of chaining into folio_try_reclaim_free()
   from inside folio_try_pageout().

This patch is in the cleanup-only series, see my reply on 5/5.

Thanks
Zhang Peng

From nobody Sun Sep 20 00:00:00 2026
From: Zhang Peng <zippermonkey@icloud.com>
To: Barry Song <baohua@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	David Hildenbrand <david@kernel.org>,
	Lorenzo Stoakes <ljs@kernel.org>,
	Vlastimil Babka <vbabka@kernel.org>,
	Mike Rapoport <rppt@kernel.org>,
	Suren Baghdasaryan <surenb@google.com>,
	Michal Hocko <mhocko@suse.com>,
	Michal Hocko <mhocko@kernel.org>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Shakeel Butt <shakeel.butt@linux.dev>,
	Axel Rasmussen <axelrasmussen@google.com>,
	Yuanchu Xie <yuanchu@google.com>,
	Wei Xu <weixugc@google.com>,
	Qi Zheng <qi.zheng@linux.dev>,
	"Liam R. Howlett" <liam@infradead.org>,
	Kairui Song <kasong@tencent.com>,
	Zhang Peng <bruzzhang@tencent.com>,
	linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 5/5] mm/vmscan: flush TLB for every 31 folios evictions
Date: Sun, 20 Sep 2026 14:19:08 +0800
References: <20260720-batch-tlb-flush-v5-0-db943a0d0d6b@icloud.com>
 <20260720-batch-tlb-flush-v5-5-db943a0d0d6b@icloud.com>
 <CAGsJ_4wrQRGjqavMzWgi2+PrRr1ztf4QRZEb6HKm02OF8nEBUQ@mail.gmail.com>
In-Reply-To: <CAGsJ_4wrQRGjqavMzWgi2+PrRr1ztf4QRZEb6HKm02OF8nEBUQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On Fri, Aug 14, 2026 at 5:58 AM Barry Song <baohua@kernel.org> wrote:
>
> Yes, I think batching the dirty flush is a great idea. I can clearly
> see that IPIs for dirty flushes (smp_call) on x86 take up a
> significant part of the flame graph when building the kernel in a
> memcg, so I think this is something we should pursue. Could we
> revisit this patch after we clean up the previous ones?

Sounds good, and thanks for confirming the workload - useful to know
the dirty-flush IPIs show up that clearly in a memcg kernel build.

I've split the series accordingly. The four cleanup patches, with the
explicit-outcome rework you asked for in 2/5 and 3/5, are posted on
their own as:

  [PATCH 0/4] mm/vmscan: refactor shrink_folio_list()
  https://lore.kernel.org/all/20260920-vmscan-refactor-v1-0-ec04d71cb761@tencent.com/

No functional change intended there. I'll repost the TLB batching on
top once that has settled.

Thanks
Zhang Peng