Changeset View
Standalone View
sys/vm/vm_swapout.c
Show First 20 Lines • Show All 724 Lines • ▼ Show 20 Lines | |||||
* process is swapped out. | * process is swapped out. | ||||
*/ | */ | ||||
static void | static void | ||||
swapout_procs(int action) | swapout_procs(int action) | ||||
{ | { | ||||
struct proc *p; | struct proc *p; | ||||
struct thread *td; | struct thread *td; | ||||
int minslptime, slptime; | int minslptime, slptime; | ||||
bool didswap; | bool didswap, doswap; | ||||
minslptime = 100000; | minslptime = 100000; | ||||
didswap = false; | didswap = false; | ||||
retry: | |||||
sx_slock(&allproc_lock); | sx_slock(&allproc_lock); | ||||
FOREACH_PROC_IN_SYSTEM(p) { | FOREACH_PROC_IN_SYSTEM(p) { | ||||
PROC_LOCK(p); | |||||
/* | /* | ||||
* Watch out for a process in | * Filter out not yet fully constructed processes. Do | ||||
* creation. It may have no | * not swap out held processes. Avoid processes which | ||||
* address space or lock yet. | * are system, exiting, traced, already swapped out or | ||||
* are in the process of being swapped in or out. | |||||
*/ | */ | ||||
alc: I would strive for consistency between the order in which conditions are described in the… | |||||
if (p->p_state == PRS_NEW) { | PROC_LOCK(p); | ||||
if (p->p_state != PRS_NORMAL || p->p_lock != 0 || (p->p_flag & | |||||
(P_SYSTEM | P_WEXIT | P_STOPPED_SINGLE | P_TRACED | | |||||
P_SWAPPINGOUT | P_SWAPPINGIN | P_INMEM)) != P_INMEM) { | |||||
Done Inline ActionsP_SYSTEM is duplicated. markj: P_SYSTEM is duplicated. | |||||
PROC_UNLOCK(p); | PROC_UNLOCK(p); | ||||
continue; | continue; | ||||
} | } | ||||
/* | |||||
* An aio daemon switches its | |||||
* address space while running. | |||||
* Perform a quick check whether | |||||
* a process has P_SYSTEM. | |||||
* Filter out exiting processes. | |||||
*/ | |||||
if ((p->p_flag & (P_SYSTEM | P_WEXIT)) != 0) { | |||||
PROC_UNLOCK(p); | |||||
continue; | |||||
} | |||||
_PHOLD_LITE(p); | _PHOLD_LITE(p); | ||||
PROC_UNLOCK(p); | |||||
sx_sunlock(&allproc_lock); | sx_sunlock(&allproc_lock); | ||||
Not Done Inline ActionsDo we still need to hold the process? markj: Do we still need to hold the process? | |||||
Not Done Inline ActionsI am not sure. swapout() drops the process lock, but it does that after all threads are marked as swapped out, which prevents them from running (see the comment above vm_thread_swapout() loop). Most likely, the process lock is dropped to avoid creating the order between object and process locks. So exit1() cannot be called while the lock is dropped, and parallel swap-in/out are blocked by the process flags. OTOH, hold around swapout was the historic behavior, and it might be needed in future again. If both you and Alan consider stopping holding the process a good idea, I will add it to the patch. kib: I am not sure.
swapout() drops the process lock, but it does that after all threads are marked… | |||||
Not Done Inline ActionsI'm confused - we PRELE() the process before calling swapout(). markj: I'm confused - we PRELE() the process before calling swapout(). | |||||
Not Done Inline ActionsAh, yes, I am confused as well. Now I see another reason for hold. Look at the loop end, we need to ensure that p is still alive when we iterate over the p_list. I think this is a clear reason for hold. kib: Ah, yes, I am confused as well.
Now I see another reason for hold. Look at the loop end, we… | |||||
Not Done Inline Actions
Are you saying that the PRELE() before swapout() is a recent change? If we could maintain the hold across the swapout() then we wouldn't need to restart the FOREACH_PROC_IN_SYSTEM() iterator every time that we pick a process to swap out. I don't see the virtue in restarting the iterator. alc: > OTOH, hold around swapout was the historic behavior, and it might be needed in future again. | |||||
Not Done Inline ActionsI think that we should add a comment describing why the hold is performed. alc: I think that we should add a comment describing why the hold is performed. | |||||
Not Done Inline Actions"Further consideration of this process for swap out requires iterating over its threads. We release allproc_lock here so that process creation and destruction are not blocked while we iterate. To later reacquire allproc_lock and resume iteration over the allproc list, we will first have to release the lock on this process. We place a hold on this process so that it remains in the allproc list while it is unlocked." alc: "Further consideration of this process for swap out requires iterating over its threads. We… | |||||
PROC_LOCK(p); | doswap = true; | ||||
if (p->p_lock != 1 || (p->p_flag & (P_STOPPED_SINGLE | | |||||
P_TRACED | P_SYSTEM)) != 0) | |||||
goto nextproc; | |||||
/* | |||||
* Only aiod changes vmspace. However, it will be | |||||
* skipped because of the if statement above checking | |||||
* for P_SYSTEM. | |||||
*/ | |||||
if ((p->p_flag & (P_INMEM | P_SWAPPINGOUT | P_SWAPPINGIN)) != | |||||
P_INMEM) | |||||
goto nextproc; | |||||
switch (p->p_state) { | |||||
default: | |||||
/* | |||||
* Don't swap out processes in any sort | |||||
* of 'special' state. | |||||
*/ | |||||
break; | |||||
case PRS_NORMAL: | |||||
/* | |||||
* do not swapout a realtime process | |||||
* Check all the thread groups.. | |||||
*/ | |||||
FOREACH_THREAD_IN_PROC(p, td) { | FOREACH_THREAD_IN_PROC(p, td) { | ||||
thread_lock(td); | thread_lock(td); | ||||
if (PRI_IS_REALTIME(td->td_pri_class)) { | |||||
thread_unlock(td); | |||||
goto nextproc; | |||||
} | |||||
slptime = (ticks - td->td_slptick) / hz; | slptime = (ticks - td->td_slptick) / hz; | ||||
Not Done Inline ActionsIn hindsight, "... on the process ..." sounds better. alc: In hindsight, "... on the process ..." sounds better. | |||||
/* | |||||
* Guarantee swap_idle_threshold1 | |||||
* time in memory. | |||||
*/ | |||||
if (slptime < swap_idle_threshold1) { | |||||
thread_unlock(td); | |||||
goto nextproc; | |||||
} | |||||
/* | /* | ||||
Not Done Inline ActionsI think that there is a missing space here before the "*/". alc: I think that there is a missing space here before the "*/". | |||||
Not Done Inline ActionsI debated whether to mention the temporary release of the process lock in swapout() as reason for doing the hold so early. I'm happy to write another sentence if you like. alc: I debated whether to mention the temporary release of the process lock in swapout() as reason… | |||||
Not Done Inline ActionsI think that the point where we hold the process is not very important as far as we do it before the first process unlock. But if you have a good wording for a note explaining it, of course it is useful to include the text. kib: I think that the point where we hold the process is not very important as far as we do it… | |||||
* Do not swapout a process if it is | * Do not swapout a realtime process. | ||||
* waiting on a critical event of some | * Guarantee swap_idle_threshold1 time in memory. | ||||
* kind or there is a thread whose | * If the system is under memory stress, or if | ||||
* pageable memory may be accessed. | * we are swapping idle processes >= | ||||
* | * swap_idle_threshold2, then swap the process | ||||
* This could be refined to support | * out. | ||||
* swapping out a thread. | |||||
*/ | */ | ||||
Not Done Inline ActionsI would suggest moving this comment outside the loop, just before initializing doswap. alc: I would suggest moving this comment outside the loop, just before initializing doswap. | |||||
if (!thread_safetoswapout(td)) { | if (PRI_IS_REALTIME(td->td_pri_class) || | ||||
thread_unlock(td); | slptime < swap_idle_threshold1 || | ||||
goto nextproc; | !thread_safetoswapout(td) || | ||||
} | ((action & VM_SWAP_NORMAL) == 0 && | ||||
/* | |||||
* If the system is under memory stress, | |||||
* or if we are swapping | |||||
* idle processes >= swap_idle_threshold2, | |||||
* then swap the process out. | |||||
*/ | |||||
if ((action & VM_SWAP_NORMAL) == 0 && | |||||
((action & VM_SWAP_IDLE) == 0 || | ((action & VM_SWAP_IDLE) == 0 || | ||||
Not Done Inline ActionsFor this function to be called, at least one of VM_SWAP_NORMAL and VM_SWAP_IDLE must be set. If (action & VM_SWAP_NORMAL) == 0 is true, then (action & VM_SWAP_IDLE) == 0 must be false. So, checking (action & VM_SWAP_IDLE) == 0 is pointless. alc: For this function to be called, at least one of VM_SWAP_NORMAL and VM_SWAP_IDLE must be set. | |||||
slptime < swap_idle_threshold2)) { | slptime < swap_idle_threshold2))) | ||||
doswap = false; | |||||
thread_unlock(td); | thread_unlock(td); | ||||
goto nextproc; | |||||
} | |||||
if (minslptime > slptime) | if (!doswap) | ||||
break; | |||||
else if (minslptime > slptime) | |||||
minslptime = slptime; | minslptime = slptime; | ||||
thread_unlock(td); | |||||
} | } | ||||
/* | /* | ||||
Done Inline ActionsDelete this blank line? The next two lines after the blank fit with the prior block comment. alc: Delete this blank line? The next two lines after the blank fit with the prior block comment. | |||||
* If the pageout daemon didn't free enough pages, | * If the pageout daemon didn't free enough pages, | ||||
* or if this process is idle and the system is | * or if this process is idle and the system is | ||||
* configured to swap proactively, swap it out. | * configured to swap proactively, swap it out. | ||||
Not Done Inline ActionsThis comment essentially repeats the comment from inside the loop. alc: This comment essentially repeats the comment from inside the loop. | |||||
*/ | */ | ||||
if ((action & VM_SWAP_NORMAL) != 0 || | if (doswap && ((action & VM_SWAP_NORMAL) != 0 || | ||||
((action & VM_SWAP_IDLE) != 0 && | ((action & VM_SWAP_IDLE) != 0 && | ||||
minslptime > swap_idle_threshold2)) { | minslptime > swap_idle_threshold2)) && | ||||
_PRELE(p); | swapout(p) == 0) | ||||
if (swapout(p) == 0) | |||||
didswap = true; | didswap = true; | ||||
/* | |||||
* Hold on the process prevents its exit. The | |||||
Not Done Inline Actions"The hold on ... The next iteration ... alc: "The hold on ... The next iteration ... | |||||
* iteration, which dereferences p->p_list, can safely | |||||
* continue after the allproc_lock is reacquired. | |||||
*/ | |||||
Not Done Inline ActionsI think that this comment is now subsumed by the earlier one. alc: I think that this comment is now subsumed by the earlier one. | |||||
PROC_UNLOCK(p); | PROC_UNLOCK(p); | ||||
goto retry; | |||||
} | |||||
} | |||||
nextproc: | |||||
PROC_UNLOCK(p); | |||||
sx_slock(&allproc_lock); | sx_slock(&allproc_lock); | ||||
PRELE(p); | PRELE(p); | ||||
} | } | ||||
sx_sunlock(&allproc_lock); | sx_sunlock(&allproc_lock); | ||||
/* | /* | ||||
* If we swapped something out, and another process needed memory, | * If we swapped something out, and another process needed memory, | ||||
* then wakeup the sched process. | * then wakeup the sched process. | ||||
*/ | */ | ||||
if (didswap) | if (didswap) | ||||
wakeup(&proc0); | wakeup(&proc0); | ||||
} | } | ||||
▲ Show 20 Lines • Show All 82 Lines • Show Last 20 Lines |
I would strive for consistency between the order in which conditions are described in the comment and tested in the code.