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@xxxxxxxxxxx> 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++;
}
