Is it possible to extract and commit refactoring bits in advance ? I mean pmap_enter_2mpage(), pmap_remove_ptes() and vm_page_ps_test() at least.

In D11556#239313, @kib wrote:

Is it possible to extract and commit refactoring bits in advance ? I mean pmap_enter_2mpage(), pmap_remove_ptes() and vm_page_ps_test() at least.

vm_page_ps_test() is easy. Let me start with that. I propose to commit that part of this change in about 9 hours. Can I declare that part as having been reviewed by you guys?

In D11556#239751, @alc wrote:

In D11556#239313, @kib wrote:

Is it possible to extract and commit refactoring bits in advance ? I mean pmap_enter_2mpage(), pmap_remove_ptes() and vm_page_ps_test() at least.

vm_page_ps_test() is easy. Let me start with that. I propose to commit that part of this change in about 9 hours. Can I declare that part as having been reviewed by you guys?

vm_page_ps_test() looks fine to me.

In D11556#239751, @alc wrote:

vm_page_ps_test() is easy. Let me start with that. I propose to commit that part of this change in about 9 hours. Can I declare that part as having been reviewed by you guys?

Sure.

In D11556#239313, @kib wrote:

Is it possible to extract and commit refactoring bits in advance ? I mean pmap_enter_2mpage(), pmap_remove_ptes() and vm_page_ps_test() at least.

I propose to peel off the refactoring of pmap_remove(), creating pmap_remove_ptes(), next. Do you guys have any further questions about pmap_remove_ptes()?

In D11556#240188, @alc wrote:

I propose to peel off the refactoring of pmap_remove(), creating pmap_remove_ptes(), next. Do you guys have any further questions about pmap_remove_ptes()?

That bit looks fine to me.

Here is some data on PostgreSQL execution that I collected with the initial version of this patch.

To be clear, I am using a database that is three times larger than physical memory. The database is stored on a Samsung 850 PRO SSD, so my tests are I/O bound, not CPU bound. Adjust your expectations accordingly. Nonetheless, I see a slight but consistent increase in TPS.

I have configured PostgreSQL to use 1/4 of physical memory for shared buffers. I am not using unmanaged pages for the shared buffers.

My machine has six cores and I am running the following test script:

$ for i in 15 15 15 15 15 15; do
> sysctl vm.pmap.pde vm.reserv vm.stats.vm
> pgbench -j 1 -c $i -T 1800 -S bench
> done ; sysctl vm.pmap.pde vm.reserv vm.stats.vm

After the first iteration, TPS results don't vary much at all, at most 10 TPS.

Before the patch:

number of transactions actually processed: 20717230
tps = 11509.560124 (including connections establishing)
tps = 11509.740169 (excluding connections establishing)

After the patch:

number of transactions actually processed: 20990549
tps = 11661.192676 (including connections establishing)
tps = 11661.376262 (excluding connections establishing)

Here is some raw data after the 2nd iteration:

Before the patch:

vm.pmap.pde.promotions: 400
vm.pmap.pde.p_failures: 6882801
vm.pmap.pde.mappings: 3879
vm.pmap.pde.demotions: 110
...
vm.reserv.fullpop: 3112
...
vm.stats.vm.v_vm_faults: 59527355

After the patch:

vm.pmap.pde.promotions: 407
vm.pmap.pde.p_failures: 59
vm.pmap.pde.mappings: 134076
vm.pmap.pde.demotions: 116
...
vm.reserv.fullpop: 3126
...
vm.stats.vm.v_vm_faults: 19490732

The most striking thing about the counts is the 3-fold reduction in page faults.

The next most striking thing to me was the difference between fullpops and promotions. In both cases, fullpops was greater than 3100, which says that we have 3100+ physical superpages, but we never promoted more than 407 virtual-to-physical mappings to 2 MB page mappings. Prior to the patch, most of the 2 MB page mappings were being created by fork(2). In other words, whatever promotions occurred in the master, template process got carried over to the child processes by pmap_copy() on fork(2).

In D11556#239313, @kib wrote:

Is it possible to extract and commit refactoring bits in advance ? I mean pmap_enter_2mpage(), pmap_remove_ptes() and vm_page_ps_test() at least.

It's hard to disentangle the pmap_enter_2mpage() / pmap_enter_pde() refactoring from the new code.

That said, there is one purely stylistic change in pmap_enter_pde() that I could peel off. I changed from the variable name "mpde" to "pdpg" for the vm_page_t-type variable that references a PD page. About half of the pmap uses "pdpg" and the other half uses "mpde". I've come to prefer "pdpg". I could do a global replace and make this consistent throughout the file. This would reduce the size of this change by a few lines. Opinions?

In D11556#240303, @alc wrote:

It's hard to disentangle the pmap_enter_2mpage() / pmap_enter_pde() refactoring from the new code.

Ok.

That said, there is one purely stylistic change in pmap_enter_pde() that I could peel off. I changed from the variable name "mpde" to "pdpg" for the vm_page_t-type variable that references a PD page. About half of the pmap uses "pdpg" and the other half uses "mpde". I've come to prefer "pdpg". I could do a global replace and make this consistent throughout the file. This would reduce the size of this change by a few lines. Opinions?

Go for it.

Here is another test case that benefits from this change:

#include <sys/param.h>
#include <sys/ipc.h>
#include <sys/shm.h>

#include <err.h>
#include <stdio.h>

#define SHM_SIZE        (8 * 1024 * 1024)

int
main(int argc, char *argv[])
{
        key_t key;
        int shmid;
        char *data;
        int i, sum;

        key = ftok("/etc/motd", 63);
        if ((shmid = shmget(key, SHM_SIZE, IPC_CREAT | SHM_R | SHM_W)) == -1)
                err(1, "shmget");
        if ((data = shmat(shmid, 0, 0)) == (void *)-1)
                err(1, "shmat");
        printf("data = %p\n", data);
        sum = 0;
        for (i = 0; i < SHM_SIZE; i++) {
                data[i] += i;
                sum += data[i];
        }
        printf("sum = %d\n", sum);
        getchar();
        if (shmdt(data) == -1)
                warn("shmdt");
#if 0
        if (shmctl(shmid, IPC_RMID, NULL) == -1)
                warn("shmctl");
#endif
        return (0);
}

The first execution of this program will go through the normal page allocation, mapping, and eventual promotion process. (If you enable kern.ipc.shm_use_phys, then the number of page faults is reduced. Essentially, a single page fault iteratively maps and eventually promotes each superpage.)

The second and subsequent executions are where we see a difference. Initially, shmat() prefaults read-only superpage mappings without the PG_A bit set because these are speculative mappings. Then, the program faults on a write access. Without this change, we demote the mapping and reenable write access page-by-page, faulting on each page until eventually we repromote with a writeable superpage mapping. With this change, on the first page fault on each superpage we recognize that the underlying physical superpage is all dirty and immediately recreate a writeable superpage mapping.

In D11556#240398, @alc wrote:

The second and subsequent executions are where we see a difference. Initially, shmat() prefaults read-only superpage mappings without the PG_A bit set because these are speculative mappings. Then, the program faults on a write access. Without this change, we demote the mapping and reenable write access page-by-page, faulting on each page until eventually we repromote with a writeable superpage mapping. With this change, on the first page fault on each superpage we recognize that the underlying physical superpage is all dirty and immediately recreate a writeable superpage mapping.

This finally prompted me to ask the question which I have for some time. Why do you test for PS_ALL_DIRTY, at all ? If the page fault which causes psind == 1 mapping would be read and next access is for write, then the whole superpage become dirty. So, why not to dirty all pages when mapping the superpage ?

I understand that this is not how current superpage promotion work, but in-tree promotion code have to operate inside pmap. With high-level vm_fault() code knowing about superpage mapping, I believe there is no architectural constraints like that.

In D11556#240401, @kib wrote:

In D11556#240398, @alc wrote:

