In D30644#688473, @jrtc27 wrote:

Wes, could you please check this does indeed fix the panic you were seeing with snmalloc, not just your hand-written test?

In D30643#688425, @kib wrote:

Don't we mark the copied mapping as clean to avoid unnecessary writes? Suppose that the source mapping is destroyed, and backing pages are marked dirty and written to the storage. Now, if the copied mapping is destroyed without ever being written to, we would re-dirty and write them again.

You wrote, "Modify pmap_copy() to make new 2MB mappings read-only, like we do on amd64. I am not sure though why we shouldn't simply copy the dirty bit over to the child."

As an aside, I would probably replicate the amd64 comments that we do not need to perform a TLB invalidation in this case.

In D30442#683738, @kib wrote:

In D30442#683737, @jhb wrote:

Won't we demote large pages if the protections of sub-pages differ? I don't think we (yet) have a flag to make mappings use a minimum page size (e.g. to force the use of 2MB pages and not permit demoting to 4K pages) for which the effect would then be as Brooks' new sentence describes.

superpages != largepages. Largepages are relatively new thing, for them we guarantee that

1. backing object is populated with contiguous pages suitable for superpage mapping
2. userspace maping is always done with superpages PTEs
3. we do not allow to clip them not at superpage boundary

In D28805#646315, @markj wrote:

In D28805#645337, @alc wrote:

I really have mixed feelings about this change. If anything, we should try to discourage page zeroing while the object lock is held. On the other hand, for VM_ALLOC_NOOBJ allocations, I think that this change makes perfect sense, which brings me to the following proposal: Remove VM_ALLOC_NOOBJ and VM_ALLOC_ZERO from vm_page_alloc{,_contig}(), and provide separate allocation functions to replace VM_ALLOC_NOOBJ, akin to vm_page_alloc_freelist().
Virtually all VM_ALLOC_NOOBJ call sites are being changed, so we as well change the function being called. This will slightly simplify vm_page_alloc(), e.g., it can assume that the object is always non-NULL and that it will only allocate from the default pool. Also, when I say, "Remove ... VM_ALLOC_ZERO from vm_page_alloc()", I would probably let PG_ZERO pass through vm_page_alloc() unchanged. Right now, we clear that flag unless VM_ALLOC_ZERO was specified. As for the functions that replace VM_ALLOC_NOOBJ, I would drop not only the object parameter, but also the pindex. The few callers that (ab)use the pindex can set it themselves.

So to be clear, your proposal is to add a vm_page_alloc_anon() (or _noobj()?) which accepts a VM_ALLOC_ZERO flag and implements it by zeroing the page, whereas vm_page_alloc(_contig)() should stop taking VM_ALLOC_ZERO and instead return PG_ZERO unchanged?

Yes, and vm_page_alloc_noobj() makes sense to me. (I think that vm_page_alloc_anon() could be too easily confused with allocation of pages to OBJ_ANON vm objects. In other words, I would avoid using any derivatives of the word anonymous.)

I like the idea of splitting the allocator functions and preserving PG_ZERO. It feels a bit odd to have inconsistent handling with respect to VM_ALLOC_ZERO though. There are situations where allocations are rare enough that zeroing under the object lock is not a problem (or it is required), and splitting the allocator entry points would make it easier to spot calls where it is likely to be a problem. The pti_obj object used to manage userspace-visible "holes" in the kernel address space is an example of this.

I'm not going to argue strenuously for leaving VM_ALLOC_ZERO support out of vm_page_alloc{,_contig}(). I agree that splitting the allocator entry points will make it easier to spot calls that shouldn't use VM_ALLOC_ZERO.

In D28924#649287, @markj wrote:

In D28924#649276, @mav wrote:

In D28924#649265, @markj wrote:

it takes about 8s to allocate 100 clusters on a system with 64GB, vs. 2-2.5s with the patch applied.

It is good to hear, but still does not sound realistic for networking purposes. Plus my systems often have 256GB or more memory. Have you tried it together with your origial optimization patch?

Right, this not expected to be a full solution to the problem. I will look more at preferentially reclaiming from the phys_segs corresponding to the default freelists, and ending the scan earlier.

