Re: [PATCH v5 1/5] mm/vmscan: introduce folio_activate_locked() helper
From: Barry Song
Date: Sun Sep 20 2026 - 17:49:59 EST
On Sun, Sep 20, 2026 at 10:38 PM Zhang Peng <zippermonkey@xxxxxxxxxx> wrote:
>
> On Mon, Aug 10, 2026 at 4:36 PM Barry Song <baohua@xxxxxxxxxx> wrote:
> >
> > Reviewed-by: Barry Song <baohua@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> To: Barry Song <baohua@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>,
> David Hildenbrand <david@xxxxxxxxxx>,
> Lorenzo Stoakes <ljs@xxxxxxxxxx>,
> Vlastimil Babka <vbabka@xxxxxxxxxx>,
> Mike Rapoport <rppt@xxxxxxxxxx>,
> Suren Baghdasaryan <surenb@xxxxxxxxxx>,
> Michal Hocko <mhocko@xxxxxxxx>,
> Michal Hocko <mhocko@xxxxxxxxxx>,
> Johannes Weiner <hannes@xxxxxxxxxxx>,
> Shakeel Butt <shakeel.butt@xxxxxxxxx>,
> Axel Rasmussen <axelrasmussen@xxxxxxxxxx>,
> Yuanchu Xie <yuanchu@xxxxxxxxxx>,
> Wei Xu <weixugc@xxxxxxxxxx>,
> Qi Zheng <qi.zheng@xxxxxxxxx>,
> "Liam R. Howlett" <liam@xxxxxxxxxxxxx>,
> Kairui Song <kasong@xxxxxxxxxxx>,
> Zhang Peng <bruzzhang@xxxxxxxxxxx>,
> linux-mm@xxxxxxxxx,
> linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> <20260720-batch-tlb-flush-v5-2-db943a0d0d6b@xxxxxxxxxx>
> <CAGsJ_4ykbJKVqEv6uGNKZaQhtV1UxxV-MWt7D1jx6SpdMN+cqA@xxxxxxxxxxxxxx>
> In-Reply-To: <CAGsJ_4ykbJKVqEv6uGNKZaQhtV1UxxV-MWt7D1jx6SpdMN+cqA@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxx> 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
> [...]
Also, Peng, I'm struggling to follow what you're saying. Could you
please reply directly to the comments where I asked the questions?
Please don't collect comments from so many different patches in one
place and reply to them all together. This makes it very difficult to
keep the context and causes a lot of unnecessary back-and-forth.
Best Regards
Barry