Re: [PATCH] mm/vmscan: check references from all memcgs for swapbacked memory

[Yosry Ahmed <yosryahmed@google.com>]

On Wed, Oct 5, 2022 at 8:51 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Wed, Oct 05, 2022 at 07:54:25AM -0700, Yosry Ahmed wrote:
> > On Wed, Oct 5, 2022 at 7:04 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > Would you mind moving this to folio_referenced() directly? There is
> > > already a comment and branch in there that IMO would extend quite
> > > naturally to cover the new exception:
> > >
> > >         /*
> > >          * If we are reclaiming on behalf of a cgroup, skip
> > >          * counting on behalf of references from different
> > >          * cgroups
> > >          */
> > >         if (memcg) {
> > >                 rwc.invalid_vma = invalid_folio_referenced_vma;
> > >         }
> > >
> > > That would keep the decision-making and doc in one place.
> >
> > Hi Johannes,
> >
> > Thanks for taking a look!
> >
> > I originally wanted to make the change in folio_referenced(). My only
> > concern was that it wouldn't be clear for people looking at reclaim
> > code in mm/vmscan.c. It would appear as if we are passing in the
> > target memcg to folio_referenced(), and only if you look within you
> > would realize that sometimes it ignores the passed memcg.
> >
> > It seemed to me that deciding whether we want to check references from
> > one memcg or all of them is a reclaim decision, while
> > folio_referenced() is just an rmap API that does what it is told: "if
> > I am passed a memcg, I only look at references coming from this
> > memcg". On the other hand, it looks like the doc has always lived in
> > folio_referenced()/page_referenced(), so I might be overthinking this
> > (I have been known to do this).
>
> I agree it would be nicer to have this policy in vmscan.c. OTOH it's a
> policy that applies to all folio_referenced() callers, and it's
> fragile to require them to opt into it individually.
>
> Vmscan is the only user of the function, so it's not the worst thing
> to treat it as an extension of the reclaim code.
>
> If it helps convince you, there is another, actually quite similar
> reclaim policy already encoded in folio_referenced():
>
>                         if (ptep_clear_flush_young_notify(vma, address,
>                                                 pvmw.pte)) {
>                                 /*
>                                  * Don't treat a reference through
>                                  * a sequentially read mapping as such.
>                                  * If the folio has been used in another mapping,
>                                  * we will catch it; if this other mapping is
>                                  * already gone, the unmap path will have set
>                                  * the referenced flag or activated the folio.
>                                  */
>                                 if (likely(!(vma->vm_flags & VM_SEQ_READ)))
>                                         referenced++;
>                         }

Thanks for clarifying. Will send v2 later today :)