mm/mprotect: try avoiding write faults for exclusive anonymous pages when changing protection

Similar to our MM_CP_DIRTY_ACCT handling for shared, writable mappings, we can try mapping anonymous pages in a private writable mapping writable if they are exclusive, the PTE is already dirty, and no special handling applies. Mapping the anonymous page writable is essentially the same thing the write fault handler would do in this case.

Special handling is required for uffd-wp and softdirty tracking, so take care of that properly. Also, leave PROT_NONE handling alone for now; in the future, we could similarly extend the logic in do_numa_page() or use pte_mk_savedwrite() here.

While this improves mprotect(PROT_READ)+mprotect(PROT_READ|PROT_WRITE) performance, it should also be a valuable optimization for uffd-wp, when un-protecting.

This has been previously suggested by Peter Collingbourne in [1], relevant in the context of the Scudo memory allocator, before we had PageAnonExclusive.

This commit doesn't add the same handling for PMDs (i.e., anonymous THP, anonymous hugetlb); benchmark results from Andrea indicate that there are minor performance gains, so it's might still be valuable to streamline that logic for all anonymous pages in the future.

As we now also set MM_CP_DIRTY_ACCT for private mappings, let's rename it to MM_CP_TRY_CHANGE_WRITABLE, to make it clearer what's actually happening.

Micro-benchmark courtesy of Andrea:

===
#define _GNU_SOURCE
#include <sys/mman.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <unistd.h>

#define SIZE (1024*1024*1024)

int main(int argc, char *argv[])

{
	char *p;
	if (posix_memalign((void **)&p, sysconf(_SC_PAGESIZE)*512, SIZE))
		perror("posix_memalign"), exit(1);
	if (madvise(p, SIZE, argc > 1 ? MADV_HUGEPAGE : MADV_NOHUGEPAGE))
		perror("madvise");
	explicit_bzero(p, SIZE);
	for (int loops = 0; loops < 40; loops++) {
		if (mprotect(p, SIZE, PROT_READ))
			perror("mprotect"), exit(1);
		if (mprotect(p, SIZE, PROT_READ|PROT_WRITE))
			perror("mprotect"), exit(1);
		explicit_bzero(p, SIZE);
	}
}

===

Results on my Ryzen 9 3900X:

Stock 10 runs (lower is better): AVG 6.398s, STDEV 0.043
Patched 10 runs (lower is better): AVG 3.780s, STDEV 0.026

===

[1] https://lkml.kernel.org/r/[email protected]

---

v2 -> v3:
* Check some VMA flags early and glue them to MM_CP_TRY_CHANGE_WRITABLE
* MM_CP_DIRTY_ACCT -> MM_CP_TRY_CHANGE_WRITABLE
* Use benchmark from Andrea and add updated results
* Add reference to Peter's patch
* Rephrase patch description + code comments

v1 -> v2:
* Rebased on v5.19-rc1
* Rerun benchmark
* Fix minor spelling issues in subject+description
* Drop IS_ENABLED(CONFIG_MEM_SOFT_DIRTY) check
* Move pte_write() check into caller

---
 include/linux/mm.h |  8 +++--
 mm/mprotect.c      | 77 +++++++++++++++++++++++++++++++++++++---------
 2 files changed, 68 insertions(+), 17 deletions(-)

index bc8f326be0ce..ae66d26dd708 100644
@@ -1962,8 +1962,12 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,
  * for now all the callers are only use one of the flags at the same
  * time.
  */
-/* Whether we should allow dirty bit accounting */
-#define  MM_CP_DIRTY_ACCT                  (1UL << 0)
+/*
+ * Whether we should manually check if we can map individual PTEs writable,
+ * because something (e.g., COW, uffd-wp) blocks that from happening for all
+ * PTEs automatically in a writable mapping.
+ */
+#define  MM_CP_TRY_CHANGE_WRITABLE	   (1UL << 0)
 /* Whether this protection change is for NUMA hints */
 #define  MM_CP_PROT_NUMA                   (1UL << 1)
 /* Whether this change is for write protecting */

base-commit: b13baccc3850ca8b8cccbf8ed9912dbaa0fdf7f3

2.35.3

I'm personally not sure why DIRTY_ACCT cannot be applied to private mappings; it sounds not only for shared but a common thing. I also don't know whether "change writable" could be misread too anyway. Say, we're never changing RO->RW mappings with this flag, but only try to unprotect the page proactively when proper, from that POV Nadav's suggestion seems slightly better on using "unprotect".

No strong opinion, the patch looks correct to me, and thanks for providing the new test results,

Peter Xu

TBH, I think the name is just absolutely unclear in that context.

write unprotection is a change from RO->RW, so I don't immediately see the difference.

Anyhow, I don't like the sounding of TRY_WRITE_UNPROTECT.

I made it match the function name that I had:

MM_CP_TRY_CHANGE_WRITABLE
-> !pte_write()?
-> can_change_pte_writable() ?
->pte_mkwrite()

Maybe MM_CP_TRY_MAKE_WRITABLE / MM_CP_TRY_MAKE_PTE_WRITABLE is clearer?

Open for suggestions because I'm apparently not the bast at naming things either.

Thanks Peter!

Thanks,

David / dhildenb

In my view "unprotect a pte" is only a subset of "grant pte write permission", since: "unprotect" has a prerequisite that it used to be "protected" so that's why we can unprotect. Aka, in mm term that's only when VM_WRITE set.

So basically it is a hint that we're only working on VM_WRITE regions, where I thought "unprotect" was slightly better.

Me neither. I don't have a strong opinion anyway, and frankly indeed the old naming is not great either to me. Maybe there's better thoughts.

Thanks,

Peter Xu