The second and subsequent executions are where we see a difference. Initially, shmat() prefaults read-only superpage mappings without the PG_A bit set because these are speculative mappings. Then, the program faults on a write access. Without this change, we demote the mapping and reenable write access page-by-page, faulting on each page until eventually we repromote with a writeable superpage mapping. With this change, on the first page fault on each superpage we recognize that the underlying physical superpage is all dirty and immediately recreate a writeable superpage mapping.

This finally prompted me to ask the question which I have for some time. Why do you test for PS_ALL_DIRTY, at all ? If the page fault which causes psind == 1 mapping would be read and next access is for write, then the whole superpage become dirty. So, why not to dirty all pages when mapping the superpage ?

I'm not sure if this is the answer to your question. Consider an mmap(..., PROT_WRITE, MAP_SHARED, fd, ...) on an actual file. I'm trying to avoid a single store making us treat the entire superpage as dirty, i.e., have to write it back to disk. Strictly speaking, the extra parameter, "skip_m", to vm_page_ps_test() is allowing us to ask whether all pages other than the one that we faulted on are dirty, and we allow the psind == 1 mapping to occur even if the one page that we faulted on is not yet dirty and we took a read fault on it, not a write fault. In a sense, I am predicting that this one page will be dirtied based on the fact that its 511 peers in the same physical superpage already are. In this case, if I'm wrong, we only wrote an extra 4KB to disk.

I understand that this is not how current superpage promotion work, but in-tree promotion code have to operate inside pmap. With high-level vm_fault() code knowing about superpage mapping, I believe there is no architectural constraints like that.

In D11556#240402, @alc wrote:

I'm not sure if this is the answer to your question. Consider an mmap(..., PROT_WRITE, MAP_SHARED, fd, ...) on an actual file. I'm trying to avoid a single store making us treat the entire superpage as dirty, i.e., have to write it back to disk. Strictly speaking, the extra parameter, "skip_m", to vm_page_ps_test() is allowing us to ask whether all pages other than the one that we faulted on are dirty, and we allow the psind == 1 mapping to occur even if the one page that we faulted on is not yet dirty and we took a read fault on it, not a write fault. In a sense, I am predicting that this one page will be dirtied based on the fact that its 511 peers in the same physical superpage already are. In this case, if I'm wrong, we only wrote an extra 4KB to disk.

Yes, but you are only considering two corner cases, where all pages except current are clean, and all pages except current are dirty. If the argument is that forcing all pages of the superpage to become dirty is too costly because all of them need to be paged out, I mentioned this by stating that we might have observed a write after the promotion, in which case we either have to demote or declare all pages dirty.

Now with the high level VM knowing about superpages, we can apply more involved policies in the place of calling pmap_enter(). E.g. we might count number of dirty pages forming the superpage and request psind == 1 if count >= 1/4 of 512 or any other arbitrary constant, or even a tunable to allow the tuning for loads.

In D11556#240408, @kib wrote:

In D11556#240402, @alc wrote:

I'm not sure if this is the answer to your question. Consider an mmap(..., PROT_WRITE, MAP_SHARED, fd, ...) on an actual file. I'm trying to avoid a single store making us treat the entire superpage as dirty, i.e., have to write it back to disk. Strictly speaking, the extra parameter, "skip_m", to vm_page_ps_test() is allowing us to ask whether all pages other than the one that we faulted on are dirty, and we allow the psind == 1 mapping to occur even if the one page that we faulted on is not yet dirty and we took a read fault on it, not a write fault. In a sense, I am predicting that this one page will be dirtied based on the fact that its 511 peers in the same physical superpage already are. In this case, if I'm wrong, we only wrote an extra 4KB to disk.

Yes, but you are only considering two corner cases, where all pages except current are clean, and all pages except current are dirty. If the argument is that forcing all pages of the superpage to become dirty is too costly because all of them need to be paged out, I mentioned this by stating that we might have observed a write after the promotion, in which case we either have to demote or declare all pages dirty.

