Without the patch, a "buildworld" generates the following counts:

vm.pmap.l2.promotions: 113985
vm.pmap.l2.p_failures: 1
vm.pmap.l2.mappings: 59015
vm.pmap.l2.demotions: 11697
vm.reserv.reclaimed: 0
vm.reserv.partpopq:
DOMAIN    LEVEL     SIZE  NUMBER

The changes look okay. I've booted a kernel with this latest version of the patch, and I'm testing it overnight. I'll let you know if the promotion count changes significantly.

I think that swapgeom_strategy() should be made to handle a BIO_DELETE just like a BIO_WRITE.

On amd64, we've done a bit of testing here with https://github.com/GraphChi/graphchi-cpp doing pagerank, and we've seen a lot of vm object lock contention and a little pmap lock contention. In regards to the pmap lock, some of the functions showing up in the lock profile, e.g., pmap_is_prefaultable(), really only need a read lock on the pmap. In other words, I think that there may be motivation beyond accessed and dirty bit emulation to consider changing the pmap lock to a rw lock.

I've been testing this change in combination with the just committed r349975, and I haven't seen any suspicious behavior. Moreover, in regards to reviewing this patch, I've read over it multiple times and I'm past the point of diminishing returns. So, I'm going to suggest that you commit it.

In D20904#453005, @markj wrote:

efi_arch_enter() and _leave() have the isb following the TLB invalidation.

Add the isb instruction to efi_arch_enter() and efi_arch_leave().

In D20863#452194, @dougm wrote:

Something less obviously blocking, though I'm sure some flags are set wrong and the wait for an available write resource (nsw_wcount_async) is unacceptible. There must a queue somewhere I could stick that task in, waiting for writing to be okay again.

To be clear, I am not arguing that the alternative design is the better one. Only that there are plausible alternatives, and that we shouldn't start making incremental changes until we have evidence for following one approach or the other.

I would really like to see a complete prototype before we start committing piecemeal changes. Largely, because it is not clear to me that a "two cursors"-based approach is the right design. Alternatively, when trim is enabled, we could introduce a "coalescing queue" in front of the blist_free() calls. In other words, when trim is enabled, where blist_free() is currently called, we would simply enqueue the block numbers. When the number and/or size of the entries in the coalescing queue crosses some threshold, we would wake up a helper thread that issues trim operations for some of the entries and calls blist_free() on the corresponding blocks after the trim operation has completed. Situations where the number of free blocks in the blist falls below a threshold, may also require special handling, possibly even skipping the trim operation. The unknown with this alternative approach is how large the coalescing queue would be, or really how much memory we would allow it to consume, in order for it to be effective.

I would refactor this differently. I would create a helper function to handle the "in transition" case, in other words, the body of the following "if" statement.

                if (entry->eflags & MAP_ENTRY_IN_TRANSITION) {
                        /*                                                                                                                                                          
                         * We have not yet clipped the entry.                                                                                                                       
                         */
...
                }

I don't feel strongly enough about this issue to request that the change be reverted.

I will be very happy to see this change committed. It's something that I've wanted to see happen for many years.

I don't like this change. In cases like this, specifically error handling in complex code, the goto is easier to understand (and very common in kernel code). A "break;" is context sensitive, i.e., I have to determine whether I am a nested loop.

Peter, can you please retest this patch? I think that all of the open issues have been addressed.

Just an FYI, when I modified arm64_icache_sync_range() to do the range-based "ic" operations, instead of flushing the entire L1 I-cache, performance got very slightly worse. I was somewhat surprised by this result.

I would ask Peter to retest this latest version, and if all is well add Kostik and Mark as reviewers. I am ready to rubber-stamp this version unless Peter turns up a problem.

Are all of the prerequisite changes for this patch now committed?

In D20763#451142, @markj wrote:

In D20763#451040, @alc wrote:

