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

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

Details

Reviewers

alc
kib
markj
dougm

Commits

rS353535: (1/6) Replace busy checks with acquires where it is trival to do so.

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jeff created this revision.Sep 6 2019, 12:43 PM

Harbormaster completed remote builds in B26307: Diff 61725.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 ?

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.