I am wondering if the intent behind the current implementation is to provide a consistent runtime for reclamation. Suppose we started scanning from the beginning of physical memory and over time reclaimed more and more runs. Subsequent scans will take longer and longer since they always start from the same place. Perhaps we could maintain some cursor that gets updated after a scan and is used to mark the beginning of subsequent scans.

mav@ could you please give your hack/test case to markj@. There should be a significant reduction in the amount scanning with this patch.

The rounding up should be capped at the largest supported buddy list order. (For allocation requests that are larger than the largest supported order, we do the following: For each block in the largest order list, we look at its successors in the vm_page array to see if a sufficient number of them are free to satisfy the request.)

I really have mixed feelings about this change. If anything, we should try to discourage page zeroing while the object lock is held. On the other hand, for VM_ALLOC_NOOBJ allocations, I think that this change makes perfect sense, which brings me to the following proposal: Remove VM_ALLOC_NOOBJ and VM_ALLOC_ZERO from vm_page_alloc{,_contig}(), and provide separate allocation functions to replace VM_ALLOC_NOOBJ, akin to vm_page_alloc_freelist(). Virtually all VM_ALLOC_NOOBJ call sites are being changed, so we as well change the function being called. This will slightly simplify vm_page_alloc(), e.g., it can assume that the object is always non-NULL and that it will only allocate from the default pool. Also, when I say, "Remove ... VM_ALLOC_ZERO from vm_page_alloc()", I would probably let PG_ZERO pass through vm_page_alloc() unchanged. Right now, we clear that flag unless VM_ALLOC_ZERO was specified. As for the functions that replace VM_ALLOC_NOOBJ, I would drop not only the object parameter, but also the pindex. The few callers that (ab)use the pindex can set it themselves.

In D28807#644991, @markj wrote:

In D28807#644985, @kib wrote:

Still, you might add a vm_page_alloc() flag that would ask to not clear PG_ZERO on return, making it the duty of the caller. Then vm_fault() could utilize it to preserve the optimization. Could it be useful for 32bit machines?

I considered it and was looking for i386 systems in the cluster so I can check the v_zfod and v_ozfod counter values. I couldn't find any though, so I will look at a VM soon and try some simple loads to see if it is worth preserving. My suspicion is that it is still a minor optimization even on 32-bit systems since we are relying on the pmap to provide pre-zeroed pages, and it will not provide very many relative to typical application usage. Pages allocated from superpage reservations are unlikely to be pre-zeroed. Finally, in principle the page will be warm in the data caches if it is zeroed on demand, while with a pre-zeroed page this is less likely.

Similarly, there is no reason to define VM_KMEM_SIZE_MIN.

In D27956#627791, @kib wrote:

In D27956#627736, @markj wrote:

Do riscv or arm64 have the same bug?

No, this bug was introduced with LA57 rewrite of pmap_allocpte(). riscv and arm64 forked pmap.c before LA57.

That's all that I have.

I have nothing more to add. Once you've dealt with Mark's latest comment, commit it. I don't see the need for me to look at this change again.

I'd be interested in seeing the output of sysctl vm.phys_segs on one of these machines. Can you post it here?

I ran into a similar problem with respect to vm_phys_segs[] in eliminating iteration from the vm_dumpset operations. I wonder if we shouldn't separate the struct vm_phys_seg and array declarations into their own separate header file that is included by vm_param.h. Then, vm_page.h could implement vm_page_domain().

Here is my first batch of comments.

After mulling this over for the past day or so, I'd like to propose the following alternative description:

I'm afraid that I can't make sense out of the summary. Is this a problem with the KASSERT that precedes the pmap lock acquire in arm64's pmap_remove_pages()? Omce pmap_remove_pages() has performed the lock acquire, prior changes to the pmap should be visible to it.

In D26876#599253, @markj wrote:

Most other implementations also use bzero(), though on them pagezero() is nothing more than bzero(va, PAGE_SIZE) anyway.

A while back, Intel quietly made it possible to measure address translation overhead for instruction accesses. I say, "quietly," because the manual provides a low-level description of the counter that doesn't really explain what it effectively measures. However, some Intel people gave a presentation at an HPC workshop about 2 years ago that explained the counter's meaning, and Intel published those slides here: https://software.intel.com/content/www/us/en/develop/download/how-top-down-microarchitecture-analysis-tma-addresses-challenges-in-modern-servers.html