* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree
@ 2014-10-16 20:39 Oleg Nesterov
2014-10-16 20:53 ` Cong Wang
2014-10-17 6:59 ` Michal Hocko
0 siblings, 2 replies; 18+ messages in thread
From: Oleg Nesterov @ 2014-10-16 20:39 UTC (permalink / raw)
To: Cong Wang, Michal Hocko, David Rientjes, Rafael J. Wysocki,
Tejun Heo, Andrew Morton
Cc: linux-kernel
> Fix the issue by checking for TIF_MEMDIE thread flag and get away from the
> fridge if it is set. oom_scan_process_thread doesn't have to check for
> the frozen task anymore because do_send_sig_info will wake up the thread
> and TIF_MEMDIE is already set by that time.
I must have missed something... but __refrigerator() sleeps in
TASK_UNINTERRUPTIBLE and do_send_sig_info() won't wake it up?
Oleg.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-16 20:39 + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree Oleg Nesterov @ 2014-10-16 20:53 ` Cong Wang 2014-10-16 21:11 ` Oleg Nesterov 2014-10-17 6:59 ` Michal Hocko 1 sibling, 1 reply; 18+ messages in thread From: Cong Wang @ 2014-10-16 20:53 UTC (permalink / raw) To: Oleg Nesterov Cc: Michal Hocko, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, LKML On Thu, Oct 16, 2014 at 1:39 PM, Oleg Nesterov <oleg@redhat.com> wrote: >> Fix the issue by checking for TIF_MEMDIE thread flag and get away from the >> fridge if it is set. oom_scan_process_thread doesn't have to check for >> the frozen task anymore because do_send_sig_info will wake up the thread >> and TIF_MEMDIE is already set by that time. > > I must have missed something... but __refrigerator() sleeps in > TASK_UNINTERRUPTIBLE and do_send_sig_info() won't wake it up? > This is exactly what we are trying to fix. Make sure you read the patch as well before reply? Otherwise, I must have missed your point. :) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-16 20:53 ` Cong Wang @ 2014-10-16 21:11 ` Oleg Nesterov 2014-10-16 21:19 ` Cong Wang 0 siblings, 1 reply; 18+ messages in thread From: Oleg Nesterov @ 2014-10-16 21:11 UTC (permalink / raw) To: Cong Wang Cc: Michal Hocko, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, LKML On 10/16, Cong Wang wrote: > > On Thu, Oct 16, 2014 at 1:39 PM, Oleg Nesterov <oleg@redhat.com> wrote: > >> Fix the issue by checking for TIF_MEMDIE thread flag and get away from the > >> fridge if it is set. oom_scan_process_thread doesn't have to check for > >> the frozen task anymore because do_send_sig_info will wake up the thread > >> and TIF_MEMDIE is already set by that time. > > > > I must have missed something... but __refrigerator() sleeps in > > TASK_UNINTERRUPTIBLE and do_send_sig_info() won't wake it up? > > > > This is exactly what we are trying to fix. Make sure you read the patch > as well before reply? I did read the patch, but I can't understand it. I am sorry about that, and I am asking for your help. I agree that oom_scan_process_thread()->__thaw_task() doesn't really help. But also I can't understand why this patch helps. The changelog says: do_send_sig_info will wake up the thread why? Oleg. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-16 21:11 ` Oleg Nesterov @ 2014-10-16 21:19 ` Cong Wang 2014-10-16 21:35 ` Oleg Nesterov 0 siblings, 1 reply; 18+ messages in thread From: Cong Wang @ 2014-10-16 21:19 UTC (permalink / raw) To: Oleg Nesterov Cc: Michal Hocko, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, LKML On Thu, Oct 16, 2014 at 2:11 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > But also I can't understand why this patch helps. The changelog says: > > do_send_sig_info will wake up the thread > > why? > This is a question for Michal who rewrites my changelog: http://marc.info/?l=linux-kernel&m=140986986423092&w=2 :) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-16 21:19 ` Cong Wang @ 2014-10-16 21:35 ` Oleg Nesterov 2014-10-16 21:52 ` Cong Wang 0 siblings, 1 reply; 18+ messages in thread From: Oleg Nesterov @ 2014-10-16 21:35 UTC (permalink / raw) To: Cong Wang Cc: Michal Hocko, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, LKML On 10/16, Cong Wang wrote: > > On Thu, Oct 16, 2014 at 2:11 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > > > But also I can't understand why this patch helps. The changelog says: > > > > do_send_sig_info will wake up the thread > > > > why? > > > > This is a question for Michal who rewrites my changelog: > > http://marc.info/?l=linux-kernel&m=140986986423092&w=2 > > :) OK, I hope Michal can answer my question if you do not want to do this ;) So far I think this patch is not right. If a task B is already frozen, it sleeps in D state. If OOM selects B as a victim after that, it won't be woken by SIGKILL, thus it obviously can't call should_thaw_current() and notice TIF_MEMDIE. Btw, I also do not understand the cgroup_freezing() check in should_thaw_current(), but this is another story. Oleg. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-16 21:35 ` Oleg Nesterov @ 2014-10-16 21:52 ` Cong Wang 2014-10-16 22:22 ` Oleg Nesterov 0 siblings, 1 reply; 18+ messages in thread From: Cong Wang @ 2014-10-16 21:52 UTC (permalink / raw) To: Oleg Nesterov Cc: Michal Hocko, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, LKML On Thu, Oct 16, 2014 at 2:35 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > If a task B is already frozen, it sleeps in D state. > > If OOM selects B as a victim after that, it won't be woken by > SIGKILL, thus it obviously can't call should_thaw_current() and > notice TIF_MEMDIE. I see your point now, it would be more clear if you can just quote the patch instead of changelog. So are you saying the loop in __refrigerator() is useless? Since it will always stay in asleep after schedule()? > > Btw, I also do not understand the cgroup_freezing() check in > should_thaw_current(), but this is another story. > I hate to repeat the previous discussion. Maybe you can just follow the link I gave to you? :) Thanks. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-16 21:52 ` Cong Wang @ 2014-10-16 22:22 ` Oleg Nesterov 2014-10-17 2:33 ` Cong Wang 0 siblings, 1 reply; 18+ messages in thread From: Oleg Nesterov @ 2014-10-16 22:22 UTC (permalink / raw) To: Cong Wang Cc: Michal Hocko, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, LKML On 10/16, Cong Wang wrote: > > On Thu, Oct 16, 2014 at 2:35 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > > > If a task B is already frozen, it sleeps in D state. > > > > If OOM selects B as a victim after that, it won't be woken by > > SIGKILL, thus it obviously can't call should_thaw_current() and > > notice TIF_MEMDIE. > > I see your point now, it would be more clear if you can just quote > the patch instead of changelog. > > So are you saying the loop in __refrigerator() is useless? No. > Since > it will always stay in asleep after schedule()? Not always. But it will stay asleep in this particular case. > > Btw, I also do not understand the cgroup_freezing() check in > > should_thaw_current(), but this is another story. > > I hate to repeat the previous discussion. Maybe you can just follow > the link I gave to you? :) May be, but this thread is huge. Will try tomorrow to read it tomorrow, but you know, I hope that someone else from cc list can copy-and-paste the relevant part of this discussion, or give me the link to some specific email. To me the comment should be more clear in any case, but perhaps it is just me who can't understand it immediately. Oleg. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-16 22:22 ` Oleg Nesterov @ 2014-10-17 2:33 ` Cong Wang 2014-10-17 7:46 ` [PATCH -v2] freezer: check OOM kill while being frozen Michal Hocko 2014-10-17 15:24 ` + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree Oleg Nesterov 0 siblings, 2 replies; 18+ messages in thread From: Cong Wang @ 2014-10-17 2:33 UTC (permalink / raw) To: Oleg Nesterov Cc: Michal Hocko, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, LKML On Thu, Oct 16, 2014 at 3:22 PM, Oleg Nesterov <oleg@redhat.com> wrote: > On 10/16, Cong Wang wrote: >> >> On Thu, Oct 16, 2014 at 2:35 PM, Oleg Nesterov <oleg@redhat.com> wrote: >> > >> > If a task B is already frozen, it sleeps in D state. >> > >> > If OOM selects B as a victim after that, it won't be woken by >> > SIGKILL, thus it obviously can't call should_thaw_current() and >> > notice TIF_MEMDIE. >> >> I see your point now, it would be more clear if you can just quote >> the patch instead of changelog. >> >> So are you saying the loop in __refrigerator() is useless? > > No. > >> Since >> it will always stay in asleep after schedule()? > > Not always. But it will stay asleep in this particular case. Hmm, so we still need to wake it up in oom killer: if (test_tsk_thread_flag(task, TIF_MEMDIE)) { if (unlikely(frozen(task))) wake_up_state(task, TASK_UNINTERRUPTIBLE); I will update the patch if Michal doesn't. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH -v2] freezer: check OOM kill while being frozen 2014-10-17 2:33 ` Cong Wang @ 2014-10-17 7:46 ` Michal Hocko 2014-10-17 16:10 ` Oleg Nesterov 2014-10-17 15:24 ` + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree Oleg Nesterov 1 sibling, 1 reply; 18+ messages in thread From: Michal Hocko @ 2014-10-17 7:46 UTC (permalink / raw) To: Cong Wang, Andrew Morton Cc: Oleg Nesterov, David Rientjes, Rafael J. Wysocki, Tejun Heo, LKML On Thu 16-10-14 19:33:39, Cong Wang wrote: > On Thu, Oct 16, 2014 at 3:22 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 10/16, Cong Wang wrote: > >> > >> On Thu, Oct 16, 2014 at 2:35 PM, Oleg Nesterov <oleg@redhat.com> wrote: > >> > > >> > If a task B is already frozen, it sleeps in D state. > >> > > >> > If OOM selects B as a victim after that, it won't be woken by > >> > SIGKILL, thus it obviously can't call should_thaw_current() and > >> > notice TIF_MEMDIE. > >> > >> I see your point now, it would be more clear if you can just quote > >> the patch instead of changelog. > >> > >> So are you saying the loop in __refrigerator() is useless? > > > > No. > > > >> Since > >> it will always stay in asleep after schedule()? > > > > Not always. But it will stay asleep in this particular case. > > Hmm, so we still need to wake it up in oom killer: > > if (test_tsk_thread_flag(task, TIF_MEMDIE)) { > if (unlikely(frozen(task))) > wake_up_state(task, TASK_UNINTERRUPTIBLE); > > I will update the patch if Michal doesn't. I think we should rather get back to __thaw_task here. Andrew could you replace the previous version by this one, please? Again I am sorry I haven't caught that before. --- >From 830324c8be8b499e1d18a73076cf4d126c4ac486 Mon Sep 17 00:00:00 2001 From: Cong Wang <xiyou.wangcong@gmail.com> Date: Thu, 4 Sep 2014 15:30:41 -0700 Subject: [PATCH -v2] 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 and get away from the fridge if it is set. Changes since v1 - 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 | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/kernel/freezer.c b/kernel/freezer.c index aa6a8aadb911..77ad6794b610 100644 --- a/kernel/freezer.c +++ b/kernel/freezer.c @@ -45,13 +45,28 @@ bool freezing_slow_path(struct task_struct *p) if (pm_nosig_freezing || cgroup_freezing(p)) return true; - if (pm_freezing && !(p->flags & PF_KTHREAD)) + if (!(p->flags & PF_KTHREAD)) return true; return false; } EXPORT_SYMBOL(freezing_slow_path); +static bool should_thaw_current(bool check_kthr_stop) +{ + if (!freezing(current)) + return true; + + if (check_kthr_stop && kthread_should_stop()) + return true; + + /* It might not be safe to check TIF_MEMDIE for pm freeze. */ + if (cgroup_freezing(current) && test_thread_flag(TIF_MEMDIE)) + return true; + + return false; +} + /* Refrigerator is place where frozen processes are stored :-). */ bool __refrigerator(bool check_kthr_stop) { @@ -67,8 +82,7 @@ bool __refrigerator(bool check_kthr_stop) spin_lock_irq(&freezer_lock); current->flags |= PF_FROZEN; - if (!freezing(current) || - (check_kthr_stop && kthread_should_stop())) + if (should_thaw_current(check_kthr_stop)) current->flags &= ~PF_FROZEN; spin_unlock_irq(&freezer_lock); -- 2.1.1 -- Michal Hocko SUSE Labs ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH -v2] freezer: check OOM kill while being frozen 2014-10-17 7:46 ` [PATCH -v2] freezer: check OOM kill while being frozen Michal Hocko @ 2014-10-17 16:10 ` Oleg Nesterov 2014-10-17 16:20 ` Michal Hocko 2014-10-20 15:17 ` Michal Hocko 0 siblings, 2 replies; 18+ messages in thread From: Oleg Nesterov @ 2014-10-17 16:10 UTC (permalink / raw) To: Michal Hocko Cc: Cong Wang, Andrew Morton, David Rientjes, Rafael J. Wysocki, Tejun Heo, LKML On 10/17, Michal Hocko wrote: > > I think we should rather get back to __thaw_task here. Yes, agreed. > Andrew could you replace the previous version by this one, please? Yes, that patch should be dropped... And can't resist... please look at http://marc.info/?l=linux-kernel&m=138427535430827 ;) > --- a/kernel/freezer.c > +++ b/kernel/freezer.c > @@ -45,13 +45,28 @@ bool freezing_slow_path(struct task_struct *p) > if (pm_nosig_freezing || cgroup_freezing(p)) > return true; > > - if (pm_freezing && !(p->flags & PF_KTHREAD)) > + if (!(p->flags & PF_KTHREAD)) Why? Doesn't this mean that try_to_freeze() can race with thaw_processes() and then this task can be frozen for no reazon? > +static bool should_thaw_current(bool check_kthr_stop) > +{ > + if (!freezing(current)) > + return true; > + > + if (check_kthr_stop && kthread_should_stop()) > + return true; > + > + /* It might not be safe to check TIF_MEMDIE for pm freeze. */ > + if (cgroup_freezing(current) && test_thread_flag(TIF_MEMDIE)) I still think that the comment should tell more to explain why this is not safe. 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? And I think that this TIF_MEMDIE should go into freezing_slow_path(), so we do not even need should_thaw_current(). 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. Oleg. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH -v2] freezer: check OOM kill while being frozen 2014-10-17 16:10 ` Oleg Nesterov @ 2014-10-17 16:20 ` Michal Hocko 2014-10-20 15:17 ` Michal Hocko 1 sibling, 0 replies; 18+ messages in thread From: Michal Hocko @ 2014-10-17 16:20 UTC (permalink / raw) To: Oleg Nesterov, Andrew Morton Cc: Cong Wang, David Rientjes, Rafael J. Wysocki, Tejun Heo, LKML On Fri 17-10-14 18:10:21, Oleg Nesterov wrote: > On 10/17, Michal Hocko wrote: > > > > I think we should rather get back to __thaw_task here. > > Yes, agreed. > > > Andrew could you replace the previous version by this one, please? > > Yes, that patch should be dropped... Andrew, drop all three patches. I will think about things raised here by Oleg and come back sometimes next week. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH -v2] freezer: check OOM kill while being frozen 2014-10-17 16:10 ` Oleg Nesterov 2014-10-17 16:20 ` Michal Hocko @ 2014-10-20 15:17 ` Michal Hocko 2014-10-20 17:40 ` Oleg Nesterov 1 sibling, 1 reply; 18+ messages in thread From: Michal Hocko @ 2014-10-20 15:17 UTC (permalink / raw) To: Cong Wang, Oleg Nesterov Cc: Andrew Morton, David Rientjes, Rafael J. Wysocki, Tejun Heo, LKML On Fri 17-10-14 18:10:21, Oleg Nesterov wrote: > On 10/17, Michal Hocko wrote: > > > > I think we should rather get back to __thaw_task here. > > Yes, agreed. > > > Andrew could you replace the previous version by this one, please? > > Yes, that patch should be dropped... > > > And can't resist... please look at > http://marc.info/?l=linux-kernel&m=138427535430827 ;) > > > --- a/kernel/freezer.c > > +++ b/kernel/freezer.c > > @@ -45,13 +45,28 @@ bool freezing_slow_path(struct task_struct *p) > > if (pm_nosig_freezing || cgroup_freezing(p)) > > return true; > > > > - if (pm_freezing && !(p->flags & PF_KTHREAD)) > > + if (!(p->flags & PF_KTHREAD)) > > Why? Doesn't this mean that try_to_freeze() can race with thaw_processes() > and then this task can be frozen for no reazon? Hmm, this wasn't there in the v4 of the original patch from Cong. http://marc.info/?l=linux-kernel&m=140986986423092. I cannot find any reference to this hunk and it might be misapplied patch when I took the patch from the list. I do not see any reason for this change. > > +static bool should_thaw_current(bool check_kthr_stop) > > +{ > > + if (!freezing(current)) > > + return true; > > + > > + if (check_kthr_stop && kthread_should_stop()) > > + return true; > > + > > + /* It might not be safe to check TIF_MEMDIE for pm freeze. */ > > + if (cgroup_freezing(current) && test_thread_flag(TIF_MEMDIE)) > > I still think that the comment should tell more to explain why this > is not safe. I have suggested removing this check here: http://marc.info/?l=linux-kernel&m=141078015130905 and it shouldn't be really here. I must have screwed something up when rebasing the series... Sorry about that! Anyway the leader of the series should describe what was unsafe and how it got fixed: http://marc.info/?l=linux-mm&m=141277728508500&w=2 > 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? > 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. > 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. Anyway, updated patch is below. I have still kept Cong as the original author but please let me know if this is not OK after considerable changes in the patch. Does it make more sense to you now, Oleg? --- >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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH -v2] freezer: check OOM kill while being frozen 2014-10-20 15:17 ` Michal Hocko @ 2014-10-20 17:40 ` Oleg Nesterov 0 siblings, 0 replies; 18+ messages in thread From: Oleg Nesterov @ 2014-10-20 17:40 UTC (permalink / raw) To: Michal Hocko Cc: Cong Wang, Andrew Morton, David Rientjes, Rafael J. Wysocki, Tejun Heo, LKML 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-17 2:33 ` Cong Wang 2014-10-17 7:46 ` [PATCH -v2] freezer: check OOM kill while being frozen Michal Hocko @ 2014-10-17 15:24 ` Oleg Nesterov 2014-10-17 16:07 ` Michal Hocko 1 sibling, 1 reply; 18+ messages in thread From: Oleg Nesterov @ 2014-10-17 15:24 UTC (permalink / raw) To: Cong Wang Cc: Michal Hocko, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, LKML On 10/16, Cong Wang wrote: > > On Thu, Oct 16, 2014 at 3:22 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 10/16, Cong Wang wrote: > >> > >> it will always stay in asleep after schedule()? > > > > Not always. But it will stay asleep in this particular case. > > Hmm, so we still need to wake it up in oom killer: > > if (test_tsk_thread_flag(task, TIF_MEMDIE)) { > if (unlikely(frozen(task))) > wake_up_state(task, TASK_UNINTERRUPTIBLE); > > I will update the patch if Michal doesn't. I think it would be better to simply keep that __thaw_task() in oom_scan_process_thread(). Oleg. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-17 15:24 ` + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree Oleg Nesterov @ 2014-10-17 16:07 ` Michal Hocko 0 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2014-10-17 16:07 UTC (permalink / raw) To: Oleg Nesterov Cc: Cong Wang, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, LKML On Fri 17-10-14 17:24:29, Oleg Nesterov wrote: > On 10/16, Cong Wang wrote: > > > > On Thu, Oct 16, 2014 at 3:22 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > > On 10/16, Cong Wang wrote: > > >> > > >> it will always stay in asleep after schedule()? > > > > > > Not always. But it will stay asleep in this particular case. > > > > Hmm, so we still need to wake it up in oom killer: > > > > if (test_tsk_thread_flag(task, TIF_MEMDIE)) { > > if (unlikely(frozen(task))) > > wake_up_state(task, TASK_UNINTERRUPTIBLE); > > > > I will update the patch if Michal doesn't. > > I think it would be better to simply keep that __thaw_task() in > oom_scan_process_thread(). yeah, v2 of the patch (I guess you were on CC) does exactly this. Thanks! -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-16 20:39 + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree Oleg Nesterov 2014-10-16 20:53 ` Cong Wang @ 2014-10-17 6:59 ` Michal Hocko 2014-10-17 15:31 ` Oleg Nesterov 1 sibling, 1 reply; 18+ messages in thread From: Michal Hocko @ 2014-10-17 6:59 UTC (permalink / raw) To: Oleg Nesterov Cc: Cong Wang, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, linux-kernel On Thu 16-10-14 22:39:54, Oleg Nesterov wrote: > > Fix the issue by checking for TIF_MEMDIE thread flag and get away from the > > fridge if it is set. oom_scan_process_thread doesn't have to check for > > the frozen task anymore because do_send_sig_info will wake up the thread > > and TIF_MEMDIE is already set by that time. > > I must have missed something... but __refrigerator() sleeps in > TASK_UNINTERRUPTIBLE and do_send_sig_info() won't wake it up? Ouch, I have completely missed this part when reviewing Cong's original patch. I got confused by the retry loop. Anyway you are right and the patch as is currently doesn't work as intented. Can we simply make the task sleep interruptible? It retries and rechecks the freezing conditions after wakeup anyway. It is true this would be a user visible change because frozen tasks won't be in D state anymore (this would make a difference for cgroup freezing because nobody would see this in PM freezer). Does anybody depend on this? Another possible way would be reintroducing freezer check into OOM path and kick the task even when it is in UN state. Thanks! -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-17 6:59 ` Michal Hocko @ 2014-10-17 15:31 ` Oleg Nesterov 2014-10-17 16:06 ` Michal Hocko 0 siblings, 1 reply; 18+ messages in thread From: Oleg Nesterov @ 2014-10-17 15:31 UTC (permalink / raw) To: Michal Hocko Cc: Cong Wang, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, linux-kernel On 10/17, Michal Hocko wrote: > > Can we simply make the task sleep interruptible? No, this will turn __refrigerator() into the busy-wait loop if signal_pending() == T. (but I still think it makes sense to kill PF_FROZEN and introduce TASK_FROZEN state, however this is offtopic). Oleg. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree 2014-10-17 15:31 ` Oleg Nesterov @ 2014-10-17 16:06 ` Michal Hocko 0 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2014-10-17 16:06 UTC (permalink / raw) To: Oleg Nesterov Cc: Cong Wang, David Rientjes, Rafael J. Wysocki, Tejun Heo, Andrew Morton, linux-kernel On Fri 17-10-14 17:31:55, Oleg Nesterov wrote: > On 10/17, Michal Hocko wrote: > > > > Can we simply make the task sleep interruptible? > > No, this will turn __refrigerator() into the busy-wait loop if > signal_pending() == T. good point. > (but I still think it makes sense to kill PF_FROZEN and introduce > TASK_FROZEN state, however this is offtopic). > > Oleg. > -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2014-10-20 17:44 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-10-16 20:39 + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree Oleg Nesterov 2014-10-16 20:53 ` Cong Wang 2014-10-16 21:11 ` Oleg Nesterov 2014-10-16 21:19 ` Cong Wang 2014-10-16 21:35 ` Oleg Nesterov 2014-10-16 21:52 ` Cong Wang 2014-10-16 22:22 ` Oleg Nesterov 2014-10-17 2:33 ` Cong Wang 2014-10-17 7:46 ` [PATCH -v2] freezer: check OOM kill while being frozen Michal Hocko 2014-10-17 16:10 ` Oleg Nesterov 2014-10-17 16:20 ` Michal Hocko 2014-10-20 15:17 ` Michal Hocko 2014-10-20 17:40 ` Oleg Nesterov 2014-10-17 15:24 ` + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree Oleg Nesterov 2014-10-17 16:07 ` Michal Hocko 2014-10-17 6:59 ` Michal Hocko 2014-10-17 15:31 ` Oleg Nesterov 2014-10-17 16:06 ` Michal Hocko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).