Re: [External] Re: [PATCH RFC 26/28] mm: memcontrol: introduce memcg_reparent_ops

[Harry Yoo <harry.yoo@oracle.com>]

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@oracle.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