As an aside, I want to ask why pmap_update_entry() both blocks interrupts and performs critical_enter()? Doesn't the former suffice?

On a translation fault within the kernel address space, perform a lock-free page table walk that handles transient invalidations due to superpage promotion or demotion.

Here is a patch based on Mark's suggestion.

Index: arm64/arm64/pmap.c
===================================================================
--- arm64/arm64/pmap.c  (revision 352346)
+++ arm64/arm64/pmap.c  (working copy)
@@ -5810,13 +5810,21 @@ pmap_fault(pmap_t pmap, uint64_t esr, uint64_t far
        case ISS_DATA_DFSC_TF_L1:
        case ISS_DATA_DFSC_TF_L2:
        case ISS_DATA_DFSC_TF_L3:
+               if (pmap == kernel_pmap) {
+                       /*
+                        * The translation fault may have occurred within a
+                        * critical section.  Therefore, we must determine if
+                        * the address was invalid due to a break-before-make
+                        * sequence without acquiring the pmap lock.
+                        */
+                       if (pmap_kextract(far) != 0)
+                               rv = KERN_SUCCESS;
+                       break;
+               }
                PMAP_LOCK(pmap);
                /* Ask the MMU to check the address */
                intr = intr_disable();
-               if (pmap == kernel_pmap)
-                       par = arm64_address_translate_s1e1r(far);
-               else
-                       par = arm64_address_translate_s1e0r(far);
+               par = arm64_address_translate_s1e0r(far);
                intr_restore(intr);
                PMAP_UNLOCK(pmap);

In D21685#473043, @markj wrote:

I don't see why this is sufficient. If the unlocked translation attempt fails, e.g., because the mapping is being promoted or demoted a second time, we may still attempt to acquire a mutex in a scenario where that is not allowed.
Can we use the same trick that pmap_kextract() does to identify mappings that are transiently invalid?

When you commit the log entry should: (1) reference the prior commit when this code became dead code and (2) say, "Tested by: pho (as part of a larger change)"

In D21642#472706, @kib wrote:

In D21642#472677, @markj wrote:

In D21642#472503, @alc wrote:

Yes, I put the const in the wrong place. So, code within vm_page.c will avoid the dereference, but code outside vm_page.c that uses the declaration from vm_page.h will still perform the dereference. Declaring vm_page_array as an array would deal with both cases.

I'm still missing something. vm_page_array cannot be defined in vm_page.h, and if it is not defined there I do not see how the compiler can elide a memory dereference.

It cannot be defined, but can be declared, same as now. Or I do not understand what you are trying to say.

I also do not see how an array symbol can be initialized to VM_MIN_KERNEL_ADDRESS.

You use .set in some asm file. No C definition is provided.

In D21642#472499, @markj wrote:

In D21642#472496, @alc wrote:

In D21642#472480, @markj wrote:

In D21642#472450, @alc wrote:

We could go further and make vm_page_array a const pointer on amd64, ...

If you start down this path, I would go a step further: Declare vm_page_array as an array, instead of a pointer, and define the symbol's value to VM_MIN_KERNEL_ADDRESS. This would eliminate the memory dereference to retrieve the start of the array.

Sorry, I don't quite see what you are proposing. My suggestion also eliminated the memory dereference, but I don't see how declaring vm_page_array as an array would help anything.

I'm afraid not. Consider this simplified example.

const int *ptr = (int *)0x10000000;
...

My proposal would be equivalent to int *const ptr = (int *)0x10000000, which yields

func:                                   # @func                                              
        .cfi_startproc                                                                         
# %bb.0:                                # %entry                  
        pushq   %rbp                           
        .cfi_def_cfa_offset 16
        .cfi_offset %rbp, -16                                                                  
        movq    %rsp, %rbp                                                                     
        .cfi_def_cfa_register %rbp                                                             
        movl    268435456, %eax                
        popq    %rbp                       
        .cfi_def_cfa %rsp, 8                                                                   
        retq                                                                                   
.Lfunc_end0:                                                                                   
        .size   func, .Lfunc_end0-func                                                                                                                                                        
        .cfi_endproc

In D21642#472480, @markj wrote:

In D21642#472450, @alc wrote:

We could go further and make vm_page_array a const pointer on amd64, ...

If you start down this path, I would go a step further: Declare vm_page_array as an array, instead of a pointer, and define the symbol's value to VM_MIN_KERNEL_ADDRESS. This would eliminate the memory dereference to retrieve the start of the array.

Sorry, I don't quite see what you are proposing. My suggestion also eliminated the memory dereference, but I don't see how declaring vm_page_array as an array would help anything.

We could go further and make vm_page_array a const pointer on amd64, ...

If you start down this path, I would go a step further: Declare vm_page_array as an array, instead of a pointer, and define the set the symbol's value to VM_MIN_KERNEL_ADDRESS. This would eliminate the memory dereference to retrieve the start of the array.

How does this perform on the microbenchmarks that you've previously used?

In D21566#470372, @kib wrote:

... Note that vm_map_wire() use of vm_fault() caused ktrace recording the wired pages, which IMO is not right.

Thank you.

I believe that PA_MIN_ADDRESS can be removed, as it was introduced by the original version of this code.

The code looks good.

In D21352#467724, @kib wrote:

Split current program stackgap state control, and the state after execve. Current state can be only disabled, while state after exec can be re-enabled.

My only hesitation from "rubber stamping" this change is the difficulty of describing the complete semantics of this option, specifically, the semantics that arise when reenabling the stack gap. Suppose that we have disabled and then reenabled stack gaps for a process. Stacks created before gaps are disabled will then have their original gap enforced, unless the stack has grown into the previously defined gap region. Stacks created while gaps are disabled will have no gap after gaps are reenabled. Consequently, I want to ask: Should we actually support reenabling stack gaps on a process once they've been disabled?

In D21424#466489, @kib wrote:

I do not object against the removals. But please note that we had systematic bugs where pages were 'forgotten' to be added to a queue after allocation or some uses, essentially making them unswappable. So some way to assert that the page is visible to the pagedaemon is needed.

In D21352#465323, @kib wrote:

That said, do you have any objections or suggestions for the procctl(2) knob ?

I would also like to suggest that we store the stack_guard_page value that was in force at the time that the guard entry was created in the map entry and use this stored value in vm_map_growstack(). The next_read field should be unused by guard entries.

In D21352#465082, @kib wrote:

In D21352#465045, @alc wrote:

In D21352#464986, @kib wrote:

There is one detail, I forgot it, and think that it is mostly irrelevant, but it is still interesting. This was the reason why vm_map_protect() does clip and then skips GUARD entries. When creating a guarded stack, libthr does mmap(total size of stack and guard), then mprotect(guard). This clips out part of the guard to the permanent guard, the rest is the shrinking entry.

Actually, I think that it is relevant. As far as I can tell, the explicitly created libthr guard doesn't disable the security.bsd.stack_guard_page guard. If so, we are actually killing the thread before it hits the libthr guard.

Well, it is relevant in the sense that the guard is created. I mean that it is irrelevant whether libthr does mprotect() or mmap(MAP_GUARD) to create the extra guard at the bottom.

In D21352#464986, @kib wrote:

In D21352#464984, @alc wrote:

In D21352#464943, @kib wrote:

In D21352#464862, @alc wrote:

Yes?

Almost, but not quite. There is pthread feature where thread can be created with some attributes. Among them are the stack size and guard size. We create the stack of the specified size as you described (two entries, guard has reserved part which cannot be grown into), and below the libthr.so puts yet another guard entry, of the size specified in the attribute. JVM hacks on that guard, not on the implicit kernel guard.

Does libthr create a third, guard entry by default, or only if you pass an explicit "attr"?

Default guard size is zero, so it is not created explicitly. Third is it because you count two guards and mapped part ?

This looks good. Can you please repeat the performance test with and without this patch.

In D21352#464943, @kib wrote:

In D21352#464862, @alc wrote:

Yes?

Almost, but not quite. There is pthread feature where thread can be created with some attributes. Among them are the stack size and guard size. We create the stack of the specified size as you described (two entries, guard has reserved part which cannot be grown into), and below the libthr.so puts yet another guard entry, of the size specified in the attribute. JVM hacks on that guard, not on the implicit kernel guard.

In D21352#464519, @kib wrote:

In D21352#464514, @alc wrote:

If I remember correctly, this would be applicable to openjdk. Specifically, my recollection is that there were different classes of threads, such as system maintenance threads and application code execution threads, and we wound up with two guard entries for application threads, one created by the kernel and another by the JVM.

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=239894

If I remember correctly, this would be applicable to openjdk. Specifically, my recollection is that there were different classes of threads, such as system maintenance threads and application code execution threads, and we wound up with two guard entries for application threads, one created by the kernel and another by the JVM.

In D20814#463451, @dougm wrote:

Simplify the patch.

My major concern with this change is that it is doing two things at once, and I would ask that these two things be done separately, so that they can be individually evaluated. As the title says, this change is making simplification one way. However, most of this change is actually about decommissioning the "prev" pointer in the map entry.

I would recommend "MFC after: 3 days".

Please take a look at the latest comment on D20599. It shows that the two-page reservation is required to preserve the BSD label when the swap partition is the first partition within a slice. Therefore, I recommend that this proposed change be abandoned.

Deindent most of pmap_kextract().

Before we get into the design of the priority mechanism, I'd like to discuss some of the test results and what I think that we should do in response to them.

Please try D21169 in combination with this change.

In D21149#459783, @andrew wrote:

Hasn't this opened a raise between promotion/demotion and pmap_kextract? pmap_kextract walks the page table so may find a zero entry from the break-before-make sequence.

In D21126#459279, @markj wrote:

Suppose we pmap_enter() a zero-filled page. How do we guarantee that the PTP update isn't visible to another CPU before the page's contents are zero'ed?

Add a comment to pmap_demote_l2_locked() about pmap_fill_l3().

In D20863#458766, @markj wrote:

In D20863#455860, @alc wrote:

Mark, I discussed this patch with Doug today and recommended that he modify swapdev_trim() so that it never sleeps and instead returns NULL. In particular, I don't think that we want the laundry thread sleeping because of a trim operation encountering a shortage of free memory. Thoughts?

Sorry, I missed this. Yes, I think that's correct. We occasionally see deadlocks in CAM in Peter's tests, where we rely on a M_WAITOK allocation succeeding in the I/O path.
I have been wondering lately if it would be reasonable to let the laundry thread maintain a chunk of reserved memory to handle these cases, but I think we should avoid introducing new ones whenever possible.