| 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
| ||
|
Message-Id: <20250114040914.9986-1-21cnbao@gmail.com>
Date: Tue, 14 Jan 2025 17:09:14 +1300
From: Barry Song <21cnbao@...il.com>
To: baolin.wang@...ux.alibaba.com
Cc: 21cnbao@...il.com,
akpm@...ux-foundation.org,
chrisl@...nel.org,
david@...hat.com,
ioworker0@...il.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 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:
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