Sorry, I wasn't clear. There are two aspects to Jeff's proposal: (1) inverting the sense of the flag and (2) making it an "aflag" so that it can be modified asynchronously. I don't think (1) helps. As I described, for breaking reservations, it would be a pessimization. However, I think (2) might prove beneficial. For example, it's not obvious to me that we could safely clear PG_CACHE_ALLOC asynchronously on an allocated page without the page and/or object lock.

Ok. I propose that we commit this diff, add a page cache for VM_FREEPOOL_DIRECT, and use PG_CACHE_ALLOC to skip vm_reserv_free_page() calls in vm_page_free_prep(). Then we can try and come up with a policy for reserving contiguous memory for _NOFREE allocations, which might involve making PG_CACHE_ALLOC an aflag.

In D20763#451029, @markj wrote:

In D20763#451027, @markj wrote:

In D20763#450948, @alc wrote:

In D20763#450130, @jeff wrote:

I wrote an experimental patch last year to segregate NOFREE zones using a new vm_phys pool. You can find it at D16620. It's better than nothing. :-) However, its primary shortcomings are that (1) it only applies to architectures with a direct map and (2) that pools only provide "soft" segregation. In other words, I don't think vm_phys pools are the right solution.

For (1), could we use a separate vmem arena for _NOFREE allocations like we do for M_EXEC?

That's not as good, since it will only provide physical contiguity so long as there are free reservation-sized chunks of physical memory.

In D20763#451027, @markj wrote:

In D20763#450948, @alc wrote:

In D20763#450130, @jeff wrote:

I wrote an experimental patch last year to segregate NOFREE zones using a new vm_phys pool. You can find it at D16620. It's better than nothing. :-) However, its primary shortcomings are that (1) it only applies to architectures with a direct map and (2) that pools only provide "soft" segregation. In other words, I don't think vm_phys pools are the right solution.

For (1), could we use a separate vmem arena for _NOFREE allocations like we do for M_EXEC?

Allow me to suggest a slight alternative to the above that I think will cover more cases. If we invert the sense of the flag and put it in an atomic field we can then set it anywhere that we discover a page that is creating fragmentation or may do so. For example, in the defrag code we could discover a single page in a superpage that is presently allocated and set the bit. We would additionally set the bit whenever we break a reservation to allocate a page because we are low on memory, which is what this case handles.

Yes, I think something like this flag could prove useful. However, in regards to reservations, when we break a reservation, the code is actually "clever" enough that it does not touch every vm_page structure. From the starting physical address of the reservation and the reservation's popmap we figure out the maximal order chunks of pages to reinsert into the buddy queues, and so vm_phys never winds up coalescing any of the pages from a broken reservation. Setting this proposed flag would introduce new vm_page structure touches.

I don't quite see how inverting the sense would make it possible to handle more cases. With this diff, we free to the per-CPU cache if and only if PG_CACHE_ALLOC is set. Defragmentation code, upon encountering an allocated page in an otherwise free 2MB chunk, could clear PG_CACHE_ALLOC to ensure that the page would bypass the per-CPU caches when it is freed by its owner. Today, vm_page_reclaim_run() frees pages directly to vm_phys and thus always bypasses the per-CPU cache, otherwise it would need to be updated by this diff.

A kernel with this change has survived multiple, back-to-back "make -j8 buildworld"'s with no spurious bus errors.

In D20763#450130, @jeff wrote:

My recollection is that when I implemented the cache there simply wasn't a lot of traffic for the other free pools and I didn't want to increase fragmentation. I have no objection to doing so now. I do believe we should have some separation for UMA NOFREE zones whether that is a different pool or some other layer I can not say.

Why is vm_object_pip_wakeup() no longer called?

Why is vm_pager_page_unswapped() no longer called?

Sync with https://reviews.freebsd.org/D20812, which applies a bunch of style fixes and one micro-optimization to the amd64 pmap_copy().

More style fixes.