In D20380#440729, @kib wrote:

In D20380#440728, @alc wrote:

Everything here looks good to me.

Does this mean 'clean to commit' ? I think testing with D20266 can wait for D20266 itself.

Everything here looks good to me.

In D20380#440650, @kib wrote:

In D20380#440642, @alc wrote:

In D20380#440609, @kib wrote:

I am not sure why do check that KERNend != round_2mpage() before rounding, IMO it simply less lines to not do that.

Are you asking about this snippet?

if (*firstaddr < round_2mpage(KERNend))
        *firstaddr = round_2mpage(KERNend);

Yes.

In D20380#440609, @kib wrote:

In D20380#440605, @alc wrote:

I _think_ that what you mean is that we should connect all preallocated page tables to directories, but only pre-promote to the rounded KERNend, and store in radix only page tables pages which were shadowed by the manual promotion.

Yes, except that the way that the loop is written, we don't need to round KERNend for correctness. I would only round it if you think that doing so makes it less likely that someone will misinterpret the code. Here are all of the changes that I would make.

I do not think that the check needs changing.
Also I see that indeed page table pages do not need non-zero initialization after we switched to PG_PROMOTED test.
I am not sure why do check that KERNend != round_2mpage() before rounding, IMO it simply less lines to not do that.

I _think_ that what you mean is that we should connect all preallocated page tables to directories, but only pre-promote to the rounded KERNend, and store in radix only page tables pages which were shadowed by the manual promotion.

Yes, except that the way that the loop is written, we don't need to round KERNend for correctness. I would only round it if you think that doing so makes it less likely that someone will misinterpret the code. Here are all of the changes that I would make.

Index: amd64/amd64/pmap.c
===================================================================
--- amd64/amd64/pmap.c  (revision 348267)
+++ amd64/amd64/pmap.c  (working copy)
@@ -1338,7 +1338,6 @@ static void
 create_pagetables(vm_paddr_t *firstaddr)
 {
        int i, j, ndm1g, nkpdpe, nkdmpde;
-       pt_entry_t *pt_p;
        pd_entry_t *pd_p;
        pdp_entry_t *pdp_p;
        pml4_entry_t *p4_p;
@@ -1399,20 +1398,21 @@ create_pagetables(vm_paddr_t *firstaddr)
        KPTphys = allocpages(firstaddr, nkpt);
        KPDphys = allocpages(firstaddr, nkpdpe);

I think that the changes within pmap_demote_pde_locked(), including the two given in my comments, are correct and should be committed. The same changes should also be made to the i386 pmap.

Can you separate the pmap_demote_pde_locked() changes from the create_pagetables()/pmap_init() changes, i.e., create a separate patch.

Also, I never intended for promotion and demotion on the kernel pmap to create an inconsistency in how we maintain the wire count on kernel page table pages. Switching to testing (oldpte & PG_PROMOTED) == 0 would allow for eliminating that inconsistency.

Index: amd64/amd64/pmap.c
===================================================================
--- amd64/amd64/pmap.c  (revision 348078)
+++ amd64/amd64/pmap.c  (working copy)
@@ -4537,8 +4537,10 @@ pmap_demote_pde_locked(pmap_t pmap, pd_entry_t *pd
                            " in pmap %p", va, pmap);
                        return (FALSE);
                }
-               if (va < VM_MAXUSER_ADDRESS)
+               if (va < VM_MAXUSER_ADDRESS) {
+                       mpte->wire_count = NPTEPG;
                        pmap_resident_count_inc(pmap, 1);
+               }
        }
        mptepa = VM_PAGE_TO_PHYS(mpte);
        firstpte = (pt_entry_t *)PHYS_TO_DMAP(mptepa);
