On Wed, Jan 4, 2023 at 2:29 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> From: Johannes Weiner <hannes@xxxxxxxxxxx>
>
> Refault decisions are made based on the lruvec where the page was
> evicted, as that determined its LRU order while it was alive. Stats
> and workingset aging must then occur on the lruvec of the new page, as
> that's the node and cgroup that experience the refault and that's the
> lruvec whose nonresident info ages out by a new resident page. Those
> lruvecs could be different when a page is shared between cgroups, or
> the refaulting page is allocated on a different node.
>
> There are currently two mix-ups:
>
> 1. When swap is available, the resident anon set must be considered
> when comparing the refault distance. The comparison is made against
> the right anon set, but the check for swap is not. When pages get
> evicted from a cgroup with swap, and refault in one without, this
> can incorrectly consider a hot refault as cold - and vice
> versa. Fix that by using the eviction cgroup for the swap check.
>
> 2. The stats and workingset age are updated against the wrong lruvec
> altogether: the right cgroup but the wrong NUMA node. When a page
> refaults on a different NUMA node, this will have confusing stats
> and distort the workingset age on a different lruvec - again
> possibly resulting in hot/cold misclassifications down the line.
>
> Fix the swap check and the refault pgdat to address both concerns.
>
> This was found during code review. It hasn't caused notable issues in
> production, suggesting that those refault-migrations are relatively
> rare in practice.
>
> Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> Co-developed-by: Nhat Pham <nphamcs@xxxxxxxxx>
> Signed-off-by: Nhat Pham <nphamcs@xxxxxxxxx>
> ---
> mm/workingset.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/workingset.c b/mm/workingset.c
> index ae7e984b23c6..79585d55c45d 100644
> --- a/mm/workingset.c
> +++ b/mm/workingset.c
> @@ -457,6 +457,7 @@ void workingset_refault(struct folio *folio, void *shadow)
> */
> nr = folio_nr_pages(folio);
> memcg = folio_memcg(folio);
> + pgdat = folio_pgdat(folio);
> lruvec = mem_cgroup_lruvec(memcg, pgdat);
>
> mod_lruvec_state(lruvec, WORKINGSET_REFAULT_BASE + file, nr);
> @@ -474,7 +475,7 @@ void workingset_refault(struct folio *folio, void *shadow)
> workingset_size += lruvec_page_state(eviction_lruvec,
> NR_INACTIVE_FILE);
> }
> - if (mem_cgroup_get_nr_swap_pages(memcg) > 0) {
> + if (mem_cgroup_get_nr_swap_pages(eviction_memcg) > 0) {
> workingset_size += lruvec_page_state(eviction_lruvec,
> NR_ACTIVE_ANON);
> if (file) {
> --
> 2.30.2
Oh this one is sent out twice too... Something is wrong with my pipeline...
Anyway, please disregard the first email and only review this one!
