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: <CAOUHufb0zxKvmvEXfG4kySenxyPtagnr_cf4Ms-6si3bQTybGQ@mail.gmail.com>
Date:   Thu, 13 Jul 2023 20:08:38 -0600
From:   Yu Zhao <yuzhao@...gle.com>
To:     Yin Fengwei <fengwei.yin@...el.com>,
        Minchan Kim <minchan@...nel.org>
Cc:     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: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

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.
   */

> The main reason to call folio_referenced() is to clear the yong of
> conresponding PTEs. So in page reclaim phase, there is good chance
> the folio can be reclaimed.
>
> Signed-off-by: Yin Fengwei <fengwei.yin@...el.com>
> ---
> This patch is based on mlock large folio support rfc2 as it depends
> on the folio_in_range() added by that patchset
>
> Also folio_op_size() can be unitfied with get_folio_mlock_step().
>
> Testing done:
>   - kselftest: No new regression introduced.
>
>  mm/madvise.c | 133 ++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 84 insertions(+), 49 deletions(-)

Also the refactor looks fine to me but it'd be better if it's a separate patch.