linux-kernel - Re: [RFC PATCH] madvise: make madvise_cold_or_pageout_pte

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOUHufY-edD2j2Nfz3xrObF2ERAGKecjFr_1Qarh5aDwyDGS2A@mail.gmail.com>
Date:   Thu, 13 Jul 2023 21:23:21 -0600
From:   Yu Zhao <yuzhao@...gle.com>
To:     "Yin, Fengwei" <fengwei.yin@...el.com>
Cc:     Minchan Kim <minchan@...nel.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
        willy@...radead.org, david@...hat.com, ryan.roberts@....com,
        shy828301@...il.com
Subject: Re: [RFC PATCH] madvise: make madvise_cold_or_pageout_pte_range()
 support large folio

On Thu, Jul 13, 2023 at 9:10 PM Yin, Fengwei <fengwei.yin@...el.com> wrote:
>
>
>
> On 7/14/2023 10:08 AM, Yu Zhao wrote:
> > On Thu, Jul 13, 2023 at 9:06 AM Yin Fengwei <fengwei.yin@...el.com> wrote:
> >>
> >> Current madvise_cold_or_pageout_pte_range() has two problems for
> >> large folio support:
> >>   - Using folio_mapcount() with large folio prevent large folio from
> >>     picking up.
> >>   - If large folio is in the range requested, shouldn't split it
> >>     in madvise_cold_or_pageout_pte_range().
> >>
> >> Fix them by:
> >>   - Use folio_estimated_sharers() with large folio
> >>   - If large folio is in the range requested, don't split it. Leave
> >>     to page reclaim phase.
> >>
> >> For large folio cross boundaries of requested range, skip it if it's
> >> page cache. Try to split it if it's anonymous folio. If splitting
> >> fails, skip it.
> >
> > For now, we may not want to change the existing semantic (heuristic).
> > IOW, we may want to stick to the "only owner" condition:
> >
> >   - if (folio_mapcount(folio) != 1)
> >   + if (folio_entire_mapcount(folio) ||
> >   +     (any_page_within_range_has_mapcount > 1))
> >
> > +Minchan Kim
> The folio_estimated_sharers() was discussed here:
> https://lore.kernel.org/linux-mm/20230118232219.27038-6-vishal.moola@gmail.com/
> https://lore.kernel.org/linux-mm/20230124012210.13963-2-vishal.moola@gmail.com/
>
> Yes. It's accurate to check each page of large folio. But it may be over killed in
> some cases (And I think madvise is one of the cases not necessary to be accurate.
> So folio_estimated_sharers() is enough. Correct me if I am wrong).

I see. Then it's possible this is also what the original commit wants
to do -- Minchan, could you clarify?

Regardless, I think we can have the following fix, potentially cc'ing stable:

-  if (folio_mapcount(folio) != 1)
+  if (folio_estimated_sharers(folio) != 1)

Sounds good?

> > Also there is an existing bug here: the later commit 07e8c82b5eff8
> > ("madvise: convert madvise_cold_or_pageout_pte_range() to use folios")
> > is incorrect for sure; the original commit 9c276cc65a58f ("mm:
> > introduce MADV_COLD") seems incorrect too.
> >
> > +Vishal Moola (Oracle)
> >
> > The "any_page_within_range_has_mapcount" test above seems to be the
> > only correct to meet condition claimed by the comments, before or
> > after the folio conversion, assuming here a THP page means the
> > compound page without PMD mappings (PMD-split). Otherwise the test is
> > always false (if it's also PMD mapped somewhere else).
> >
> >   /*
> >    * Creating a THP page is expensive so split it only if we
> >    * are sure it's worth. Split it if we are only owner.
> >    */