Received-SPF: pass (google.com: domain of linux-kernel+bounces-101172-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240311150058.1122862-1-ryan.roberts@arm.com> <20240311150058.1122862-7-ryan.roberts@arm.com>
In-Reply-To: <20240311150058.1122862-7-ryan.roberts@arm.com>
From: Barry Song <21cnbao@gmail.com>
Date: Wed, 13 Mar 2024 20:19:49 +1300
Message-ID: <CAGsJ_4wpjqRsn7ouO=Ut9oMBLSh803=XuSPX6gJ5nQ3jyqh3hQ@mail.gmail.com>
Subject: Re: [PATCH v4 6/6] mm: madvise: Avoid split during MADV_PAGEOUT and MADV_COLD
To: Ryan Roberts <ryan.roberts@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, David Hildenbrand <david@redhat.com>, 
	Matthew Wilcox <willy@infradead.org>, Huang Ying <ying.huang@intel.com>, Gao Xiang <xiang@kernel.org>, 
	Yu Zhao <yuzhao@google.com>, Yang Shi <shy828301@gmail.com>, Michal Hocko <mhocko@suse.com>, 
	Kefeng Wang <wangkefeng.wang@huawei.com>, Chris Li <chrisl@kernel.org>, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 12, 2024 at 4:01=E2=80=AFAM Ryan Roberts <ryan.roberts@arm.com>=
 wrote:
>
> Rework madvise_cold_or_pageout_pte_range() to avoid splitting any large
> folio that is fully and contiguously mapped in the pageout/cold vm
> range. This change means that large folios will be maintained all the
> way to swap storage. This both improves performance during swap-out, by
> eliding the cost of splitting the folio, and sets us up nicely for
> maintaining the large folio when it is swapped back in (to be covered in
> a separate series).
>
> Folios that are not fully mapped in the target range are still split,
> but note that behavior is changed so that if the split fails for any
> reason (folio locked, shared, etc) we now leave it as is and move to the
> next pte in the range and continue work on the proceeding folios.
> Previously any failure of this sort would cause the entire operation to
> give up and no folios mapped at higher addresses were paged out or made
> cold. Given large folios are becoming more common, this old behavior
> would have likely lead to wasted opportunities.
>
> While we are at it, change the code that clears young from the ptes to
> use ptep_test_and_clear_young(), which is more efficent than
> get_and_clear/modify/set, especially for contpte mappings on arm64,
> where the old approach would require unfolding/refolding and the new
> approach can be done in place.
>
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>

This looks so much better than our initial RFC.
Thank you for your excellent work!

> ---
>  mm/madvise.c | 89 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 51 insertions(+), 38 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 547dcd1f7a39..56c7ba7bd558 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -336,6 +336,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *p=
md,
>         LIST_HEAD(folio_list);
>         bool pageout_anon_only_filter;
>         unsigned int batch_count =3D 0;
> +       int nr;
>
>         if (fatal_signal_pending(current))
>                 return -EINTR;
> @@ -423,7 +424,8 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *p=
md,
>                 return 0;
>         flush_tlb_batched_pending(mm);
>         arch_enter_lazy_mmu_mode();
> -       for (; addr < end; pte++, addr +=3D PAGE_SIZE) {
> +       for (; addr < end; pte +=3D nr, addr +=3D nr * PAGE_SIZE) {
> +               nr =3D 1;
>                 ptent =3D ptep_get(pte);
>
>                 if (++batch_count =3D=3D SWAP_CLUSTER_MAX) {
> @@ -447,55 +449,66 @@ static int madvise_cold_or_pageout_pte_range(pmd_t =
*pmd,
>                         continue;
>
>                 /*
> -                * Creating a THP page is expensive so split it only if w=
e
> -                * are sure it's worth. Split it if we are only owner.
> +                * If we encounter a large folio, only split it if it is =
not
> +                * fully mapped within the range we are operating on. Oth=
erwise
> +                * leave it as is so that it can be swapped out whole. If=
 we
> +                * fail to split a folio, leave it in place and advance t=
o the
> +                * next pte in the range.
>                  */
>                 if (folio_test_large(folio)) {
> -                       int err;
> -
> -                       if (folio_estimated_sharers(folio) > 1)
> -                               break;
> -                       if (pageout_anon_only_filter && !folio_test_anon(=
folio))
> -                               break;
> -                       if (!folio_trylock(folio))
> -                               break;
> -                       folio_get(folio);
> -                       arch_leave_lazy_mmu_mode();
> -                       pte_unmap_unlock(start_pte, ptl);
> -                       start_pte =3D NULL;
> -                       err =3D split_folio(folio);
> -                       folio_unlock(folio);
> -                       folio_put(folio);
> -                       if (err)
> -                               break;
> -                       start_pte =3D pte =3D
> -                               pte_offset_map_lock(mm, pmd, addr, &ptl);
> -                       if (!start_pte)
> -                               break;
> -                       arch_enter_lazy_mmu_mode();
> -                       pte--;
> -                       addr -=3D PAGE_SIZE;
> -                       continue;
> +                       const fpb_t fpb_flags =3D FPB_IGNORE_DIRTY |
> +                                               FPB_IGNORE_SOFT_DIRTY;
> +                       int max_nr =3D (end - addr) / PAGE_SIZE;
> +
> +                       nr =3D folio_pte_batch(folio, addr, pte, ptent, m=
ax_nr,
> +                                            fpb_flags, NULL);

I wonder if we have a quick way to avoid folio_pte_batch() if users
are doing madvise() on a portion of a large folio.

> +
> +                       if (nr < folio_nr_pages(folio)) {
> +                               int err;
> +
> +                               if (folio_estimated_sharers(folio) > 1)
> +                                       continue;
> +                               if (pageout_anon_only_filter && !folio_te=
st_anon(folio))
> +                                       continue;
> +                               if (!folio_trylock(folio))
> +                                       continue;
> +                               folio_get(folio);
> +                               arch_leave_lazy_mmu_mode();
> +                               pte_unmap_unlock(start_pte, ptl);
> +                               start_pte =3D NULL;
> +                               err =3D split_folio(folio);
> +                               folio_unlock(folio);
> +                               folio_put(folio);
> +                               if (err)
> +                                       continue;
> +                               start_pte =3D pte =3D
> +                                       pte_offset_map_lock(mm, pmd, addr=
, &ptl);
> +                               if (!start_pte)
> +                                       break;
> +                               arch_enter_lazy_mmu_mode();
> +                               nr =3D 0;
> +                               continue;
> +                       }
>                 }
>
>                 /*
>                  * Do not interfere with other mappings of this folio and
> -                * non-LRU folio.
> +                * non-LRU folio. If we have a large folio at this point,=
 we
> +                * know it is fully mapped so if its mapcount is the same=
 as its
> +                * number of pages, it must be exclusive.
>                  */
> -               if (!folio_test_lru(folio) || folio_mapcount(folio) !=3D =
1)
> +               if (!folio_test_lru(folio) ||
> +                   folio_mapcount(folio) !=3D folio_nr_pages(folio))
>                         continue;

This looks so perfect and is exactly what I wanted to achieve.

>
>                 if (pageout_anon_only_filter && !folio_test_anon(folio))
>                         continue;
>
> -               VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
> -
> -               if (!pageout && pte_young(ptent)) {
> -                       ptent =3D ptep_get_and_clear_full(mm, addr, pte,
> -                                                       tlb->fullmm);
> -                       ptent =3D pte_mkold(ptent);
> -                       set_pte_at(mm, addr, pte, ptent);
> -                       tlb_remove_tlb_entry(tlb, pte, addr);
> +               if (!pageout) {
> +                       for (; nr !=3D 0; nr--, pte++, addr +=3D PAGE_SIZ=
E) {
> +                               if (ptep_test_and_clear_young(vma, addr, =
pte))
> +                                       tlb_remove_tlb_entry(tlb, pte, ad=
dr);
> +                       }

This looks so smart. if it is not pageout, we have increased pte
and addr here; so nr is 0 and we don't need to increase again in
for (; addr < end; pte +=3D nr, addr +=3D nr * PAGE_SIZE)

otherwise, nr won't be 0. so we will increase addr and
pte by nr.


>                 }
>
>                 /*
> --
> 2.25.1
>

Overall, LGTM,

Reviewed-by: Barry Song <v-songbaohua@oppo.com>