@@ -4553,10 +4555,8 @@ pmap_demote_pde_locked(pmap_t pmap, pd_entry_t *pd
        /*
         * If the page table page is new, initialize it.
         */
-       if (mpte->wire_count == 1) {
-               mpte->wire_count = NPTEPG;
+       if ((oldpde & PG_PROMOTED) == 0)
                pmap_fill_ptp(firstpte, newpte);
-       }
        KASSERT((*firstpte & PG_FRAME) == (newpte & PG_FRAME),
            ("pmap_demote_pde: firstpte and newpte map different physical"
            " addresses"));

What remains here has already been committed, but with LK_SHARED passed to vn_lock(). This can be closed.

What are the errors that Peter reported?

I did a careful review of these changes a while ago, and I'm convinced that all of the issues that we discussed last fall are dealt with.

In D20256#437068, @pho wrote:

I ran tests for 18 hours, including a buildworld.
Last vm.reserv.broken value was 247091.
No problems seen.

I think that it would be worthwhile to write a test program that creates and destroys a large number of anonymous mappings and then, in particular, profile that program for data cache misses with pmcstat. The one tricky thing about writing this program is that you would need to ensure that the anonymous mappings can't coalesce by ensuring that there is always a gap between two mappings.

I think that there is a better approach than what you are attempting here. Once you have the patch that you describe as "My patch to alter the behavior of SO_REUSEPORT_LB listen sockets ...", the thread executing sendfile(2) will be running on the same domain as the socket. Recall Kostik's first comment, "Vm objects can have domain policies assigned, and object policy trumps a thread policy, if present." In other words, if you don't define an object policy, the page allocations will be governed by the thread's policy. A thread policy that says allocate from the local domain should achieve your desired outcome.

I question if there shouldn't be an explicit statement somewhere in this man page that this feature can be used by an application to implement memory "safety" mechanisms, but not "security". Because the RDPKRU and WRPKRU instructions are not privileged, a compromised application could always change the access rights. This is different from the protection key mechanisms on other architectures such as 32-bit ARM.

Ask Peter to test this patch.

In D18058#388822, @pho wrote:

D18058.id50652.diff ran the stress2/misc/stealer.sh test scenario for 11 hours before freezing up: https://people.freebsd.org/~pho/stress/log/dougm015.txt

In D18058#389007, @dougm_rice.edu wrote:

Allow bighint to be updated for a meta-node where the cursor lies ahead of all nonempty children, or even when there are no nonempty children, since blist_alloc depends on a reduction of the root bighint to allow a failed allocation to finish.
Alternatively, we could change the test in blist_alloc to depend on something other than bighint. This is the only case where having a too-large bighint breaks things.

Yes, this is clearly a bug. Hopefully, it is the one that Peter is tripping over.

In D16712#385630, @alc wrote:

In D16712#385624, @kib wrote:

Prefer to remove the inline keyword.

Okay, I'm going to commit this change in a few hours without the inline keyword.

In D16712#385624, @kib wrote:

Prefer to remove the inline keyword.

I like the introduction of the #define, but I'd to rename it to have the suffix _MASK. The point being that when a reader sees a use case as opposed to the definition the reader doesn't think that it's yet another flag.

On a somewhat related note, the version of jemalloc that we use in HEAD and 12.x is frequently allocating small, unaligned chunks from the kernel. In contrast, older versions were (always?) allocating large, aligned chunks. You can see the effects here: P196. Clang reads source files using mmap(2), and between most mapped files there is a sliver of anonymous heap. For "buildworld", I estimate that this has reduced superpage promotions by 1/3. The explicit clustering of anonymous memory allocations by this patch likely cures this problem.

In D5603#168859, @lattera-gmail.com wrote:

In D5603#168856, @emaste wrote:

ASR has a measurable performance hit; ASLR does not.

I'd be quite interested in the detail here if you can provide a link or reference to this.

How expensive is pulling from the entropy pool at least once, at most five times, per call to mmap?

Later today, I'm going to commit the bits here that are orthogonal to the main point of this patch. In other words, the bits that migrated here from an earlier, abandoned review that Kostik had asked about last week (if I recall correctly).

Aside from my one comment, this looks fine. Once that comment is addressed, consider this change "Accepted" by me.

arm64 is missing code from pmap_remove_l2() that reinstalls the (empty) page table page in the kernel pmap.

In regards to

mt = PHYS_TO_VM_PAGE(*pde & PG_FRAME);

the earlier call to pmap_remove_pde() and its helper function, pmap_remove_kernel_pde(), reinstalled the page table page that mt references, so this is correct code.

I would also suggest mentioning the use of this new flag in the comment at the top of the struct vm_map definition where we talk about min and max offset.