Messages in this thread Patch in this message | | | Date | Mon, 20 Oct 2014 19:40:43 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH -v2] freezer: check OOM kill while being frozen |
| |
On 10/20, Michal Hocko wrote: > > On Fri 17-10-14 18:10:21, Oleg Nesterov wrote: > > > And if this is not safe, it is not clear how/why cgroup_freezing() can > > save us, both pm_freezing and CGROUP_FREEZING can be true? > > You mean that the pm_freezer would race with cgroup one?
Yes, so if we actually want this check we should probably check !pm_freezing or update the comment.
Nevermind, you removed this check and I agree. Even if we add it back for some reason, it can come in a separate patch with the detailed explanation.
> > And I think that this TIF_MEMDIE should go into freezing_slow_path(), > > so we do not even need should_thaw_current(). > > OK, it would make the patch simpler. On the other hand having the check > in the __refrigerator makes it easier to follow. freezing is called from > too many places. But I see your point, I guess. It really doesn't make > sense to go into fridge when it is clear that the task wouldn't get > frozen anyway. Some users even check the return value of freezing and do > different things in two paths. Those seem to be mostly kernel threads > but I haven't checked all the places. Anyway this should be irrelevant > to the OOM POV.
Yes, thanks.
> > This also looks more safe to me. Suppose that a task does > > > > while (try_to_freeze()) > > ; > > > > Yes, this is pointless but correct. And in fact I think this pattern > > is possible. If this task is killed by OOM, it will spin forever. > > I am really not sure what such a code would be supposed to do.
and I actually meant
while (freezing()) try_to_freeze();
yes, sure, this looks strange and pointless. But still correct. And you never know what some driver can do. This pattern can be hidden in a more complex code, say,
for (;;) { lock_something(); // we can't use wait_event_freezable() under the lock wait_event_interruptible(condition() || freezing()); // check this before signal_pending() to avoid the restart, // or we can't restart, or it was just written this way for // no reason. if (freezing()) { unlock_something(); try_to_freeze(); continue; } unlock_something();
if (signal_pending()) break;
... }
sure, most probably the code like this asks for cleanups, but it is easy to notice that it is actually wrong if try_to_freeze() could return with freezing() == T. But I agree, this issue is minor.
> Does it make more sense to you now, Oleg?
Thanks!
Acked-by: Oleg Nesterov <oleg@redhat.com>
> From 6e8b92e7133307e30afe35c6a0637cb58c0fc147 Mon Sep 17 00:00:00 2001 > From: Cong Wang <xiyou.wangcong@gmail.com> > Date: Mon, 20 Oct 2014 17:16:01 +0200 > Subject: [PATCH] freezer: check OOM kill while being frozen > > Since f660daac474c6f (oom: thaw threads if oom killed thread is frozen > before deferring) OOM killer relies on being able to thaw a frozen task > to handle OOM situation but a3201227f803 (freezer: make freezing() test > freeze conditions in effect instead of TIF_FREEZE) has reorganized the > code and stopped clearing freeze flag in __thaw_task. This means that > the target task only wakes up and goes into the fridge again because the > freezing condition hasn't changed for it. This reintroduces the bug > fixed by f660daac474c6f. > > Fix the issue by checking for TIF_MEMDIE thread flag in > freezing_slow_path and exclude the task from freezing completely. If a > task was already frozen it would get woken by __thaw_task from OOM killer > and get out of freezer after rechecking freezing(). > > Changes since v1 > - put TIF_MEMDIE check into freezing_slowpath rather than in __refrigerator > as per Oleg > - return __thaw_task into oom_scan_process_thread because > oom_kill_process will not wake task in the fridge because it is > sleeping uninterruptible > > [mhocko@suse.cz: rewrote the changelog] > Fixes: a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE) > Cc: stable@vger.kernel.org # 3.3+ > Cc: David Rientjes <rientjes@google.com> > Cc: Michal Hocko <mhocko@suse.cz> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Tejun Heo <tj@kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Acked-by: Michal Hocko <mhocko@suse.cz> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: Michal Hocko <mhocko@suse.cz> > --- > kernel/freezer.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/kernel/freezer.c b/kernel/freezer.c > index aa6a8aadb911..8f9279b9c6d7 100644 > --- a/kernel/freezer.c > +++ b/kernel/freezer.c > @@ -42,6 +42,9 @@ bool freezing_slow_path(struct task_struct *p) > if (p->flags & (PF_NOFREEZE | PF_SUSPEND_TASK)) > return false; > > + if (test_thread_flag(TIF_MEMDIE)) > + return false; > + > if (pm_nosig_freezing || cgroup_freezing(p)) > return true; > > -- > 2.1.1 > > -- > Michal Hocko > SUSE Labs
| |