Eliminate vm object relocks in vm fault.

[kib]

Summary

There are actually two unrelated changed.

First one, stops taking the object wlock after the pmap_enter() is done for the main vm_fault flow. The lock is not needed there for anything, I believe, except for dropping the paging in progress count. I moved the pip decrement before pmap_enter() while still owning vm object lock. This makes it possible for vm_object_terminate_pages() to find a busy page on the object queue. Instead of asserting that the pages are not busy, I restart the termination loop after the state passed. I expect that vm_page_free_prep() is idempotent.

Second one is less delicate. For the vm_fault_prefault() call from vm_fault_soft_fast(), extend the scope of the object rlock to avoid re-taking it inside vm_fault_prefault(). It causes pmap_enter_quick() sometimes called with shadow object lock as well as the page lock, but this looks innocent.

Both cases were pointed out by mjg. who also benchmarked the change. The patches were not tested otherwise yet.

[kib]

If the object is unlocked around vm_page_xunbusy(), vm_object_terminate_pages() might see busy pages.

[mjg]

I made the change flippable with a sysctl. It nicely speeds up the initital page faults when postgres warms up:

https://people.freebsd.org/~mjg/pgsql-D15122.png

x axis is time in seconds, y is queries

this is a postgres process bound to 64 threads on 2 sockets, while pgbench runs over a unix socket and is bound to a different set of threads. The test is a 64-way select-only. In order to get rid of other obfuscations it runs with an experimental poll -> kqueue patch. Postgres was restarted multiple times and retested with both old and new codepath, flippable thanks to the sysctl.

The top waiting for vm object spot is unlock_and_deallocate2 -> vm_object_deallocate

[markj]

I expect that vm_page_free_prep() is idempotent.

(I did not look closely at the diff yet.)

Hrm. I noted in D14707 that vm_page_free_prep() is no longer idempotent after that revision, because vm_reserv_free_page() is not idempotent.

[kib]

Make vm_page_free_prep() idempotent by tracking the calls.

[markj]

sys/vm/vm_object.c

743 ↗

(On Diff #41634)

Don't we potentially need to unlock mtx and the pagequeue here?

[kib]

sys/vm/vm_object.c

743 ↗

(On Diff #41634)

Pagequeue certainly, but the page must be locked. Of course, I did it wrong.

[kib]

Attempt to improve locking.

[kib]

Update after the queuing batch commit.

[markj]

sys/vm/vm_object.c

743 ↗

(On Diff #41826)

Hmm, is it possible for a page belonging to an unmanaged object to be busy?

sys/vm/vm_page.c

3494 ↗

(On Diff #41826)

I don't think this is sufficient: PG_FREEPREP needs to be set before the vm_reserv_free_page() call. If the page was indeed allocated from a reservation, we will not set PG_FREEPREP before returning, and a second call will attempt to free the page back to the reservation a second time.

sys/vm/vm_page.h

380 ↗

(On Diff #41826)

I would s/called/completed/.

[markj]

sys/vm/vm_object.c

743 ↗

(On Diff #41826)

Never mind, it must be possible.

[kib]

Handle Mark' notes.

[markj]

I can't see any problems with this. The PG_FREEPREP flag is not very satisfying, but I don't have an alternative proposal.

[kib]

mjg@, does this still provides an improvement in your benchmarks ? Can you provide the numbers ?

[mjg]

In D15122#320743, @kib wrote:

mjg@, does this still provides an improvement in your benchmarks ? Can you provide the numbers ?

Same test, pretty much same win:
https://people.freebsd.org/~mjg/D15122/pgsql-D15122-2.png

See the directory for source files.

[kib]

Peter, could you, please, test this change ?

[pho]

In D15122#320768, @kib wrote:

Peter, could you, please, test this change ?

Sure. Building now.

[kib]

Remove assert which is no longer holds since the object lock does not prevent page unbusy.

Reported by: pho

[alc]

The vm_fault_prefault() changes look good. Please commit them.

[pho]

LGTM.

[kib]

The rest of the patch, left after the commit.

[kib]

In D15122#321014, @pho wrote:

LGTM.

To make it clear, Peter tested the whole patch.