Re: [PATCH] mm: madvise: fix uneven accounting of psi

[Suren Baghdasaryan <surenb@google.com>]

On Fri, Jun 9, 2023 at 5:42 AM Charan Teja Kalla
<quic_charante@quicinc.com> wrote:
>
> Thanks Suren & Johannes,
>
> On 6/7/2023 1:18 AM, Suren Baghdasaryan wrote:
> > Hi Folks. Sorry for being late to the party.
> > Yeah, userspace does not have a crystal ball to predict future user
> > behavior, so there will always be pathological cases when usual
> > assumptions and resulting madvise() would make things worse.
> >
> > I think this discussion can be split into several questions/issues:
> > 1. Inconsistency in how madvise(MADV_PAGEOUT) would affect PSI
> > calculation when the page is refaulted, based on the path it took
> > before being evicted by madvise(). In your initial description case
> > (a) is inconsistent with (b) and (c) and it's probably worth fixing.
> > IMHO (a) should be made consistent with others, not the other way
> > around. My reasoning is that page was expelled from the active list,
> > so it was part of the active workingset.
> >
> That means we should be setting Workingset on the page while it is on
> the active list and when it is being pageout through madvising. Right? I
> see, this makes it consistent.

This was my opinion but others might think otherwise, like I found out
in some recent conversations. So, it would be great to get some more
feedback before making the change.

>
> On the same note, discussing with Suren offline, Should the refaulted
> madvise pages start always at the inactive list? If they are really
> active, they get promoted anyway..
>
> > 2. Whether refaults caused by incorrect madvise(MADV_PAGEOUT) should
> > be counted as workingset refault and affect PSI.
> > This one I think is trickier. IMHO it should be counted as workingset
> > refault simply because it was refaulted and it was part of the
> > workingset. Whether it should affect PSI, which is supposed to be an
> > indicator of "pressure" is, I think, debatable. With madvise() in the
> > mix, refault might happen without any real memory pressure... So, the
> > answer is not obvious to me.
> >
> > 3. Should refaults caused by incorrect madvise(MADV_PAGEOUT) be
> > distinguished from the ones which were evicted by kernel reclaim
> > mechanisms.
> > I can see use for that from userspace to detect incorrect madvise()
> > and adjust its aggressiveness. I think the API might get a bit complex
> > because of the need to associate refaults with specific madvise()/VMAs
> > to understand which hint was incorrect and adjust the behavior.
> > Instead what is the opinion about giving an MADV_PAGEOUT_INACTIVE
> interface which does operate on a page only If it is on the inactive
> list and !PageWorkingset ?

IOW you want a less aggressive mechanism which can be used by the
userspace to tell the kernel "I think these pages won't be used but
I'm not 100% sure, so drop them only if they are inactive"?
 I don't know how much that will help when the madvise() ends up being
wrong but maybe you can quickly experiment and tell us if the
difference is substantial?

>
> > Hope my feedback is useful and if we can improve Android's userspace
> > behavior, I'm happy to help make that happen.
> Thanks...