vm_pageout: Avoid rounding down the inactive scan target

[markj]

Summary

With helper page daemon threads, we divide the inactive target by the
number of threads, rounding down, and sum the total number of pages
freed by the threads. This sum is compared with the original target,
but by rounding down we might lose pages, causing the page daemon
control loop to conclude that inactive queue scanning isn't keeping up
with demand for free pages. Typically this results in excessive
swapping.

Fix the problem by accounting for the error in the main pagedaemon
thread's target. Initially I wanted to just set the per-thread target
to howmany(shortage, threads), but I think the code is easier to reason
about if we aim to meet the global target precisely.

[cem]

I am unfamiliar with the wider system effects, but given the summary of the problem, the solution looks correct to me.

[dhw]

After having noted the problem (during a test for merging from r364434 to r366011), I tested again after applying the patch. [Gleb actually applied the patch and built the image; I ran the tests] Test cluster is 22 control/canary pairs of machines, each pair of which has slightly different hardware -- all amd64, though; the problem was evident with 18 of the canaries without the patch, and after applying the patch, no issues were seen.

[alc]

sys/vm/vm_pageout.c

1667–1668 ↗

(On Diff #77689)

Is there any reason why these assignments can't be performed before the domain lock is acquired?

[markj]

Move assignments out of the domain lock scope.

[markj]

sys/vm/vm_pageout.c

1667–1668 ↗

(On Diff #77689)

No, there shouldn't be. The helper threads will be sleeping at this point, and each will acquire and release the domain lock in turn after being woken up before reading the value of vmd_inactive_shortage.

[dougm]

sys/vm/vm_pageout.c

1666 ↗

(On Diff #77762)

Doesn't slop = 1 work too? With slop>1 you seem to be scanning more than you need.

Disclaimer - I don't know this code at all, but still, it looks wrong.

[markj]

sys/vm/vm_pageout.c

1666 ↗

(On Diff #77762)

I don't quite see why it looks wrong - we want to have vmd_inactive_shortage * threads + slop == shortage.

To provide a bit more background, we have threads - 1 helper threads, each of which calls vm_pageout_scan_inactive() to reclaim vmd_inactive_shortage pages. The "main" thread reclaims vmd_inactive_shortage + slop pages. Each thread adds the number of pages reclaimed to vmd_inactive_freed, and once all threads have finished the difference shortage - freed should be positive if and only if the threads failed to meet the target (typically due to a shortage of reclaimable pages).

[dougm]

sys/vm/vm_pageout.c

1665 ↗

(On Diff #77762)

Is threads initialized here? I don't think so.

If threads were initialized before the if (), couldn't vmd->vmd_inactive_shortage and slop also be initialized before the if()?

1666 ↗

(On Diff #77762)

Thanks for the clarification.

[markj]

Don't use an uninitialized variable.

[markj]

Try to simplify a bit.