Message-ID: <CAAmzW4Mpm6PyZp1jXUo__S-OZ2=MKPuyTA+gpL0X8cW+H0ps4Q@mail.gmail.com>
Date:   Fri, 13 Mar 2020 16:40:18 +0900
From:   Joonsoo Kim <js1304@...il.com>
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Michal Hocko <mhocko@...nel.org>,
        Hugh Dickins <hughd@...gle.com>,
        Minchan Kim <minchan@...nel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Mel Gorman <mgorman@...hsingularity.net>, kernel-team@....com,
        Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH v2 2/9] mm/vmscan: protect the workingset on anonymous LRU

2020년 3월 13일 (금) 오전 12:14, Johannes Weiner <hannes@...xchg.org>님이 작성:
>
> On Thu, Feb 20, 2020 at 02:11:46PM +0900, js1304@...il.com wrote:
> > @@ -1010,8 +1010,15 @@ static enum page_references page_check_references(struct page *page,
> >               return PAGEREF_RECLAIM;
> >
> >       if (referenced_ptes) {
> > -             if (PageSwapBacked(page))
> > -                     return PAGEREF_ACTIVATE;
> > +             if (PageSwapBacked(page)) {
> > +                     if (referenced_page) {
> > +                             ClearPageReferenced(page);
> > +                             return PAGEREF_ACTIVATE;
> > +                     }
>
> This looks odd to me. referenced_page = TestClearPageReferenced()
> above, so it's already be clear. Why clear it again?

Oops... it's just my fault. Will remove it.

> > +
> > +                     SetPageReferenced(page);
> > +                     return PAGEREF_KEEP;
> > +             }
>
> The existing file code already does:
>
>                 SetPageReferenced(page);
>                 if (referenced_page || referenced_ptes > 1)
>                         return PAGEREF_ACTIVATE;
>                 if (vm_flags & VM_EXEC)
>                         return PAGEREF_ACTIVATE;
>                 return PAGEREF_KEEP;
>
> The differences are:
>
> 1) referenced_ptes > 1. We did this so that heavily shared file
> mappings are protected a bit better than others. Arguably the same
> could apply for anon pages when we put them on the inactive list.

Yes, these check should be included for anon.

> 2) vm_flags & VM_EXEC. This mostly doesn't apply to anon pages. The
> exception would be jit code pages, but if we put anon pages on the
> inactive list we should protect jit code the same way we protect file
> executables.

I'm not sure that this is necessary for anon page. From my understanding,
executable mapped file page is more precious than other mapped file page
because this mapping is usually used by *multiple* thread and there is
no way to check it by MM. If anon JIT code has also such characteristic, this
code should be included for anon, but, should be included separately. It
seems that it's beyond of this patch.

> Seems to me you don't need to add anything. Just remove the
> PageSwapBacked branch and apply equal treatment to both types.

I will rework the code if you agree with my opinion.

> > @@ -2056,6 +2063,15 @@ static void shrink_active_list(unsigned long nr_to_scan,
> >                       }
> >               }
> >
> > +             /*
> > +              * Now, newly created anonymous page isn't appened to the
> > +              * active list. We don't need to clear the reference bit here.
> > +              */
> > +             if (PageSwapBacked(page)) {
> > +                     ClearPageReferenced(page);
> > +                     goto deactivate;
> > +             }
>
> I don't understand this.
>
> If you don't clear the pte references, you're leaving behind stale
> data. You already decide here that we consider the page referenced
> when it reaches the end of the inactive list, regardless of what
> happens in between. That makes the deactivation kind of useless.

My idea is that the pages newly appended to the inactive list, for example,
a newly allocated anon page or deactivated page, start at the same line.
A newly allocated anon page would have a mapping (reference) so I
made this code to help for deactivated page to have a mapping (reference).
I think that there is no reason to devalue the page accessed on active list.

Before this patch is applied, all newly allocated anon page are started
at the active list so clearing the pte reference on deactivation is required
to check the further access. However, it is not the case so I skip it here.

> And it blurs the lines between the inactive and active list.
>
> shrink_page_list() (and page_check_references()) are written with the
> notion that any references they look at are from the inactive list. If
> you carry over stale data, this can cause more subtle bugs later on.

It's not. For file page, PageReferenced() is maintained even if deactivation
happens and it means one reference.

> And again, I don't quite understand why anon would need different
> treatment here than file.

In order to preserve the current behaviour for the file page, I leave the code
as is for the file page and change the code for the anon page. There is
fundamental difference between them such as how referenced is checked,
accessed by mapping and accessed by syscall. I think that some difference
would be admitted.

Thanks.

Powered by blists - more mailing lists