linux-kernel - Re: [PATCH v2 0/6] mm: Optimize mseal checks

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: <CABi2SkXMs0uhYTC4XhYPrLGO5z=1MnxQB2OJ5a-Jtg32Fh21aQ@mail.gmail.com>
Date: Thu, 15 Aug 2024 14:11:14 -0700
From: Jeff Xu <jeffxu@...omium.org>
To: Pedro Falcato <pedro.falcato@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, 
	Vlastimil Babka <vbabka@...e.cz>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org, oliver.sang@...el.com, 
	torvalds@...ux-foundation.org, jeffxu@...gle.com, 
	Michael Ellerman <mpe@...erman.id.au>
Subject: Re: [PATCH v2 0/6] mm: Optimize mseal checks

On Wed, Aug 7, 2024 at 2:13 PM Pedro Falcato <pedro.falcato@...il.com> wrote:
>
> [based on mm-unstable, 98808d08]
>
> Optimize mseal checks by removing the separate can_modify_mm() step, and
> just doing checks on the individual vmas, when various operations are
> themselves iterating through the tree. This provides a nice speedup and restores
> performance parity with pre-mseal[3].
>
> This series also depends on (and will eventually very slightly conflict with)
> the powerpc series that removes arch_unmap[2].
>
> will-it-scale mmap1_process[1] -t 1 results:
>
> commit 3450fe2b574b4345e4296ccae395149e1a357fee:
>
> min:277605 max:277605 total:277605
> min:281784 max:281784 total:281784
> min:277238 max:277238 total:277238
> min:281761 max:281761 total:281761
> min:274279 max:274279 total:274279
> min:254854 max:254854 total:254854
> measurement
> min:269143 max:269143 total:269143
> min:270454 max:270454 total:270454
> min:243523 max:243523 total:243523
> min:251148 max:251148 total:251148
> min:209669 max:209669 total:209669
> min:190426 max:190426 total:190426
> min:231219 max:231219 total:231219
> min:275364 max:275364 total:275364
> min:266540 max:266540 total:266540
> min:242572 max:242572 total:242572
> min:284469 max:284469 total:284469
> min:278882 max:278882 total:278882
> min:283269 max:283269 total:283269
> min:281204 max:281204 total:281204
>
> After this patch set:
>
> min:280580 max:280580 total:280580
> min:290514 max:290514 total:290514
> min:291006 max:291006 total:291006
> min:290352 max:290352 total:290352
> min:294582 max:294582 total:294582
> min:293075 max:293075 total:293075
> measurement
> min:295613 max:295613 total:295613
> min:294070 max:294070 total:294070
> min:293193 max:293193 total:293193
> min:291631 max:291631 total:291631
> min:295278 max:295278 total:295278
> min:293782 max:293782 total:293782
> min:290361 max:290361 total:290361
> min:294517 max:294517 total:294517
> min:293750 max:293750 total:293750
> min:293572 max:293572 total:293572
> min:295239 max:295239 total:295239
> min:292932 max:292932 total:292932
> min:293319 max:293319 total:293319
> min:294954 max:294954 total:294954
>
> This was a Completely Unscientific test but seems to show there were around 5-10% gains on ops per second.
>
It is likely true that the test " was Completely Unscientific".
Without the necessary information, such as stddiv, stderr, and large
enough sample size, it is impossible to prove the test can reasonably
measure the impact of the patch. Unless you add that information, it
would be prudent to omit this test data from the cover letter.

> Oliver performed his own tests and showed[3] a similar ~5% gain in them.
>
> [1]: mmap1_process does mmap and munmap in a loop. I didn't bother testing multithreading cases.
> [2]: https://lore.kernel.org/all/20240807124103.85644-1-mpe@ellerman.id.au/
> [3]: https://lore.kernel.org/all/ZrMMJfe9aXSWxJz6@xsang-OptiPlex-9020/
> Link: https://lore.kernel.org/all/202408041602.caa0372-oliver.sang@intel.com/
>
> Changes in v2:
>  - Rebased on top of mm-unstable
>  - Removed a superfluous check in mremap (Jeff Xu)
Thanks for taking this suggestion. When I posted that comment, I was
studying the mremap change in V2, I'm not 100% sure if it is possible,
or reasonable the code is written that way (even before mseal is
added.)

This week, I wrote more self-tests [1] for the mremap to reason about
the code handling of mremap across the VMA boundary, as part of an
effort to test your patch.

During the process I realized that we can probably improve the mremap
code further, even without considering the sealing,  so I made a refactor
patch for mremap. [2].

Testing is 90% of the time, if not more, when I modify the kernel
code. If you agree with my refactor on mremap, please feel free to
rebase  or include it as part of your patch series.

Thanks
-Jeff

[1] https://lore.kernel.org/linux-mm/20240814071424.2655666-2-jeffxu@chromium.org/
[2] https://lore.kernel.org/linux-mm/20240814071424.2655666-3-jeffxu@chromium.org/


> Pedro Falcato (6):
>   mm: Move can_modify_vma to mm/internal.h
>   mm/munmap: Replace can_modify_mm with can_modify_vma
>   mm/mprotect: Replace can_modify_mm with can_modify_vma
>   mm/mremap: Replace can_modify_mm with can_modify_vma
>   mseal: Replace can_modify_mm_madv with a vma variant
>   mm: Remove can_modify_mm()
>
>  mm/internal.h | 30 ++++++++++++++++++++--------
>  mm/madvise.c  | 13 +++---------
>  mm/mmap.c     | 13 +-----------
>  mm/mprotect.c | 12 +++--------
>  mm/mremap.c   | 30 ++++------------------------
>  mm/mseal.c    | 55 ++++-----------------------------------------------
>  mm/vma.c      | 23 ++++++++++-----------
>  7 files changed, 49 insertions(+), 127 deletions(-)
>
>
> base-commit: 98808d08fc0f78ee638e0c0a88020fbbaf581ec6
> --
> 2.46.0
>
>