linux-kernel - Re: [External] Re: [PATCH RFC 26/28] mm: memcontrol: introduce memcg_reparent

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aG20emoRBvezUXTx@hyeyoo>
Date: Wed, 9 Jul 2025 09:14:50 +0900
From: Harry Yoo <harry.yoo@...cle.com>
To: Muchun Song <songmuchun@...edance.com>
Cc: hannes@...xchg.org, mhocko@...nel.org, roman.gushchin@...ux.dev,
        shakeel.butt@...ux.dev, muchun.song@...ux.dev,
        akpm@...ux-foundation.org, david@...morbit.com,
        zhengqi.arch@...edance.com, yosry.ahmed@...ux.dev, nphamcs@...il.com,
        chengming.zhou@...ux.dev, linux-kernel@...r.kernel.org,
        cgroups@...r.kernel.org, linux-mm@...ck.org,
        hamzamahfooz@...ux.microsoft.com, apais@...ux.microsoft.com
Subject: Re: [External] Re: [PATCH RFC 26/28] mm: memcontrol: introduce
 memcg_reparent_ops

On Mon, Jul 07, 2025 at 05:29:22PM +0800, Muchun Song wrote:
> On Wed, Jul 2, 2025 at 6:13 AM Harry Yoo <harry.yoo@...cle.com> wrote:
> > > Hi,
> > >
> > > It seems unnecessarily complicated to 1) acquire objcg, lruvec and
> > > thp_sq locks, 2) call their ->relocate() callbacks, and
> > > 3) release those locks.
> > >
> > > Why not simply do the following instead?
> > >
> > > for (i = 0; i < ARRAY_SIZE(memcg_reparent_ops); i++) {
> > >       local_irq_disable();
> > >       memcg_reparent_ops[i]->lock(src, dst);
> > >       memcg_reparent_ops[i]->relocate(src, dst);
> > >       memcg_reparent_ops[i]->unlock(src, dst);
> > >       local_irq_enable();
> > > }
> > >
> > > As there is no actual lock dependency between the three.
> > >
> > > Or am I missing something important about the locking requirements?
> >
> > Hmm... looks like I was missing some important requirements!
> >
> > It seems like:
> >
> > 1) objcg should be reparented under lruvec locks, otherwise
> >    users can observe folio_memcg(folio) != lruvec_memcg(lruvec)
> >
> > 2) Similarly, lruvec_reparent_relocate() should reparent all folios
> >    at once under lruvec locks, otherwise users can observe
> >    folio_memcg(folio) != lruvec_memcg(lruvec) for some folios.
> >
> >    IoW, lruvec_reparent_relocate() cannot do something like this:
> >    while (lruvec is not empty) {
> >            move some pages;
> >            unlock lruvec locks;
> >            cond_resched();
> >            lock lruvec locks;
> >    }
> >
> > Failing to satisfy 1) and 2) means user can't rely on a stable binding
> > between a folio and a memcg, which is a no-go.
> >
> > Also, 2) makes it quite undesirable to iterate over folios and move each
> > one to the right generation in MGLRU as this will certainly introduce
> > soft lockups as the memcg size grows...
> 
> Sorry for the late reply.

No problem, thanks for replying!

> Yes, you are right. We should iterate each folio
> without holding any spinlocks.

Wait, did you decide to iterate each folio and update the generation
anyway? I assumed you're still in move-folios-at-once-without-iteration
camp.

> So I am confirming if we could do some
> preparation work like making it suitable for reparenting without holding any
> spinlock. Then we could reparent those folios like the non-MGLRU case.

You mean something like take folios from LRU under spinlock and iterate
them without spinlock?

Anyway, I'd be more than happy to review future revisions of the series.

-- 
Cheers,
Harry / Hyeonggon