Swap in WKILLED processes.

[kib]

Summary

Swapped-out process that is WKILLED must be swapped in as soon as possible. The reason is that such process can be killed by OOM and its pages can be only freed if the process exits. To exit, the kernel stack of the process must be mapped.

When allocating pages for the stack of the WKILLED process on swap in, use VM_ALLOC_SYSTEM requests to increase the chance of the allocation to succeed.

Add counter of the swapped out processes to avoid unneeded iteration over the allprocs list when there is no work to do, reducing the allproc_lock ownership.

Test Plan

Peter, could you, please, test this change ?

[markj]

sys/vm/vm_swapout.c

646 ↗

(On Diff #45969)

This can't be done with the non-sleepable proc lock held.

[kib]

sys/vm/vm_swapout.c

646 ↗

(On Diff #45969)

Yes. Also, the decrement can be done only if P_INMEM was clear.

[kib]

Fix bugs with the decrement of the swapped out counter.

[alc]

sys/kern/kern_sig.c

3081 ↗

(On Diff #46028)

Wouldn't it make sense to test INMEM, and perhaps SWAPPINGIN, before calling wakeup? Only rarely would the wakeup be needed.

[alc]

sys/vm/vm_swapout.c

607 ↗

(On Diff #46028)

Do we actually need the oom_swapin flag? Can't this function simply test for the WKILLED flag?

[kib]

Remove faultin1().
Avoid some wakeups in proc_wkilled().
Pass the vm_page_alloc() flag to vm_thread_swapin() instead of oom indicator.

[alc]

sys/vm/vm_swapout.c

690 ↗

(On Diff #46230)

Now that there is no extra argument to faultin(), can't you eliminate this call, and let swapper() perform it? Also, at the same time, I'm tempted to suggest that the return value be the struct proc * rather than a bool, and that struct proc * could already be locked when you return to swapper().

734 ↗

(On Diff #46230)

Aren't we missing initialization and reset of p to NULL?

[kib]

Restructure the code, hiding all swap in selection in swapper_selector().

[alc]

sys/vm/vm_swapout.c

576 ↗

(On Diff #46251)

Was the insertion of a space after the cast intentional?

[alc]

sys/vm/vm_swapout.c

687 ↗

(On Diff #46251)

Please add another space after the period.

[kib]

sys/vm/vm_swapout.c

576 ↗

(On Diff #46251)

Yes (almost). It happen, and then I decided to keep it because there seems to be more cases with the space, then without, in sys/vm and sys/ufs. What do you prefer ?

Hm, there is an example in style(9) without space. I will remove it in the next version of the patch.

[alc]

sys/vm/vm_swapout.c

576 ↗

(On Diff #46251)

It was my recollection that style(9) said no space. That's why I mentioned it.

[kib]

Style fixes.

[alc]

This comment applies to both the existing and this new version. I do wonder if it really makes sense to swap in more than one non-killed process at a time. It will take a while for a non-killed process to page in its working set, and until that happens we really don't know if there will be enough free memory for yet another process to be swapped in.

[kib]

In D16489#352169, @alc wrote:

This comment applies to both the existing and this new version. I do wonder if it really makes sense to swap in more than one non-killed process at a time. It will take a while for a non-killed process to page in its working set, and until that happens we really don't know if there will be enough free memory for yet another process to be swapped in.

I will code the serialization of swap in for !WKILLED processes, after this patch is finalized.

Since faultin() is synchronous, we only should prevent swapper from starting swapin until any other thread does it, right ?

[alc]

In D16489#352181, @kib wrote:

In D16489#352169, @alc wrote:

This comment applies to both the existing and this new version. I do wonder if it really makes sense to swap in more than one non-killed process at a time. It will take a while for a non-killed process to page in its working set, and until that happens we really don't know if there will be enough free memory for yet another process to be swapped in.

I will code the serialization of swap in for !WKILLED processes, after this patch is finalized.

Sounds good.

Since faultin() is synchronous, we only should prevent swapper from starting swapin until any other thread does it, right ?

Checking ...

[alc]

In D16489#352342, @alc wrote:

In D16489#352181, @kib wrote:

Since faultin() is synchronous, we only should prevent swapper from starting swapin until any other thread does it, right ?

Checking ...

I'm not 100% sure that I actually understand this question. Can you please elaborate?

[kib]

In D16489#352344, @alc wrote:

In D16489#352342, @alc wrote:

In D16489#352181, @kib wrote:

Since faultin() is synchronous, we only should prevent swapper from starting swapin until any other thread does it, right ?

Checking ...

I'm not 100% sure that I actually understand this question. Can you please elaborate?

I mean that faultin() blocks the calling thread until all stack pages are allocated, read and mapped. So all calls to faultin() from the swapper thread are already serialized. We only need to care about faultin() calls outside the swapper (the only caller is PHOLD()).

So I think that a mechanism that prevents swapper from starting faultin() on non-WKILLED process while there is a thread doing faultin() elsewere, would be good enough.

[pho]

I ran all of the stress2 tests on amd64 and a few on i386.
No problems seen.