On Wed, Oct 5, 2022 at 8:51 AM Johannes Weiner <hannes@xxxxxxxxxxx> 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@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++;
> }
Thanks for clarifying. Will send v2 later today :)
