Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1420729 > unrolled thread
| Started by | Michal Hocko <mhocko@kernel.org> |
|---|---|
| First post | 2016-06-13 13:30 +0200 |
| Last post | 2016-06-17 15:30 +0200 |
| Articles | 9 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem Michal Hocko <mhocko@kernel.org> - 2016-06-13 13:30 +0200
Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-06-16 15:00 +0200
Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem Michal Hocko <mhocko@kernel.org> - 2016-06-16 16:40 +0200
Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-06-16 17:50 +0200
Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem Michal Hocko <mhocko@kernel.org> - 2016-06-16 18:00 +0200
Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-06-17 13:40 +0200
Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem Michal Hocko <mhocko@kernel.org> - 2016-06-17 14:30 +0200
Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-06-17 15:20 +0200
Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem Michal Hocko <mhocko@kernel.org> - 2016-06-17 15:30 +0200
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-06-13 13:30 +0200 |
| Subject | Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem |
| Message-ID | <rJye5-5AU-5@gated-at.bofh.it> |
On Sat 11-06-16 17:10:03, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > > Also, I think setting TIF_MEMDIE on p when find_lock_task_mm(p) != p is
> > > wrong. While oom_reap_task() will anyway clear TIF_MEMDIE even if we set
> > > TIF_MEMDIE on p when p->mm == NULL, it is not true for CONFIG_MMU=n case.
> >
> > Yes this would be racy for !CONFIG_MMU but does it actually matter?
>
> I don't know because I've never used CONFIG_MMU=n kernels. But I think it
> actually matters. You fixed this race by commit 83363b917a2982dd ("oom:
> make sure that TIF_MEMDIE is set under task_lock").
Yes and that commit was trying to address a highly theoretical issue
reported by you. Let me quote:
:oom_kill_process is currently prone to a race condition when the OOM
:victim is already exiting and TIF_MEMDIE is set after the task releases
:its address space. This might theoretically lead to OOM livelock if the
:OOM victim blocks on an allocation later during exiting because it
:wouldn't kill any other process and the exiting one won't be able to exit.
:The situation is highly unlikely because the OOM victim is expected to
:release some memory which should help to sort out OOM situation.
Even if such a race is possible it wouldn't be with the oom
reaper. Regarding CONFIG_MMU=n I am even less sure it is possible and
I would rather focus on CONFIG_MMU=y where we know that problems exist
rather than speculating about something as special as nommu which even
might not care at all.
--
Michal Hocko
SUSE Labs
[toc] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2016-06-16 15:00 +0200 |
| Message-ID | <rKF3Q-8v4-7@gated-at.bofh.it> |
| In reply to | #1420729 |
Michal Hocko wrote:
> On Sat 11-06-16 17:10:03, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > > Also, I think setting TIF_MEMDIE on p when find_lock_task_mm(p) != p is
> > > > wrong. While oom_reap_task() will anyway clear TIF_MEMDIE even if we set
> > > > TIF_MEMDIE on p when p->mm == NULL, it is not true for CONFIG_MMU=n case.
> > >
> > > Yes this would be racy for !CONFIG_MMU but does it actually matter?
> >
> > I don't know because I've never used CONFIG_MMU=n kernels. But I think it
> > actually matters. You fixed this race by commit 83363b917a2982dd ("oom:
> > make sure that TIF_MEMDIE is set under task_lock").
>
> Yes and that commit was trying to address a highly theoretical issue
> reported by you. Let me quote:
> :oom_kill_process is currently prone to a race condition when the OOM
> :victim is already exiting and TIF_MEMDIE is set after the task releases
> :its address space. This might theoretically lead to OOM livelock if the
> :OOM victim blocks on an allocation later during exiting because it
> :wouldn't kill any other process and the exiting one won't be able to exit.
> :The situation is highly unlikely because the OOM victim is expected to
> :release some memory which should help to sort out OOM situation.
>
> Even if such a race is possible it wouldn't be with the oom
> reaper. Regarding CONFIG_MMU=n I am even less sure it is possible and
> I would rather focus on CONFIG_MMU=y where we know that problems exist
> rather than speculating about something as special as nommu which even
> might not care at all.
I still don't like it. current->mm == NULL in
- if (current->mm &&
- (fatal_signal_pending(current) || task_will_free_mem(current))) {
+ if (task_will_free_mem(current)) {
is not highly unlikely. You obviously break commit d7a94e7e11badf84
("oom: don't count on mm-less current process") on CONFIG_MMU=n kernels.
Also, since commit f44666b04605d1c7 ("mm,oom: speed up select_bad_process() loop")
changed to iterate using thread group leaders, it is no longer highly unlikely
that p is a thread group leader which already released mm. What you call "a highly
theoretical issue" (which is true as of commit 83363b917a2982dd ("oom: make sure
that TIF_MEMDIE is set under task_lock") was proposed) may not be true any more.
Regarding CONFIG_MMU=n kernels, making sure that TIF_MEMDIE is set on a thread
with non-NULL mm does matter.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-06-16 16:40 +0200 |
| Message-ID | <rKGCB-14P-13@gated-at.bofh.it> |
| In reply to | #1424028 |
On Thu 16-06-16 21:54:27, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Sat 11-06-16 17:10:03, Tetsuo Handa wrote:
[...]
> I still don't like it. current->mm == NULL in
>
> - if (current->mm &&
> - (fatal_signal_pending(current) || task_will_free_mem(current))) {
> + if (task_will_free_mem(current)) {
>
> is not highly unlikely. You obviously break commit d7a94e7e11badf84
> ("oom: don't count on mm-less current process") on CONFIG_MMU=n kernels.
I still fail to see why you care about that case so much. The heuristic
was broken for other reasons before this patch. The patch fixes a class
of issues for both mmu and nommu. I can restore the current->mm check
for now but the more I am thinking about it the less I am sure the
commit you are referring to is evem correct/necessary.
It claims that the OOM killer would be stuck because the child would be
sitting in the final schedule() until the parent reaps it. That is not
true, though, because victim would be unhashed down in release_task()
path so it is not visible by the oom killer when it is waiting for the
parent. I have completely missed that part when reviewing the patch. Or
am I missing something...
Anyway, would you be OK with the patch if I added the current->mm check
and resolve its necessity in a separate patch?
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2016-06-16 17:50 +0200 |
| Message-ID | <rKHIm-1K6-21@gated-at.bofh.it> |
| In reply to | #1424129 |
Michal Hocko wrote:
> On Thu 16-06-16 21:54:27, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Sat 11-06-16 17:10:03, Tetsuo Handa wrote:
> [...]
> > I still don't like it. current->mm == NULL in
> >
> > - if (current->mm &&
> > - (fatal_signal_pending(current) || task_will_free_mem(current))) {
> > + if (task_will_free_mem(current)) {
> >
> > is not highly unlikely. You obviously break commit d7a94e7e11badf84
> > ("oom: don't count on mm-less current process") on CONFIG_MMU=n kernels.
>
> I still fail to see why you care about that case so much. The heuristic
> was broken for other reasons before this patch. The patch fixes a class
> of issues for both mmu and nommu. I can restore the current->mm check
> for now but the more I am thinking about it the less I am sure the
> commit you are referring to is evem correct/necessary.
>
> It claims that the OOM killer would be stuck because the child would be
> sitting in the final schedule() until the parent reaps it. That is not
> true, though, because victim would be unhashed down in release_task()
> path so it is not visible by the oom killer when it is waiting for the
> parent. I have completely missed that part when reviewing the patch. Or
> am I missing something...
That explanation started from 201411292304.CGF68419.MOLHVQtSFFOOJF@I-love.SAKURA.ne.jp
(Sat, 29 Nov 2014 23:04:33 +0900) in your mailbox. I confirmed that a TIF_MEMDIE
zombie inside the final schedule() in do_exit() is waiting for parent to reap.
release_task() will be called when parent noticed that there is a zombie, but
this OOM livelock situation prevented parent looping inside page allocator waiting
for that TIF_MEMDIE zombie from noticing that there is a zombie.
>
> Anyway, would you be OK with the patch if I added the current->mm check
> and resolve its necessity in a separate patch?
Please correct task_will_free_mem() in oom_kill_process() as well.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-06-16 18:00 +0200 |
| Message-ID | <rKHS1-1Nf-7@gated-at.bofh.it> |
| In reply to | #1424213 |
On Fri 17-06-16 00:40:41, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Thu 16-06-16 21:54:27, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Sat 11-06-16 17:10:03, Tetsuo Handa wrote:
> > [...]
> > > I still don't like it. current->mm == NULL in
> > >
> > > - if (current->mm &&
> > > - (fatal_signal_pending(current) || task_will_free_mem(current))) {
> > > + if (task_will_free_mem(current)) {
> > >
> > > is not highly unlikely. You obviously break commit d7a94e7e11badf84
> > > ("oom: don't count on mm-less current process") on CONFIG_MMU=n kernels.
> >
> > I still fail to see why you care about that case so much. The heuristic
> > was broken for other reasons before this patch. The patch fixes a class
> > of issues for both mmu and nommu. I can restore the current->mm check
> > for now but the more I am thinking about it the less I am sure the
> > commit you are referring to is evem correct/necessary.
> >
> > It claims that the OOM killer would be stuck because the child would be
> > sitting in the final schedule() until the parent reaps it. That is not
> > true, though, because victim would be unhashed down in release_task()
> > path so it is not visible by the oom killer when it is waiting for the
> > parent. I have completely missed that part when reviewing the patch. Or
> > am I missing something...
>
> That explanation started from 201411292304.CGF68419.MOLHVQtSFFOOJF@I-love.SAKURA.ne.jp
> (Sat, 29 Nov 2014 23:04:33 +0900) in your mailbox. I confirmed that a TIF_MEMDIE
> zombie inside the final schedule() in do_exit() is waiting for parent to reap.
> release_task() will be called when parent noticed that there is a zombie, but
> this OOM livelock situation prevented parent looping inside page allocator waiting
> for that TIF_MEMDIE zombie from noticing that there is a zombie.
I cannot seem to find this msg-id. Anyway, let's forget it for now
to not get side tracked. I have to study that code more deeply to better
understand it.
> > Anyway, would you be OK with the patch if I added the current->mm check
> > and resolve its necessity in a separate patch?
>
> Please correct task_will_free_mem() in oom_kill_process() as well.
We cannot hold task_lock over all task_will_free_mem I am even not sure
we have to develop an elaborate way to make it raceless just for the nommu
case. The current case is simple as we cannot race here. Is that
sufficient for you?
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2016-06-17 13:40 +0200 |
| Message-ID | <rL0hX-5Wp-11@gated-at.bofh.it> |
| In reply to | #1424218 |
Michal Hocko wrote:
> > > Anyway, would you be OK with the patch if I added the current->mm check
> > > and resolve its necessity in a separate patch?
> >
> > Please correct task_will_free_mem() in oom_kill_process() as well.
>
> We cannot hold task_lock over all task_will_free_mem I am even not sure
> we have to develop an elaborate way to make it raceless just for the nommu
> case. The current case is simple as we cannot race here. Is that
> sufficient for you?
We can use find_lock_task_mm() inside mark_oom_victim().
That is, call wake_oom_reaper() from mark_oom_victim() like
void mark_oom_victim(struct task_struct *tsk, bool can_use_oom_reaper)
{
WARN_ON(oom_killer_disabled);
/* OOM killer might race with memcg OOM */
tsk = find_lock_task_mm(tsk);
if (!tsk)
return;
if (test_and_set_tsk_thread_flag(tsk, TIF_MEMDIE)) {
task_unlock(tsk);
return;
}
task_unlock(tsk);
atomic_inc(&tsk->signal->oom_victims);
/*
* Make sure that the task is woken up from uninterruptible sleep
* if it is frozen because OOM killer wouldn't be able to free
* any memory and livelock. freezing_slow_path will tell the freezer
* that TIF_MEMDIE tasks should be ignored.
*/
__thaw_task(tsk);
atomic_inc(&oom_victims);
if (can_use_oom_reaper)
wake_oom_reaper(tsk);
}
and move mark_oom_victim() by normal path to after task_unlock(victim).
do_send_sig_info(SIGKILL, SEND_SIG_FORCED, victim, true);
- mark_oom_victim(victim);
- if (can_oom_reap)
- wake_oom_reaper(victim);
+ wake_oom_reaper(victim, can_oom_reap);
If you don't like possibility of showing different pid for
pr_err("Killed process %d (%s)
and
pr_info("oom_reaper: reaped process %d (%s)
messages, you can defer the former till mark_oom_victim() locks that task.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-06-17 14:30 +0200 |
| Message-ID | <rL14m-6sY-39@gated-at.bofh.it> |
| In reply to | #1424962 |
On Fri 17-06-16 20:38:01, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > > > Anyway, would you be OK with the patch if I added the current->mm check
> > > > and resolve its necessity in a separate patch?
> > >
> > > Please correct task_will_free_mem() in oom_kill_process() as well.
> >
> > We cannot hold task_lock over all task_will_free_mem I am even not sure
> > we have to develop an elaborate way to make it raceless just for the nommu
> > case. The current case is simple as we cannot race here. Is that
> > sufficient for you?
>
> We can use find_lock_task_mm() inside mark_oom_victim().
> That is, call wake_oom_reaper() from mark_oom_victim() like
>
> void mark_oom_victim(struct task_struct *tsk, bool can_use_oom_reaper)
> {
> WARN_ON(oom_killer_disabled);
> /* OOM killer might race with memcg OOM */
> tsk = find_lock_task_mm(tsk);
> if (!tsk)
> return;
> if (test_and_set_tsk_thread_flag(tsk, TIF_MEMDIE)) {
> task_unlock(tsk);
> return;
> }
> task_unlock(tsk);
> atomic_inc(&tsk->signal->oom_victims);
> /*
> * Make sure that the task is woken up from uninterruptible sleep
> * if it is frozen because OOM killer wouldn't be able to free
> * any memory and livelock. freezing_slow_path will tell the freezer
> * that TIF_MEMDIE tasks should be ignored.
> */
> __thaw_task(tsk);
> atomic_inc(&oom_victims);
> if (can_use_oom_reaper)
> wake_oom_reaper(tsk);
> }
>
> and move mark_oom_victim() by normal path to after task_unlock(victim).
>
> do_send_sig_info(SIGKILL, SEND_SIG_FORCED, victim, true);
> - mark_oom_victim(victim);
>
> - if (can_oom_reap)
> - wake_oom_reaper(victim);
> + wake_oom_reaper(victim, can_oom_reap);
I do not like this because then we would have to check the reapability
from inside the oom_reaper again.
But let me ask again. Does this really matter so much just because of
nommu where we can fall in different traps? Can we simply focus on mmu
(aka vast majority of cases) make it work reliably and see what we can
do with nommu later?
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2016-06-17 15:20 +0200 |
| Message-ID | <rL1QJ-70p-19@gated-at.bofh.it> |
| In reply to | #1425015 |
Michal Hocko wrote:
> On Fri 17-06-16 20:38:01, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > > > Anyway, would you be OK with the patch if I added the current->mm check
> > > > > and resolve its necessity in a separate patch?
> > > >
> > > > Please correct task_will_free_mem() in oom_kill_process() as well.
> > >
> > > We cannot hold task_lock over all task_will_free_mem I am even not sure
> > > we have to develop an elaborate way to make it raceless just for the nommu
> > > case. The current case is simple as we cannot race here. Is that
> > > sufficient for you?
> >
> > We can use find_lock_task_mm() inside mark_oom_victim().
> > That is, call wake_oom_reaper() from mark_oom_victim() like
> >
> > void mark_oom_victim(struct task_struct *tsk, bool can_use_oom_reaper)
> > {
> > WARN_ON(oom_killer_disabled);
> > /* OOM killer might race with memcg OOM */
> > tsk = find_lock_task_mm(tsk);
> > if (!tsk)
> > return;
> > if (test_and_set_tsk_thread_flag(tsk, TIF_MEMDIE)) {
> > task_unlock(tsk);
> > return;
> > }
> > task_unlock(tsk);
> > atomic_inc(&tsk->signal->oom_victims);
> > /*
> > * Make sure that the task is woken up from uninterruptible sleep
> > * if it is frozen because OOM killer wouldn't be able to free
> > * any memory and livelock. freezing_slow_path will tell the freezer
> > * that TIF_MEMDIE tasks should be ignored.
> > */
> > __thaw_task(tsk);
> > atomic_inc(&oom_victims);
> > if (can_use_oom_reaper)
> > wake_oom_reaper(tsk);
> > }
> >
> > and move mark_oom_victim() by normal path to after task_unlock(victim).
> >
> > do_send_sig_info(SIGKILL, SEND_SIG_FORCED, victim, true);
> > - mark_oom_victim(victim);
> >
> > - if (can_oom_reap)
> > - wake_oom_reaper(victim);
> > + wake_oom_reaper(victim, can_oom_reap);
>
> I do not like this because then we would have to check the reapability
> from inside the oom_reaper again.
I didn't understand why you think so. But strictly speaking, can_oom_reap calculation
in oom_kill_process() is always racy, and [PATCH 10/10] is not safe.
CPU0 (memory allocating task) CPU1 (kthread) CPU2 (OOM victim)
Calls use_mm(victim->mm).
Starts some worker.
Enters out_of_memory().
Enters oom_kill_process().
Finishes some worker.
Calls rcu_read_lock().
Sets can_oom_reap = false due to process_shares_mm() && !same_thread_group() && (p->flags & PF_KTHREAD).
Calls unuse_mm(victim->mm).
Continues scanning other processes.
Calls mmput(victim->mm).
Sends SIGKILL to victim.
Calls rcu_read_unlock().
Leaves oom_kill_process().
Calls do_exit().
Leaves out_of_memory().
Sets victim->mm = NULL from exit_mm().
Calls mmput() from exit_mm().
__mmput() is called because victim was the last user.
Enters out_of_memory().
oom_scan_process_thread() returns OOM_SCAN_ABORT.
Leaves out_of_memory().
__mmput() stalls but the oom_reaper is not called.
For correctness, can_oom_reap needs to be calculated inside the oom_reaper.
>
> But let me ask again. Does this really matter so much just because of
> nommu where we can fall in different traps? Can we simply focus on mmu
> (aka vast majority of cases) make it work reliably and see what we can
> do with nommu later?
To me, timeout based one is sufficient for handling any traps that hit
nommu kernels after the OOM killer is invoked.
Anyway, I don't like this series because this series ignores theoretical cases.
I can't make progress as long as you repeat "does it really matter/occur".
Please go ahead without Reviewed-by: or Acked-by: from me.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-06-17 15:30 +0200 |
| Message-ID | <rL20q-73O-35@gated-at.bofh.it> |
| In reply to | #1425064 |
On Fri 17-06-16 22:12:22, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Fri 17-06-16 20:38:01, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > > > Anyway, would you be OK with the patch if I added the current->mm check
> > > > > > and resolve its necessity in a separate patch?
> > > > >
> > > > > Please correct task_will_free_mem() in oom_kill_process() as well.
> > > >
> > > > We cannot hold task_lock over all task_will_free_mem I am even not sure
> > > > we have to develop an elaborate way to make it raceless just for the nommu
> > > > case. The current case is simple as we cannot race here. Is that
> > > > sufficient for you?
> > >
> > > We can use find_lock_task_mm() inside mark_oom_victim().
> > > That is, call wake_oom_reaper() from mark_oom_victim() like
> > >
> > > void mark_oom_victim(struct task_struct *tsk, bool can_use_oom_reaper)
> > > {
> > > WARN_ON(oom_killer_disabled);
> > > /* OOM killer might race with memcg OOM */
> > > tsk = find_lock_task_mm(tsk);
> > > if (!tsk)
> > > return;
> > > if (test_and_set_tsk_thread_flag(tsk, TIF_MEMDIE)) {
> > > task_unlock(tsk);
> > > return;
> > > }
> > > task_unlock(tsk);
> > > atomic_inc(&tsk->signal->oom_victims);
> > > /*
> > > * Make sure that the task is woken up from uninterruptible sleep
> > > * if it is frozen because OOM killer wouldn't be able to free
> > > * any memory and livelock. freezing_slow_path will tell the freezer
> > > * that TIF_MEMDIE tasks should be ignored.
> > > */
> > > __thaw_task(tsk);
> > > atomic_inc(&oom_victims);
> > > if (can_use_oom_reaper)
> > > wake_oom_reaper(tsk);
> > > }
> > >
> > > and move mark_oom_victim() by normal path to after task_unlock(victim).
> > >
> > > do_send_sig_info(SIGKILL, SEND_SIG_FORCED, victim, true);
> > > - mark_oom_victim(victim);
> > >
> > > - if (can_oom_reap)
> > > - wake_oom_reaper(victim);
> > > + wake_oom_reaper(victim, can_oom_reap);
> >
> > I do not like this because then we would have to check the reapability
> > from inside the oom_reaper again.
>
> I didn't understand why you think so. But strictly speaking, can_oom_reap calculation
> in oom_kill_process() is always racy, and [PATCH 10/10] is not safe.
>
> CPU0 (memory allocating task) CPU1 (kthread) CPU2 (OOM victim)
>
> Calls use_mm(victim->mm).
> Starts some worker.
> Enters out_of_memory().
> Enters oom_kill_process().
> Finishes some worker.
> Calls rcu_read_lock().
> Sets can_oom_reap = false due to process_shares_mm() && !same_thread_group() && (p->flags & PF_KTHREAD).
> Calls unuse_mm(victim->mm).
> Continues scanning other processes.
> Calls mmput(victim->mm).
> Sends SIGKILL to victim.
> Calls rcu_read_unlock().
> Leaves oom_kill_process().
> Calls do_exit().
> Leaves out_of_memory().
> Sets victim->mm = NULL from exit_mm().
> Calls mmput() from exit_mm().
> __mmput() is called because victim was the last user.
> Enters out_of_memory().
> oom_scan_process_thread() returns OOM_SCAN_ABORT.
> Leaves out_of_memory().
> __mmput() stalls but the oom_reaper is not called.
>
> For correctness, can_oom_reap needs to be calculated inside the oom_reaper.
Why it would be any less racy than the above? It doesn't employ any
serialization with use_mm users nor it serialize with the exit path.
The timing would get different but not in the way to talk about
correctness.
> > But let me ask again. Does this really matter so much just because of
> > nommu where we can fall in different traps? Can we simply focus on mmu
> > (aka vast majority of cases) make it work reliably and see what we can
> > do with nommu later?
>
> To me, timeout based one is sufficient for handling any traps that hit
> nommu kernels after the OOM killer is invoked.
>
> Anyway, I don't like this series because this series ignores
> theoretical cases.
I am pretty sure you would end up in the land of surprises and new
classes of races with any timeout based solutions as well. But we've
been through that discussion already.
> I can't make progress as long as you repeat "does it really matter/occur".
> Please go ahead without Reviewed-by: or Acked-by: from me.
Fair enough. I appreciate your review which has caught many real bugs
and subtle issues!
I will repost the series on Monday.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web