Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230713150558.200545-1-fengwei.yin@intel.com>
 <CAOUHufb0zxKvmvEXfG4kySenxyPtagnr_cf4Ms-6si3bQTybGQ@mail.gmail.com> <8547495c-9051-faab-a47d-1962f2e0b1da@intel.com>
In-Reply-To: <8547495c-9051-faab-a47d-1962f2e0b1da@intel.com>
From:   Yu Zhao <yuzhao@google.com>
Date:   Thu, 13 Jul 2023 21:23:21 -0600
Message-ID: <CAOUHufY-edD2j2Nfz3xrObF2ERAGKecjFr_1Qarh5aDwyDGS2A@mail.gmail.com>
Subject: Re: [RFC PATCH] madvise: make madvise_cold_or_pageout_pte_range()
 support large folio
To:     "Yin, Fengwei" <fengwei.yin@intel.com>
Cc:     Minchan Kim <minchan@kernel.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, akpm@linux-foundation.org,
        willy@infradead.org, david@redhat.com, ryan.roberts@arm.com,
        shy828301@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Jul 13, 2023 at 9:10=E2=80=AFPM 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=E2=80=AFAM 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) !=3D 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@gmai=
l.com/
> https://lore.kernel.org/linux-mm/20230124012210.13963-2-vishal.moola@gmai=
l.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 a=
ccurate.
> 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 stabl=
e:

-  if (folio_mapcount(folio) !=3D 1)
+  if (folio_estimated_sharers(folio) !=3D 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.
> >    */