vm_fault: Defer marking COW pages valid

[markj]

Summary

Suppose an object O has two shadow objects S1, S2 mapped into processes
P1, P2. Suppose a page resident in O is mapped read-only into P1. Now
suppose that P1 writes to the page, triggering a COW fault: it allocates
a new page in S1 and copies the page, then marks it valid. If the page
in O was busy, it would have to release the map lock and sleep first.
Then, after copying the page, P1 must re-check the map lookup because
locks were dropped. Suppose the map indeed changed, so P1 has to retry
the fault.

At this point, the mapped page in O is shadowed by a valid page in S1.
If P2 exits, S2 will be deallocated, resulting in a collapse of O into
S1. In this case, P2 will free the mapped page, which is illegal; this
triggers a "freeing mapped page" assertion in invariants kernels.

Fix the problem by deferring the vm_page_valid() call which marks the
COW copy valid: only mark it once we know that the fault handler will
succeed. It's okay to leave an invalid page in the top-level object; it
will be freed when the fault is retried, and vm_object_collapse_scan()
will similarly free invalid pages in the shadow object.

Sponsored by: Innovate UK

[kib]

I agree that this should work.
But, I somewhat dislike an idea that we keep actually valid page on the object' queue but do not mark it as valid. Might be, unmapping the shadowed page instead would also work, and then it is IMO both cleaner and possibly slightly optimizes parallel faults.

[markj]

In D49758#1134870, @kib wrote:

I agree that this should work.
But, I somewhat dislike an idea that we keep actually valid page on the object' queue but do not mark it as valid.
Might be, unmapping the shadowed page instead would also work, and then it is IMO both cleaner and possibly slightly optimizes parallel faults.

Well, that would also unmap unrelated mappings of the same backing page, resulting in unnecessary page faults. Plus, it's more expensive. To me that seems even worse, but I'm not sure how it would optimize parallel faults.

[kib]

In D49758#1134882, @markj wrote:

In D49758#1134870, @kib wrote:

I agree that this should work.
But, I somewhat dislike an idea that we keep actually valid page on the object' queue but do not mark it as valid.
Might be, unmapping the shadowed page instead would also work, and then it is IMO both cleaner and possibly slightly optimizes parallel faults.

Well, that would also unmap unrelated mappings of the same backing page, resulting in unnecessary page faults.

Why? pmap_remove() would only remove this mapping at the faulted address from this pmap().

Plus, it's more expensive. To me that seems even worse, but I'm not sure how it would optimize parallel faults.

If another thread accesses the same address while retrying thread restarts the fault, that thread already gets the valid page to map (soft fault). And parallel accesses to the same address is not that rare.

[markj]

In D49758#1134892, @kib wrote:

In D49758#1134882, @markj wrote:

In D49758#1134870, @kib wrote:

I agree that this should work.
But, I somewhat dislike an idea that we keep actually valid page on the object' queue but do not mark it as valid.
Might be, unmapping the shadowed page instead would also work, and then it is IMO both cleaner and possibly slightly optimizes parallel faults.

Well, that would also unmap unrelated mappings of the same backing page, resulting in unnecessary page faults.

Why? pmap_remove() would only remove this mapping at the faulted address from this pmap().

I was thinking of using pmap_remove_all(m) (this was my first solution actually). pmap_remove() takes a vaddr instead of a physical page, which implies that we know what is mapped at that vaddr. But we're restarting the fault because the map changed -- how do we know that it is safe and correct to pmap_remove() at that address? What if something else was mapped there while locks were dropped, e.g., a non-transparent superpage?

Plus, it's more expensive. To me that seems even worse, but I'm not sure how it would optimize parallel faults.

If another thread accesses the same address while retrying thread restarts the fault, that thread already gets the valid page to map (soft fault). And parallel accesses to the same address is not that rare.

If another threads writes to the same page, yes. On the other hand, if another thread is reading the page, it must now take a page fault to re-establish the mapping.

[kib]

In D49758#1134893, @markj wrote:

In D49758#1134892, @kib wrote:

In D49758#1134882, @markj wrote:

In D49758#1134870, @kib wrote:

I agree that this should work.
But, I somewhat dislike an idea that we keep actually valid page on the object' queue but do not mark it as valid.
Might be, unmapping the shadowed page instead would also work, and then it is IMO both cleaner and possibly slightly optimizes parallel faults.

Well, that would also unmap unrelated mappings of the same backing page, resulting in unnecessary page faults.

Why? pmap_remove() would only remove this mapping at the faulted address from this pmap().

I was thinking of using pmap_remove_all(m) (this was my first solution actually). pmap_remove() takes a vaddr instead of a physical page, which implies that we know what is mapped at that vaddr. But we're restarting the fault because the map changed -- how do we know that it is safe and correct to pmap_remove() at that address? What if something else was mapped there while locks were dropped, e.g., a non-transparent superpage?

Then we would remove the superpage mapping, which would be restored by next populate fault. The only thing that seems needs a change then, is the allowance to remove when partial address range is specified for non-transparent superpage. Perhaps yes, this is enough argument pro your approach.

Plus, it's more expensive. To me that seems even worse, but I'm not sure how it would optimize parallel faults.

If another thread accesses the same address while retrying thread restarts the fault, that thread already gets the valid page to map (soft fault). And parallel accesses to the same address is not that rare.

If another threads writes to the same page, yes. On the other hand, if another thread is reading the page, it must now take a page fault to re-establish the mapping.

For read faults, it is same: the page is resident but not mapped.

[markj]

In D49758#1134894, @kib wrote:

In D49758#1134893, @markj wrote:

In D49758#1134892, @kib wrote:

In D49758#1134882, @markj wrote:

In D49758#1134870, @kib wrote:

I agree that this should work.
But, I somewhat dislike an idea that we keep actually valid page on the object' queue but do not mark it as valid.
Might be, unmapping the shadowed page instead would also work, and then it is IMO both cleaner and possibly slightly optimizes parallel faults.

Well, that would also unmap unrelated mappings of the same backing page, resulting in unnecessary page faults.

Why? pmap_remove() would only remove this mapping at the faulted address from this pmap().

I was thinking of using pmap_remove_all(m) (this was my first solution actually). pmap_remove() takes a vaddr instead of a physical page, which implies that we know what is mapped at that vaddr. But we're restarting the fault because the map changed -- how do we know that it is safe and correct to pmap_remove() at that address? What if something else was mapped there while locks were dropped, e.g., a non-transparent superpage?

Then we would remove the superpage mapping, which would be restored by next populate fault. The only thing that seems needs a change then, is the allowance to remove when partial address range is specified for non-transparent superpage. Perhaps yes, this is enough argument pro your approach.

Plus, it's more expensive. To me that seems even worse, but I'm not sure how it would optimize parallel faults.

If another thread accesses the same address while retrying thread restarts the fault, that thread already gets the valid page to map (soft fault). And parallel accesses to the same address is not that rare.

If another threads writes to the same page, yes. On the other hand, if another thread is reading the page, it must now take a page fault to re-establish the mapping.

For read faults, it is same: the page is resident but not mapped.

Well, in my scenario the backing page is mapped RO. After the fault is restarted, pmap_enter() from the COW thread will replace the mapping, but the reading thread does not need to enter the kernel.

In general this might not be the case though, indeed.