Re: [RFC PATCH] madvise: make madvise_cold_or_pageout_pte_range() support large folio

[Yu Zhao <yuzhao@google.com>]

On Thu, Jul 13, 2023 at 9:10 PM Yin, Fengwei <fengwei.yin@intel.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@intel.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.
> >    */