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 <o...@redhat.com>
> From 6e8b92e7133307e30afe35c6a0637cb58c0fc147 Mon Sep 17 00:00:00 2001
> From: Cong Wang <xiyou.wangc...@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
>
> [mho...@suse.cz: rewrote the changelog]
> Fixes: a3201227f803 (freezer: make freezing() test freeze conditions in
> effect instead of TIF_FREEZE)
> Cc: sta...@vger.kernel.org # 3.3+
> Cc: David Rientjes <rient...@google.com>
> Cc: Michal Hocko <mho...@suse.cz>
> Cc: "Rafael J. Wysocki" <r...@rjwysocki.net>
> Cc: Tejun Heo <t...@kernel.org>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Acked-by: Michal Hocko <mho...@suse.cz>
> Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
> Signed-off-by: Michal Hocko <mho...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