Now with the high level VM knowing about superpages, we can apply more involved policies in the place of calling pmap_enter(). E.g. we might count number of dirty pages forming the superpage and request psind == 1 if count >= 1/4 of 512 or any other arbitrary constant, or even a tunable to allow the tuning for loads.

Ok, now I understand. Yes, we could apply more speculative policies. For example, my temptation would be to distinguish between OBJT_VNODE and OBJT_DEFAULT, applying a higher threshold to the former. Already, along these lines, we don't care whether unmanaged pages are dirty or not when creating a writeable mapping.

I just doubled-checked my recollection about PostgreSQL: Relaxing the all-dirty requirement has no effect, specifically, there is no reduction in page faults.

In D11556#240441, @alc wrote:

I just doubled-checked my recollection about PostgreSQL: Relaxing the all-dirty requirement has no effect, specifically, there is no reduction in page faults.

Postgres has very specific usage pattern, it first writes whole large buffer, and then reads it as a whole.

I'm able to trip the "freeing mapped page" assertion in vm_page_free_toq() when running with this diff applied. firefox was exiting after dumping core (almost certainly also a result of this change) and was deallocating objects and freeing their pages. I have to go AFK for a few hours now but will debug further when I get back.

On an unrelated note, does pmap_enter(psind > 0) create large mappings even when pg_ps_enabled = 0?

In D11556#240553, @markj wrote:

I'm able to trip the "freeing mapped page" assertion in vm_page_free_toq() when running with this diff applied. firefox was exiting after dumping core (almost certainly also a result of this change) and was deallocating objects and freeing their pages. I have to go AFK for a few hours now but will debug further when I get back.

On an unrelated note, does pmap_enter(psind > 0) create large mappings even when pg_ps_enabled = 0?

Hmm, yes. Worse yet, pagesizes[1] is 0 when pg_ps_enabled is 0. That could produce strange behavior from the vm_fault_soft_fast().

To be clear, when pg_ps_enabled is 0, the reservation code will still be active and "promote" physical pages, i.e., set m->psind to 1. Prior to this patch, we just didn't create superpage mappings.

Did you have pg_ps_enabled set to 0?

In D11556#240559, @alc wrote:

To be clear, when pg_ps_enabled is 0, the reservation code will still be active and "promote" physical pages, i.e., set m->psind to 1. Prior to this patch, we just didn't create superpage mappings.

Did you have pg_ps_enabled set to 0?

Isilon does, but we have do have reservations enabled. It appears to have been disabled back when we were on FreeBSD 7 (i.e., before my time there), but I can't find any reasoning for it in the commit logs. I believe Jeff experimented with enabling them earlier this year and found that it didn't give much of an improvement, but I don't know the details. I'll try to find out more.

Peter, could you please run this patch through your battery of tests?

I can trigger this with ease:
panic: Assertion m_super->object == fs->first_object failed at ../../../vm/vm_fault.c:290
Details @ https://people.freebsd.org/~pho/stress/log/alc007.txt

I too observed firefox SIGSEGV. Using:
firefox -no-remote http://b.dk/ and exiting after the images were loaded would trigger the fault.

In D11556#241098, @pho wrote:

I can trigger this with ease:
panic: Assertion m_super->object == fs->first_object failed at ../../../vm/vm_fault.c:290
Details @ https://people.freebsd.org/~pho/stress/log/alc007.txt

I believe that the updated patch will address this assertion failure.

I too observed firefox SIGSEGV. Using:
firefox -no-remote http://b.dk/ and exiting after the images were loaded would trigger the fault.

In D11556#241123, @alc wrote:

In D11556#241098, @pho wrote:

I can trigger this with ease:
panic: Assertion m_super->object == fs->first_object failed at ../../../vm/vm_fault.c:290
Details @ https://people.freebsd.org/~pho/stress/log/alc007.txt

I believe that the updated patch will address this assertion failure.

It did. I have not observed any other problems with this patch, apart from the firefox issue.

In D11556#241371, @pho wrote:

In D11556#241123, @alc wrote:

In D11556#241098, @pho wrote:

