Split active and inactive queue scans into separate functions.
ClosedPublic
Actions

Authored by markj on May 19 2018, 7:30 PM.

Details

Reviewers

alc
kib
jeff

Commits

rS334179: Update r334154 with review feedback from D15490.
rS334154: Split the active and inactive queue scans into separate subroutines.

Summary

The aim here is to address a problem reported by avg, where we
erroneously pause() after a scan in a situation where we should really
be continuing to scan the active queue in order to address a shortfall
of inactive pages. Consider an application with a read-only mapping of a
file several times the size of RAM, which is accessing blocks in a
random order. Most of the system's pages will end up in the active
queue, and during memory shortages we will end up pause()ing for a
second at a time in between scans. The pause() is intended to provide
time for laundering to complete, and is incorrect in this scenario.
Having a separate subroutine for active queue scans makes it easier for
the control loop to determine if the active queue scan met its target.

The change should not have any functional impact. I think the use of the
"pass" parameter doesn't make much sense anymore, and is incorrect
anyway since it never gets reset to 0 in the current code. I will
address this in a separate commit.

Test Plan

So far I've only verified that we boot.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.May 19 2018, 7:30 PM

Harbormaster completed remote builds in B16701: Diff 42740.May 19 2018, 7:30 PM

markj edited the test plan for this revision. (Show Details)May 19 2018, 7:37 PM

markj added reviewers: alc, kib, jeff.

markj added a subscriber: avg.

kib accepted this revision.May 19 2018, 8:49 PM

This revision is now accepted and ready to land.May 19 2018, 8:49 PM

alc accepted this revision.May 22 2018, 4:29 PM

alc added inline comments.

sys/vm/vm_pageout.c
1120 ↗	(On Diff #42740)	I don't think that the meaning of "that they can eventually be reused" is clear.
1123 ↗	(On Diff #42740)	I don't think that this function needs to have "scan_" in its name.
1172–1173 ↗	(On Diff #42740)	This is, perhaps, unrelated to this particular change, but I point out that it's unclear what we are maintaining "consistency" with. Also, I'm glad to see that our discussions last year about avoiding requeueing on the active queue have come to fruition.
1258–1259 ↗	(On Diff #42740)	I think there is a missing step in the explanation here. I suggest: "However, during a page shortage, the inactive queue is necessarily small, and so dirty pages would only spend a trivial amount of time in the inactive queue. Therefore, we might as well place them directly in the laundry queue to reduce queueing overhead."