Page MenuHomeFreeBSD

(vm object 1) Replace busy checks and sleeps with acquires where it is trivial to do so.
ClosedPublic

Authored by jeff on Sep 6 2019, 12:43 PM.

Details

Summary

This patch converts most busy tests to acquires. This furthers the goal of converting busy to an actual lock that functions without the object lock held. This should be functionally a nop for now.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jeff created this revision.Sep 6 2019, 12:43 PM
jeff edited the summary of this revision. (Show Details)Sep 6 2019, 12:45 PM
jeff added reviewers: alc, kib, markj, dougm.
jeff retitled this revision from Replace busy checks and sleeps with acquires where it is trivial to do so. to (vm object 1) Replace busy checks and sleeps with acquires where it is trivial to do so..Sep 10 2019, 7:59 PM
kib added inline comments.Sep 14 2019, 5:30 PM
sys/kern/vfs_bio.c
3644 ↗(On Diff #61725)

So effectively this drains the xbusy state on bdwrite(). Do we need this ?

sys/vm/vm_object.c
718 ↗(On Diff #61725)

Why is this assert moved ?

893 ↗(On Diff #61725)

I somewhat dislike the lost of unique wmsgs for busy acquire. Might be, add wmsg arg to vm_page_busy_acquire() and use default msg if the arg is NULL ?

1425 ↗(On Diff #61725)

Assert that ?

jeff added a comment.Sep 14 2019, 6:45 PM

One possible refinement that I should mention;

We have many places that want to do I/O and really just want to be certain that they are the first shared busy holder but don't really want an exclusive lock. Today I implement that with xbusy & downgrade. It could be a special kind of first-sbusy-only acquire. This is relevant in page clustering for write back, for example, where you don't want to do work that is already being done.

sys/kern/vfs_bio.c
3644 ↗(On Diff #61725)

Yes without it we are relying on the object lock to protect valid/dirty. Before we would make sure that busy was not held by checking it, now we make sure by acquiring it. The result is the same until you have consumers that busy without the object lock.

At the end of the patchset this is no longer sufficient and the object lock is not even acquired here.

sys/vm/vm_object.c
718 ↗(On Diff #61725)

While the page is wired it could be busied by something like the buffer cache. I don't believe there is code that would trip this race today because we won't tear down a vm object while pages are in the buffer cache but it seemed prudent at the time. Perhaps it is unnecessary to move it.

893 ↗(On Diff #61725)

I can add the argument back if we really want.

1425 ↗(On Diff #61725)

In patch 2 this is changed to not release and the comment goes away. This comment is mirroring another one from vm_fault.c which documents the same behavior.

kib accepted this revision.Sep 14 2019, 7:39 PM
kib added inline comments.
sys/vm/vm_object.c
893 ↗(On Diff #61725)

I think it would be useful. Often even knowing the wchan for busy wait provides clue.

This revision is now accepted and ready to land.Sep 14 2019, 7:39 PM
alc added inline comments.Sep 15 2019, 3:27 AM
sys/vm/phys_pager.c
214 ↗(On Diff #61725)

This assignment is redundant. Passing TRUE to vm_page_zero_invalid() results in the page being marked valid.

jeff updated this revision to Diff 62923.Oct 4 2019, 10:28 PM

Fixed wired/busy check order.

This revision now requires review to proceed.Oct 4 2019, 10:28 PM
kib accepted this revision.Oct 8 2019, 8:40 AM
This revision is now accepted and ready to land.Oct 8 2019, 8:40 AM