| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aQ3yLER4C4jY70BH@harry>
Date: Fri, 7 Nov 2025 22:20:57 +0900
From: Harry Yoo <harry.yoo@...cle.com>
To: Qi Zheng <qi.zheng@...ux.dev>
Cc: hannes@...xchg.org, hughd@...gle.com, mhocko@...e.com,
roman.gushchin@...ux.dev, shakeel.butt@...ux.dev,
muchun.song@...ux.dev, david@...hat.com, lorenzo.stoakes@...cle.com,
ziy@...dia.com, imran.f.khan@...cle.com, kamalesh.babulal@...cle.com,
axelrasmussen@...gle.com, yuanchu@...gle.com, weixugc@...gle.com,
akpm@...ux-foundation.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, cgroups@...r.kernel.org,
Muchun Song <songmuchun@...edance.com>,
Qi Zheng <zhengqi.arch@...edance.com>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Clark Williams <clrkwllms@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>, linux-rt-devel@...ts.linux.dev
Subject: Re: [PATCH v1 04/26] mm: vmscan: refactor move_folios_to_lru()
On Fri, Nov 07, 2025 at 02:41:13PM +0800, Qi Zheng wrote:
> Hi Harry,
>
> On 11/7/25 1:11 PM, Harry Yoo wrote:
> > On Tue, Oct 28, 2025 at 09:58:17PM +0800, Qi Zheng wrote:
> > > From: Muchun Song <songmuchun@...edance.com>
> > >
> > > In a subsequent patch, we'll reparent the LRU folios. The folios that are
> > > moved to the appropriate LRU list can undergo reparenting during the
> > > move_folios_to_lru() process. Hence, it's incorrect for the caller to hold
> > > a lruvec lock. Instead, we should utilize the more general interface of
> > > folio_lruvec_relock_irq() to obtain the correct lruvec lock.
> > >
> > > This patch involves only code refactoring and doesn't introduce any
> > > functional changes.
> > >
> > > Signed-off-by: Muchun Song <songmuchun@...edance.com>
> > > Acked-by: Johannes Weiner <hannes@...xchg.org>
> > > Signed-off-by: Qi Zheng <zhengqi.arch@...edance.com>
> > > ---
> > > mm/vmscan.c | 46 +++++++++++++++++++++++-----------------------
> > > 1 file changed, 23 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 3a1044ce30f1e..660cd40cfddd4 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -2016,9 +2016,9 @@ static unsigned long shrink_inactive_list(unsigned long nr_to_scan,
> > > nr_reclaimed = shrink_folio_list(&folio_list, pgdat, sc, &stat, false,
> > > lruvec_memcg(lruvec));
> > > - spin_lock_irq(&lruvec->lru_lock);
> > > - move_folios_to_lru(lruvec, &folio_list);
> > > + move_folios_to_lru(&folio_list);
> > > + spin_lock_irq(&lruvec->lru_lock);
> > > __mod_lruvec_state(lruvec, PGDEMOTE_KSWAPD + reclaimer_offset(sc),
> > > stat.nr_demoted);
> >
> > Maybe I'm missing something or just confused for now, but let me ask...
> >
> > How do we make sure the lruvec (and the mem_cgroup containing the
> > lruvec) did not disappear (due to offlining) after move_folios_to_lru()?
>
> We obtained lruvec through the following method:
>
> memcg = mem_cgroup_iter(target_memcg, NULL, partial);
> do {
> struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
>
> shrink_lruvec(lruvec, sc);
> --> shrink_inactive_list
> } while ((memcg = mem_cgroup_iter(target_memcg, memcg, partial)));
>
> The mem_cgroup_iter() will hold the refcount of this memcg, so IIUC,
> the memcg will not disappear at this time.
Ah, right!
It can be offlined, but won't be released due to the refcount.
Thanks for the explanation.
> > > __mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, -nr_taken);
> > > @@ -2166,11 +2166,10 @@ static void shrink_active_list(unsigned long nr_to_scan,
> > > /*
> > > * Move folios back to the lru list.
> > > */
> > > - spin_lock_irq(&lruvec->lru_lock);
> > > -
> > > - nr_activate = move_folios_to_lru(lruvec, &l_active);
> > > - nr_deactivate = move_folios_to_lru(lruvec, &l_inactive);
> > > + nr_activate = move_folios_to_lru(&l_active);
> > > + nr_deactivate = move_folios_to_lru(&l_inactive);
> > > + spin_lock_irq(&lruvec->lru_lock);
> > > __count_vm_events(PGDEACTIVATE, nr_deactivate);
> > > count_memcg_events(lruvec_memcg(lruvec), PGDEACTIVATE, nr_deactivate);
> > > @@ -4735,14 +4734,15 @@ static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
> > > set_mask_bits(&folio->flags.f, LRU_REFS_FLAGS, BIT(PG_active));
> > > }
> > > - spin_lock_irq(&lruvec->lru_lock);
> > > -
> > > - move_folios_to_lru(lruvec, &list);
> > > + move_folios_to_lru(&list);
> > > + local_irq_disable();
> > > walk = current->reclaim_state->mm_walk;
> > > if (walk && walk->batched) {
> > > walk->lruvec = lruvec;
> > > + spin_lock(&lruvec->lru_lock);
> > > reset_batch_size(walk);
> > > + spin_unlock(&lruvec->lru_lock);
> > > }
> >
> > Cc'ing RT folks as they may not want to disable IRQ on PREEMPT_RT.
> >
> > IIRC there has been some effort in MM to reduce the scope of
> > IRQ-disabled section in MM when PREEMPT_RT config was added to the
> > mainline. spin_lock_irq() doesn't disable IRQ on PREEMPT_RT.
>
> Thanks for this information.
>
> >
> > Also, this will break RT according to Documentation/locking/locktypes.rst:
> > > The changes in spinlock_t and rwlock_t semantics on PREEMPT_RT kernels
> > > have a few implications. For example, on a non-PREEMPT_RT kernel
> > > the following code sequence works as expected:
> > >
> > > local_irq_disable();
> > > spin_lock(&lock);
> > >
> > > and is fully equivalent to:
> > >
> > > spin_lock_irq(&lock);
> > > Same applies to rwlock_t and the _irqsave() suffix variants.
> > >
> > > On PREEMPT_RT kernel this code sequence breaks because RT-mutex requires
> > > a fully preemptible context. Instead, use spin_lock_irq() or
> > > spin_lock_irqsave() and their unlock counterparts.
> > >
> > > In cases where the interrupt disabling and locking must remain separate,
> > > PREEMPT_RT offers a local_lock mechanism. Acquiring the local_lock pins
> > > the task to a CPU, allowing things like per-CPU interrupt disabled locks
> > > to be acquired. However, this approach should be used only where absolutely
> > > necessary.
>
> But how do we determine if it's necessary?
Although it's mentioned in the locking documentation, I'm afraid that
local_lock is not the right interface to use here. Preemption will be
disabled anyway (on both PREEMPT_RT and !PREEMPT_RT) when the stats are
updated (in __mod_node_page_state()).
Here we just want to disable IRQ only on !PREEMPT_RT (to update
the stats safely).
Maybe we'll need some ifdeffery? I don't see a better way around...
--
Cheers,
Harry / Hyeonggon
Powered by blists - more mailing lists