amd64/arm64 pmap: Stop requiring the accessed bit for superpage promotion
ClosedPublic
Actions

Authored by alc on Jun 9 2023, 6:04 PM.

Details

Reviewers

kib
markj
manu

Commits

rG34eeabff5a86: amd64/arm64 pmap: Stop requiring the accessed bit for superpage promotion

Summary

Stop requiring all of the PTEs to have the accessed bit set for superpage promotion to occur. Given that change, add support for promotion to pmap_enter_quick(), which does not set the accessed bit in the PTE that it creates.

Since the final mapping within a superpage-aligned and sized region of a memory-mapped file is typically created by a call to pmap_enter_quick(), we now achieve promotions in circumstances where they did not occur before, for example, this mapping within the X server's address space:

1745        0x844400000        0x84766f000 r-- 12911 26232   4   2 CNS-- vn /usr/local/llvm15/lib/libLLVM-15.so

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

alc created this revision.Jun 9 2023, 6:04 PM

Herald added a reviewer: manu. · View Herald TranscriptJun 9 2023, 6:04 PM

Herald added subscribers: emaste, andrew. · View Herald Transcript

alc requested review of this revision.Jun 9 2023, 6:04 PM

alc edited the summary of this revision. (Show Details)Jun 9 2023, 6:08 PM

markj accepted this revision.Jun 9 2023, 9:21 PM

markj added inline comments.

sys/amd64/amd64/pmap.c
4073	There should be a semicolon instead of a comma after "PG_A".
4080	I'd perhaps assert that `promoted \|\| !allpte_PG_A_set` is true (or use a tri-state variable instead of two bools).
sys/arm64/arm64/pmap.c
4026	There should be a semicolon after ATTR_AF.

This revision is now accepted and ready to land.Jun 9 2023, 9:21 PM

kib added inline comments.Jun 9 2023, 11:30 PM

sys/amd64/amd64/pmap.c
4082	IMO it would be useful to document special use of the mpte valid field and enumerate the values.
7756	Could this block and the same block from pmap_enter() made into a function?

alc marked 3 inline comments as done.Jun 10 2023, 6:00 AM

alc added inline comments.Jun 10 2023, 6:54 AM

sys/amd64/amd64/pmap.c
4082	I'm happy to, but where, vm_page.h? The above comment already describes what values are assigned to the PTP's valid field under different circumstances.
7756	Each of the three instances has a slightly different body. This one may have to calculate the PDE. The others do not. The EPT instance increments an EPT-specific counter. (The EPT instance could also be more different than it is now in that it currently pointlessly tests mpte == NULL and before that va < VM_MAXUSER_ADDRESS.)

kib accepted this revision.Jun 10 2023, 8:32 PM

kib added inline comments.

sys/amd64/amd64/pmap.c
4082	It is MD, so should be somewhere in pmap.h or pmap.c. Perhaps together with the explanation of the pindex use for ptps, and wirings. Currently all that comments are spread over the pmap.c, like pmap_unwire_ptp() and pmap_allocpte(_nosleep()).
7756	I might try to look at this after your commit.

Address Mark's comments.

In pmap_enter_quick_locked(), when promotion is attempted, return NULL, so that the unmapped PTP isn't passed to pmap_enter_quick_locked() by pmap_enter_object().

Add an EPT-related KASSERT to pmap_promote_pde().

This revision now requires review to proceed.Jun 11 2023, 8:19 AM

alc added inline comments.Jun 11 2023, 6:03 PM

sys/amd64/amd64/pmap.c

7756

Since we expect pmap_ps_enabled(pmap) to be true, I think that the following makes sense.

diff --git a/sys/amd64/amd64/pmap.c b/sys/amd64/amd64/pmap.c
index 3cb02a4f9daa..a36bbe16bee8 100644
--- a/sys/amd64/amd64/pmap.c
+++ b/sys/amd64/amd64/pmap.c
@@ -436,7 +436,7 @@ pt_entry_t pg_nx;
 static SYSCTL_NODE(_vm, OID_AUTO, pmap, CTLFLAG_RD | CTLFLAG_MPSAFE, 0,
     "VM/pmap parameters");
 
-static int pg_ps_enabled = 1;
+static int __read_frequently pg_ps_enabled = 1;
 SYSCTL_INT(_vm_pmap, OID_AUTO, pg_ps_enabled, CTLFLAG_RDTUN | CTLFLAG_NOFETCH,
     &pg_ps_enabled, 0, "Are large page mappings enabled?");
 
@@ -6865,6 +6865,10 @@ pmap_promote_pde(pmap_t pmap, pd_entry_t *pde, vm_offset_t va, vm_page_t mpte,
        pt_entry_t allpte_PG_A, PG_A, PG_G, PG_M, PG_PKU_MASK, PG_RW, PG_V;
        int PG_PTE_CACHE;
 
+       PMAP_LOCK_ASSERT(pmap, MA_OWNED);
+       if (!pmap_ps_enabled(pmap))
+               return;
+
        PG_A = pmap_accessed_bit(pmap);
        PG_G = pmap_global_bit(pmap);
        PG_M = pmap_modified_bit(pmap);
@@ -6873,8 +6877,6 @@ pmap_promote_pde(pmap_t pmap, pd_entry_t *pde, vm_offset_t va, vm_page_t mpte,
        PG_PKU_MASK = pmap_pku_mask_bit(pmap);
        PG_PTE_CACHE = pmap_cache_mask(pmap, 0);
 
-       PMAP_LOCK_ASSERT(pmap, MA_OWNED);
-
        /*
         * Examine the first PTE in the specified PTP.  Abort if this PTE is
         * ineligible for promotion due to hardware errata, invalid, or does
@@ -7391,7 +7393,6 @@ pmap_enter(pmap_t pmap, vm_offset_t va, vm_page_t m, vm_prot_t prot,
         * populated, then attempt promotion.
         */
        if ((mpte == NULL || mpte->ref_count == NPTEPG) &&
-           pmap_ps_enabled(pmap) &&
            (m->flags & PG_FICTITIOUS) == 0 &&
            vm_reserv_level_iffullpop(m) == 0)
                pmap_promote_pde(pmap, pde, va, mpte, &lock);
@@ -7782,7 +7783,6 @@ pmap_enter_quick_locked(pmap_t pmap, vm_offset_t va, vm_page_t m,
         * attempt promotion.
         */
        if ((mpte == NULL || mpte->ref_count == NPTEPG) &&
-           pmap_ps_enabled(pmap) &&
            (m->flags & PG_FICTITIOUS) == 0 &&
            vm_reserv_level_iffullpop(m) == 0) {
                if (pde == NULL)
@@ -10359,7 +10359,6 @@ pmap_emulate_accessed_dirty(pmap_t pmap, vm_offset_t va, int ftype)
        m = PHYS_TO_VM_PAGE(*pte & PG_FRAME);
 
        if ((mpte == NULL || mpte->ref_count == NPTEPG) &&
-           pmap_ps_enabled(pmap) &&
            (m->flags & PG_FICTITIOUS) == 0 &&
            vm_reserv_level_iffullpop(m) == 0) {
                pmap_promote_pde(pmap, pde, va, mpte, &lock);