Paths

Table of Contentst

arm64 pmap: Avoid redundant icache synchronization using PGA_EXECUTABLE
AcceptedPublic
Actions

Authored by alc on Sun, Sep 20, 10:31 PM.

Details

Reviewers

kib
markj
andrew
manu

Summary

Creating an executable user-space mapping to write-back memory synchronizes the icache with the page's contents, whether or not those contents have changed since the previous synchronization. Use the page flag PGA_EXECUTABLE to record that the icache has been synchronized with a managed page's contents and that the page has no writable mappings. When the flag is set, the creation of another executable mapping of the page can skip the synchronization. The flag is cleared when a writable mapping of the page is created, using a single atomic operation that keeps PGA_WRITEABLE and PGA_EXECUTABLE from ever being simultaneously set, and when the page's last mapping is destroyed.

(This is stricter than mmu_oea64's use of the same flag, PGA_EXECUTABLE. Specifically, mmu_oea64 allows PGA_WRITEABLE and PGA_EXECUTABLE to be set simultaneously.)

Assisted-by: Claude Code (Fable 5.1)

Test Plan

I've tested this patch on an EC2 a1.4xlarge (16x Cortex A72) machine to see the greatest impact. (This is the entire machine, so there isn't any variance due to other VMs on the machine.) I added a COUNTER_U64 to track icache flushes by the pmap. A buildworld performed 229.09M icache synchronizations per run before this change and 8.33M after it, a 96.4% reduction. System time fell from 5097 s to 4502 s (-11.7%), user time fell by 1.1%, and wall-clock time fell by 1.5%.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

alc created this revision.Sun, Sep 20, 10:31 PM

Herald added a reviewer: andrew. · View Herald TranscriptSun, Sep 20, 10:31 PM

Herald added a reviewer: andrew. · View Herald Transcript

Herald added a reviewer: manu. · View Herald Transcript

Herald added a subscriber: emaste. · View Herald Transcript

alc requested review of this revision.Sun, Sep 20, 10:31 PM

kib accepted this revision.Mon, Sep 21, 12:19 PM

kib added inline comments.

sys/arm64/arm64/pmap.c
5690	Should we assert there and in loop that _WRITEABLE and _EXECUTABLE are not set simultaneously?

This revision is now accepted and ready to land.Mon, Sep 21, 12:19 PM

markj added inline comments.Wed, Sep 23, 3:25 PM

sys/arm64/arm64/pmap.c
5634	I wonder if we should use a more precise name than PGA_EXECUTABLE. If one knows what PGA_WRITEABLE means, then it's natural to conclude that PGA_EXECUTABLE is set on a page iff it has at least one executable mapping, but that's not quite true.
6573	This is potentially syncing the entire 64KB range more than once, no?

alc marked an inline comment as done.Wed, Sep 23, 4:32 PM

alc added inline comments.

sys/arm64/arm64/pmap.c
5634	Your point is valid, but I'm skeptical that it's worth defining a new PGA_* flag. If anything, I would argue for replacing PGA_EXECUTABLE in vm_page.h with PGA_"machine dependent" and have mmu_oea64 and arm64 each provide their own local definition in pmap.c. Then, mmu_oea64 and arm64 could use different names. This is similar in spirit to what I did for the flags to pmap_enter().
5690	I'm happy to add those KASSERTs.
6573	I'm not sure that I understand what you are getting at. A given call to pmap_enter_l3c() will only call "sync icache" once, because of the below `break`, but a later call will call "sync icache" again if PGA_EXECUTABLE was not set on all of the pages. I'm not convinced that in practice it's worth being more selective about which pages get written back from the dcache and flushed from the icache on that later mapping.

markj added inline comments.Wed, Sep 23, 4:37 PM

sys/arm64/arm64/pmap.c
5634	Yes, sorry, I didn't mean to suggest we should add a new flag instead. I was imagining something like `#define PGA_EXECUTABLE_CLEAN PGA_EXECUTABLE` at the top of this file. Your proposal is better.
6573	I just completely missed the `break` below.

alc marked an inline comment as done.Wed, Sep 23, 4:47 PM

alc added inline comments.

sys/arm64/arm64/pmap.c
5634	Suggestions for names for these new flags would be welcomed.

markj added inline comments.Wed, Sep 23, 5:01 PM

sys/arm64/arm64/pmap.c
5634	PGA_ICACHE_SYNCED or PGA_ICACHE_SYNCED, perhaps?

alc added a parent revision: D59995: vm_page: Replace PGA_EXECUTABLE with PGA_MACHDEP0004.Fri, Sep 25, 4:31 AM