I can trigger this with ease:
panic: Assertion m_super->object == fs->first_object failed at ../../../vm/vm_fault.c:290
Details @ https://people.freebsd.org/~pho/stress/log/alc007.txt

I believe that the updated patch will address this assertion failure.

It did. I have not observed any other problems with this patch, apart from the firefox issue.

My suspicion is that this is a copy-on-write issue, specifically, that a page on which we took a copy-on-write fault is being replaced by the original unmodified page through the 2MB page mapping.

Peter, do you have any other information on the SIGSEGV? Knowing the faulting address and the vm_map_entry's for the address space, e.g., "procstat -v" or the like, might help.

In D11556#241616, @alc wrote:

In D11556#241371, @pho wrote:

In D11556#241123, @alc wrote:

In D11556#241098, @pho wrote:

I can trigger this with ease:
panic: Assertion m_super->object == fs->first_object failed at ../../../vm/vm_fault.c:290
Details @ https://people.freebsd.org/~pho/stress/log/alc007.txt

I believe that the updated patch will address this assertion failure.

It did. I have not observed any other problems with this patch, apart from the firefox issue.

My suspicion is that this is a copy-on-write issue, specifically, that a page on which we took a copy-on-write fault is being replaced by the original unmodified page through the 2MB page mapping.

Peter, do you have any other information on the SIGSEGV? Knowing the faulting address and the vm_map_entry's for the address space, e.g., "procstat -v" or the like, might help.

I took a step back and ran the same firefox test on a pristine kernel:
$ firefox -no-remote http://b.dk/
GLib-GIO-Message: Using the 'memory' GSettings backend. Your settings will not be saved or shared with other applications.
Segmentation fault (core dumped)
$ uname -a
FreeBSD t1.osted.lan 12.0-CURRENT FreeBSD 12.0-CURRENT #0 r321074: Thu Jul 20 08:25:22 CEST 2017 pho@t1.osted.lan:/usr/src/sys/amd64/compile/PHO amd64
$
So I would say that this problem is unrelated to your patch.

In D11556#241767, @alc wrote:

Check for a mismatch between the reservation's object field and the page's object field in vm_reserv_to_superpage(). As long as vm_fault()'s fast path is the only caller to vm_reserv_to_superpage(), I don't think that a mismatch is possible, but it could be required if we start calling vm_reserv_to_superpage() in other places.

I tested this version with all of the tests I have that uses mmap(2). Also did a buildworld. No problems seen.

In D11556#242095, @pho wrote:

In D11556#241767, @alc wrote:

Check for a mismatch between the reservation's object field and the page's object field in vm_reserv_to_superpage(). As long as vm_fault()'s fast path is the only caller to vm_reserv_to_superpage(), I don't think that a mismatch is possible, but it could be required if we start calling vm_reserv_to_superpage() in other places.

I tested this version with all of the tests I have that uses mmap(2). Also did a buildworld. No problems seen.

Thanks, Peter.

I haven't been able to conjure up an actual test case that demonstrates a need for the new consistency check in vm_page_ps_test(). However, I am still worried that a case could arise. Essentially, we have a fully populated reservation that originates in what becomes a backing object (as opposed to a "first" object). We take an ordinary copy-on-write fault that leads to the creation of a shadow object and a new writeable physical page. Then, the conditions change so that future write faults are eligible for the "optimized" COW handling, wherein we migrate the existing, previously read-only physical page to the first object and simply reenable write access. (In other words, we don't really copy the page on this optimized path.) A critical fact here is that we also migrate the reservation. My hypothetical problem scenario is that somehow one of the newly read/write mappings created by the optimized COW path within the pmap level gets invalidated. And, we fault on it again. Now, we find ourselves in vm_fault_soft_fast() and without the new check in vm_page_ps_test() we could incorrectly map the superpage even though the initial COW fault logically replaced one of the 4KB pages.

Did that make sense? If so, I propose to declare victory and commit these changes. I don't have any remaining nagging doubts.

Peter, given the nature of my latest change, I don't think that you need to repeat any of your testing.