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: <CAK1f24=yxwEP1xoC6ZHCXDES8hwD3wA27TxO+XWo8d8LgocDxA@mail.gmail.com>
Date: Tue, 14 Jan 2025 14:30:01 +0800
From: Lance Yang <ioworker0@...il.com>
To: Barry Song <21cnbao@...il.com>
Cc: baolin.wang@...ux.alibaba.com, akpm@...ux-foundation.org, 
	chrisl@...nel.org, david@...hat.com, kasong@...cent.com, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, linux-riscv@...ts.infradead.org, 
	lorenzo.stoakes@...cle.com, ryan.roberts@....com, v-songbaohua@...o.com, 
	x86@...nel.org, ying.huang@...el.com, zhengtangquan@...o.com
Subject: Re: [PATCH v2 4/4] mm: Avoid splitting pmd for lazyfree pmd-mapped
 THP in try_to_unmap

On Tue, Jan 14, 2025 at 12:09 PM Barry Song <21cnbao@...il.com> wrote:
>
> On Tue, Jan 14, 2025 at 4:40 PM Baolin Wang <baolin.wang@...ux.alibaba.com> wrote:
> >
> >
> >
> > On 2025/1/13 11:39, Barry Song wrote:
> > > From: Barry Song <v-songbaohua@...o.com>
> > >
> > > The try_to_unmap_one() function currently handles PMD-mapped THPs
> > > inefficiently. It first splits the PMD into PTEs, copies the dirty
> > > state from the PMD to the PTEs, iterates over the PTEs to locate
> > > the dirty state, and then marks the THP as swap-backed. This process
> > > involves unnecessary PMD splitting and redundant iteration. Instead,
> > > this functionality can be efficiently managed in
> > > __discard_anon_folio_pmd_locked(), avoiding the extra steps and
> > > improving performance.
> > >
> > > The following microbenchmark redirties folios after invoking MADV_FREE,
> > > then measures the time taken to perform memory reclamation (actually
> > > set those folios swapbacked again) on the redirtied folios.
> > >
> > >   #include <stdio.h>
> > >   #include <sys/mman.h>
> > >   #include <string.h>
> > >   #include <time.h>
> > >
> > >   #define SIZE 128*1024*1024  // 128 MB
> > >
> > >   int main(int argc, char *argv[])
> > >   {
> > >       while(1) {
> > >               volatile int *p = mmap(0, SIZE, PROT_READ | PROT_WRITE,
> > >                               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > >
> > >               memset((void *)p, 1, SIZE);
> > >               madvise((void *)p, SIZE, MADV_FREE);
> > >               /* redirty after MADV_FREE */
> > >               memset((void *)p, 1, SIZE);
> > >
> > >               clock_t start_time = clock();
> > >               madvise((void *)p, SIZE, MADV_PAGEOUT);
> > >               clock_t end_time = clock();
> > >
> > >               double elapsed_time = (double)(end_time - start_time) / CLOCKS_PER_SEC;
> > >               printf("Time taken by reclamation: %f seconds\n", elapsed_time);
> > >
> > >               munmap((void *)p, SIZE);
> > >       }
> > >       return 0;
> > >   }
> > >
> > > Testing results are as below,
> > > w/o patch:
> > > ~ # ./a.out
> > > Time taken by reclamation: 0.007300 seconds
> > > Time taken by reclamation: 0.007226 seconds
> > > Time taken by reclamation: 0.007295 seconds
> > > Time taken by reclamation: 0.007731 seconds
> > > Time taken by reclamation: 0.007134 seconds
> > > Time taken by reclamation: 0.007285 seconds
> > > Time taken by reclamation: 0.007720 seconds
> > > Time taken by reclamation: 0.007128 seconds
> > > Time taken by reclamation: 0.007710 seconds
> > > Time taken by reclamation: 0.007712 seconds
> > > Time taken by reclamation: 0.007236 seconds
> > > Time taken by reclamation: 0.007690 seconds
> > > Time taken by reclamation: 0.007174 seconds
> > > Time taken by reclamation: 0.007670 seconds
> > > Time taken by reclamation: 0.007169 seconds
> > > Time taken by reclamation: 0.007305 seconds
> > > Time taken by reclamation: 0.007432 seconds
> > > Time taken by reclamation: 0.007158 seconds
> > > Time taken by reclamation: 0.007133 seconds
> > > …
> > >
> > > w/ patch
> > >
> > > ~ # ./a.out
> > > Time taken by reclamation: 0.002124 seconds
> > > Time taken by reclamation: 0.002116 seconds
> > > Time taken by reclamation: 0.002150 seconds
> > > Time taken by reclamation: 0.002261 seconds
> > > Time taken by reclamation: 0.002137 seconds
> > > Time taken by reclamation: 0.002173 seconds
> > > Time taken by reclamation: 0.002063 seconds
> > > Time taken by reclamation: 0.002088 seconds
> > > Time taken by reclamation: 0.002169 seconds
> > > Time taken by reclamation: 0.002124 seconds
> > > Time taken by reclamation: 0.002111 seconds
> > > Time taken by reclamation: 0.002224 seconds
> > > Time taken by reclamation: 0.002297 seconds
> > > Time taken by reclamation: 0.002260 seconds
> > > Time taken by reclamation: 0.002246 seconds
> > > Time taken by reclamation: 0.002272 seconds
> > > Time taken by reclamation: 0.002277 seconds
> > > Time taken by reclamation: 0.002462 seconds
> > > …
> > >
> > > This patch significantly speeds up try_to_unmap_one() by allowing it
> > > to skip redirtied THPs without splitting the PMD.
> > >
> > > Suggested-by: Baolin Wang <baolin.wang@...ux.alibaba.com>
> > > Suggested-by: Lance Yang <ioworker0@...il.com>
> > > Signed-off-by: Barry Song <v-songbaohua@...o.com>
> > > ---
> > >   mm/huge_memory.c | 17 ++++++++++++++---
> > >   mm/rmap.c        | 11 ++++++++++-
> > >   2 files changed, 24 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index 3d3ebdc002d5..aea49f7125f1 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -3070,8 +3070,12 @@ static bool __discard_anon_folio_pmd_locked(struct vm_area_struct *vma,
> > >       int ref_count, map_count;
> > >       pmd_t orig_pmd = *pmdp;
> > >
> > > -     if (folio_test_dirty(folio) || pmd_dirty(orig_pmd))
> > > +     if (pmd_dirty(orig_pmd))
> > > +             folio_set_dirty(folio);
> > > +     if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) {
> > > +             folio_set_swapbacked(folio);
> > >               return false;
> > > +     }
> > >
> > >       orig_pmd = pmdp_huge_clear_flush(vma, addr, pmdp);
> > >
> > > @@ -3098,8 +3102,15 @@ static bool __discard_anon_folio_pmd_locked(struct vm_area_struct *vma,
> > >        *
> > >        * The only folio refs must be one from isolation plus the rmap(s).
> > >        */
> > > -     if (folio_test_dirty(folio) || pmd_dirty(orig_pmd) ||
> > > -         ref_count != map_count + 1) {
> > > +     if (pmd_dirty(orig_pmd))
> > > +             folio_set_dirty(folio);
> > > +     if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) {
> > > +             folio_set_swapbacked(folio);
> > > +             set_pmd_at(mm, addr, pmdp, orig_pmd);
> > > +             return false;
> > > +     }
> > > +
> > > +     if (ref_count != map_count + 1) {
> > >               set_pmd_at(mm, addr, pmdp, orig_pmd);
> > >               return false;
> > >       }
> > > diff --git a/mm/rmap.c b/mm/rmap.c
> > > index 3ef659310797..02c4e4b2cd7b 100644
> > > --- a/mm/rmap.c
> > > +++ b/mm/rmap.c
> > > @@ -1671,7 +1671,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> > >       DEFINE_FOLIO_VMA_WALK(pvmw, folio, vma, address, 0);
> > >       pte_t pteval;
> > >       struct page *subpage;
> > > -     bool anon_exclusive, ret = true;
> > > +     bool anon_exclusive, lazyfree, ret = true;
> > >       struct mmu_notifier_range range;
> > >       enum ttu_flags flags = (enum ttu_flags)(long)arg;
> > >       int nr_pages = 1;
> > > @@ -1724,9 +1724,18 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> > >               }
> > >
> > >               if (!pvmw.pte) {
> > > +                     lazyfree = folio_test_anon(folio) && !folio_test_swapbacked(folio);
> >
> > You've checked lazyfree here, so can we remove the duplicate check in
> > unmap_huge_pmd_locked()? Then the code should be:
> >
> >                 if (lazyfree && unmap_huge_pmd_locked(...))
> >                         goto walk_done;
>
>
> right. it seems unmap_huge_pmd_locked() only handles lazyfree pmd-mapped
> thp. so i guess the code could be:

Yep, it would be better to remove the duplicate check in
unmap_huge_pmd_locked(), and we can extend unmap_huge_pmd_locked()
in the future if needed.

Thanks,
Lance


>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index aea49f7125f1..c4c3a7896de4 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3131,11 +3131,10 @@ bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
>         VM_WARN_ON_FOLIO(!folio_test_pmd_mappable(folio), folio);
>         VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
>         VM_WARN_ON_ONCE(!IS_ALIGNED(addr, HPAGE_PMD_SIZE));
> +       VM_WARN_ON_FOLIO(!folio_test_anon(folio), folio);
> +       VM_WARN_ON_FOLIO(folio_test_swapbacked(folio), folio);
>
> -       if (folio_test_anon(folio) && !folio_test_swapbacked(folio))
> -               return __discard_anon_folio_pmd_locked(vma, addr, pmdp, folio);
> -
> -       return false;
> +       return __discard_anon_folio_pmd_locked(vma, addr, pmdp, folio);
>  }
>
>  static void remap_page(struct folio *folio, unsigned long nr, int flags)
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 02c4e4b2cd7b..72907eb1b8fe 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1671,7 +1671,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>         DEFINE_FOLIO_VMA_WALK(pvmw, folio, vma, address, 0);
>         pte_t pteval;
>         struct page *subpage;
> -       bool anon_exclusive, lazyfree, ret = true;
> +       bool anon_exclusive, ret = true;
>         struct mmu_notifier_range range;
>         enum ttu_flags flags = (enum ttu_flags)(long)arg;
>         int nr_pages = 1;
> @@ -1724,18 +1724,16 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>                 }
>
>                 if (!pvmw.pte) {
> -                       lazyfree = folio_test_anon(folio) && !folio_test_swapbacked(folio);
> -
> -                       if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd,
> -                                                 folio))
> -                               goto walk_done;
> -                       /*
> -                        * unmap_huge_pmd_locked has either already marked
> -                        * the folio as swap-backed or decided to retain it
> -                        * due to GUP or speculative references.
> -                        */
> -                       if (lazyfree)
> +                       if (folio_test_anon(folio) && !folio_test_swapbacked(folio)) {
> +                               if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd, folio))
> +                                       goto walk_done;
> +                               /*
> +                                * unmap_huge_pmd_locked has either already marked
> +                                * the folio as swap-backed or decided to retain it
> +                                * due to GUP or speculative references.
> +                                */
>                                 goto walk_abort;
> +                       }
>
>                         if (flags & TTU_SPLIT_HUGE_PMD) {
>                                 /*
>
> >
> > >                       if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd,
> > >                                                 folio))
> > >                               goto walk_done;
> > > +                     /*
> > > +                      * unmap_huge_pmd_locked has either already marked
> > > +                      * the folio as swap-backed or decided to retain it
> > > +                      * due to GUP or speculative references.
> > > +                      */
> > > +                     if (lazyfree)
> > > +                             goto walk_abort;
> > >
> > >                       if (flags & TTU_SPLIT_HUGE_PMD) {
> > >                               /*

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