PGA_ICACHE_SYNCED and KASSERTs

This revision now requires review to proceed.Fri, Sep 25, 6:03 AM

kib accepted this revision.Fri, Sep 25, 6:14 AM

This revision is now accepted and ready to land.Fri, Sep 25, 6:14 AM

markj accepted this revision.Fri, Sep 25, 3:07 PM

markj added inline comments.

sys/arm64/arm64/pmap.c
5634	... the first one was supposed to be PGA_ICACHE_CLEAN, but I think _SYNCED makes more sense.

Do we have somewhere we can document the contract with userspace & when it needs to manage the cache? e.g. removing write & enabling execute on a page will cause the kernel to sync the cache, but having both together means userspace needs to perform a sync operation.

I've also tested the prior version of this patch on a 32-core machine (c6g.8xlarge), where icache coherence is maintained by the hardware and the sync operation is simply a dsb and an isb. I just wanted to make sure that the patch didn't negatively effect performance on such machines. I compared 18 buildworld runs on HEAD with 22 runs using this patch. The kernel was -NODEBUG, MALLOC PRODUCTION was enabled, and LLVM assertions were disabled. System time actually fell by 7.8 seconds, from 2,060 to 2,052 seconds, because we avoided a couple hundred million dsb and isb instructions at the cost of the flag maintenance. That is 0.4%, which is four times larger than the random error you would expect. User time fell by 21 seconds, or 0.05%, which is probably real but too small to assert a similar claim relative to the random error. Wall-clock time did not change measurably.

sys/arm64/arm64/pmap.c

6354–6360

Until we start doing this

diff --git a/lib/clang/libclang/Makefile b/lib/clang/libclang/Makefile
index 952e3984951c..26c68fb73f40 100644
--- a/lib/clang/libclang/Makefile
+++ b/lib/clang/libclang/Makefile
@@ -1443,4 +1443,8 @@ CLEANFILES+=      ${TGHDRS} ${TGHDRS:C/$/.d/}
 
 .include "../clang.build.mk"
 
+.if ${MACHINE_CPUARCH} == "aarch64" || ${MACHINE_CPUARCH} == "amd64"
+LDFLAGS+=      -Wl,-z,max-page-size=0x200000
+.endif
+
 .include <bsd.lib.mk>
diff --git a/lib/clang/libllvm/Makefile b/lib/clang/libllvm/Makefile
index a234c73b15b9..4c9aaa285e01 100644
--- a/lib/clang/libllvm/Makefile
+++ b/lib/clang/libllvm/Makefile
@@ -2502,4 +2502,9 @@ CLEANFILES+=      ${TGHDRS} ${TGHDRS:C/$/.d/}
 CLEANFILES+=   ${GENSRCS} ${GENSRCS:C/$/.d/}
 
 .include "../llvm.build.mk"
+
+.if ${MACHINE_CPUARCH} == "aarch64" || ${MACHINE_CPUARCH} == "amd64"
+LDFLAGS+=      -Wl,-z,max-page-size=0x200000
+.endif
+
 .include <bsd.lib.mk>

on large executables, the 2nd, 3rd, etc. segments are always misaligned, so we can't have 2 MB code superpages. Moreover, on Ryzen processors the automatic PTE coalescing can't work on the code either.

Here is a link to the paper I published a while back on PTE coalescing. At that time, the changes in Linux to exploit small folios were just beginning to get merged.

alc added inline comments.Fri, Sep 25, 4:47 PM

sys/arm64/arm64/pmap.c
6583–6593	One of my worries about this patch is that this loop and the similar loop in pmap_enter_l2() introduce touches to all of the vm_page structures making up the superpage. That is one of the reasons why I did the tests on the c6g.8xlarge, where coherence is supported by the hardware. In this function, the cache lines backing the vm_page structures are already "hot" because we just created PV entries for each of those pages. 2 MB superpages will be a different story, because we only create a single PV entry. However, as I explained earlier, that case is currently moot. But, I have prototyped a patch that would introduce a flag to the 2 MB PV entry that says, "all 512 pages have been checked; no need to do it again."

In D59865#1377062, @andrew wrote:

Do we have somewhere we can document the contract with userspace & when it needs to manage the cache? e.g. removing write & enabling execute on a page will cause the kernel to sync the cache, but having both together means userspace needs to perform a sync operation.

There are more changes to come. I will write up a description once everything has landed.

Revision Contents
Changeset List

Path

Size

sys/

arm64/

pmap.c

212 lines

Diff 187304

View Options

arm64 pmap: Avoid redundant icache synchronization using PGA_EXECUTABLEAcceptedPublicActions