[PATCH 0/2] TIF_MEMDIE usage fixlet

[PATCH 0/2] TIF_MEMDIE usage fixlet

[Michal Hocko]

Hi,
while I was looking to move TIF_MEMDIE out of the thread info for other purpose
I have noticed these two usages which are not correct. I do not think any of them
warrant a stable backport because it is highly unlikely they would ever hit but
it is better to have them fixed.

I would route them via Andrew unless anybody has anything against.

[PATCH 0/2] TIF_MEMDIE usage fixlet

[Michal Hocko]

Hi,
while I was looking to move TIF_MEMDIE out of the thread info for other purpose
I have noticed these two usages which are not correct. I do not think any of them
warrant a stable backport because it is highly unlikely they would ever hit but
it is better to have them fixed.

I would route them via Andrew unless anybody has anything against.

[PATCH 0/2] TIF_MEMDIE usage fixlet

[Michal Hocko]

Hi,
while I was looking to move TIF_MEMDIE out of the thread info for other purpose
I have noticed these two usages which are not correct. I do not think any of them
warrant a stable backport because it is highly unlikely they would ever hit but
it is better to have them fixed.

I would route them via Andrew unless anybody has anything against.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/2] freezer, oom: check TIF_MEMDIE on the correct task

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

freezing_slow_path is checking TIF_MEMDIE to skip OOM killed
tasks. It is, however, checking the flag on the current task rather than
the given one. This is really confusing because freezing() can be called
also on !current tasks. It would end up working correctly for its main
purpose because __refrigerator will be always called on the current task
so the oom victim will never get frozen. But it could lead to surprising
results when a task which is freezing a cgroup got oom killed because
only part of the cgroup would get frozen. This is highly unlikely but
worth fixing as the resulting code would be more clear anyway.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 kernel/freezer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index a8900a3bc27a..6f56a9e219fa 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -42,7 +42,7 @@ bool freezing_slow_path(struct task_struct *p)
 	if (p->flags & (PF_NOFREEZE | PF_SUSPEND_TASK))
 		return false;
 
-	if (test_thread_flag(TIF_MEMDIE))
+	if (test_tsk_thread_flag(p, TIF_MEMDIE))
 		return false;
 
 	if (pm_nosig_freezing || cgroup_freezing(p))
-- 
2.8.1

[PATCH 1/2] freezer, oom: check TIF_MEMDIE on the correct task

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

freezing_slow_path is checking TIF_MEMDIE to skip OOM killed
tasks. It is, however, checking the flag on the current task rather than
the given one. This is really confusing because freezing() can be called
also on !current tasks. It would end up working correctly for its main
purpose because __refrigerator will be always called on the current task
so the oom victim will never get frozen. But it could lead to surprising
results when a task which is freezing a cgroup got oom killed because
only part of the cgroup would get frozen. This is highly unlikely but
worth fixing as the resulting code would be more clear anyway.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 kernel/freezer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index a8900a3bc27a..6f56a9e219fa 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -42,7 +42,7 @@ bool freezing_slow_path(struct task_struct *p)
 	if (p->flags & (PF_NOFREEZE | PF_SUSPEND_TASK))
 		return false;
 
-	if (test_thread_flag(TIF_MEMDIE))
+	if (test_tsk_thread_flag(p, TIF_MEMDIE))
 		return false;
 
 	if (pm_nosig_freezing || cgroup_freezing(p))
-- 
2.8.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] cpuset, mm: fix TIF_MEMDIE check in cpuset_change_task_nodemask

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

c0ff7453bb5c ("cpuset,mm: fix no node to alloc memory when changing
cpuset's mems") has added TIF_MEMDIE and PF_EXITING check but it is
checking the flag on the current task rather than the given one.
This doesn't make much sense and it is actually wrong. If the current
task which updates the nodemask of a cpuset got killed by the OOM killer
then a part of the cpuset cgroup processes would have incompatible
nodemask which is surprising to say the least.

The comment suggests the intention was to skip oom victim or an exiting
task so we should be checking the given task. But even then it would be
layering violation becuase it is the memory allocator to interpret the
TIF_MEMDIE meaning. Simply drop both checks. All tasks in the cpuset
should simply follow the same mask.

Cc: Miao Xie <miaox@cn.fujitsu.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 kernel/cpuset.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 73e93e53884d..c7fd2778ed50 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1034,15 +1034,6 @@ static void cpuset_change_task_nodemask(struct task_struct *tsk,
 {
 	bool need_loop;
 
-	/*
-	 * Allow tasks that have access to memory reserves because they have
-	 * been OOM killed to get memory anywhere.
-	 */
-	if (unlikely(test_thread_flag(TIF_MEMDIE)))
-		return;
-	if (current->flags & PF_EXITING) /* Let dying task have memory */
-		return;
-
 	task_lock(tsk);
 	/*
 	 * Determine if a loop is necessary if another thread is doing
-- 
2.8.1

[PATCH 2/2] cpuset, mm: fix TIF_MEMDIE check in cpuset_change_task_nodemask

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

c0ff7453bb5c ("cpuset,mm: fix no node to alloc memory when changing
cpuset's mems") has added TIF_MEMDIE and PF_EXITING check but it is
checking the flag on the current task rather than the given one.
This doesn't make much sense and it is actually wrong. If the current
task which updates the nodemask of a cpuset got killed by the OOM killer
then a part of the cpuset cgroup processes would have incompatible
nodemask which is surprising to say the least.

The comment suggests the intention was to skip oom victim or an exiting
task so we should be checking the given task. But even then it would be
layering violation becuase it is the memory allocator to interpret the
TIF_MEMDIE meaning. Simply drop both checks. All tasks in the cpuset
should simply follow the same mask.

Cc: Miao Xie <miaox@cn.fujitsu.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 kernel/cpuset.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 73e93e53884d..c7fd2778ed50 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1034,15 +1034,6 @@ static void cpuset_change_task_nodemask(struct task_struct *tsk,
 {
 	bool need_loop;
 
-	/*
-	 * Allow tasks that have access to memory reserves because they have
-	 * been OOM killed to get memory anywhere.
-	 */
-	if (unlikely(test_thread_flag(TIF_MEMDIE)))
-		return;
-	if (current->flags & PF_EXITING) /* Let dying task have memory */
-		return;
-
 	task_lock(tsk);
 	/*
 	 * Determine if a loop is necessary if another thread is doing
-- 
2.8.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] freezer, oom: check TIF_MEMDIE on the correct task

[Rafael J. Wysocki]

On Mon, Jun 27, 2016 at 2:15 PM, Michal Hocko <mhocko@kernel.org> wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> freezing_slow_path is checking TIF_MEMDIE to skip OOM killed
> tasks. It is, however, checking the flag on the current task rather than
> the given one. This is really confusing because freezing() can be called
> also on !current tasks. It would end up working correctly for its main
> purpose because __refrigerator will be always called on the current task
> so the oom victim will never get frozen. But it could lead to surprising
> results when a task which is freezing a cgroup got oom killed because
> only part of the cgroup would get frozen. This is highly unlikely but
> worth fixing as the resulting code would be more clear anyway.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Looks reasonable to me, so ACK.

> ---
>  kernel/freezer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index a8900a3bc27a..6f56a9e219fa 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -42,7 +42,7 @@ bool freezing_slow_path(struct task_struct *p)
>         if (p->flags & (PF_NOFREEZE | PF_SUSPEND_TASK))
>                 return false;
>
> -       if (test_thread_flag(TIF_MEMDIE))
> +       if (test_tsk_thread_flag(p, TIF_MEMDIE))
>                 return false;
>
>         if (pm_nosig_freezing || cgroup_freezing(p))
> --

Re: [PATCH 1/2] freezer, oom: check TIF_MEMDIE on the correct task

[Rafael J. Wysocki]

On Mon, Jun 27, 2016 at 2:15 PM, Michal Hocko <mhocko@kernel.org> wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> freezing_slow_path is checking TIF_MEMDIE to skip OOM killed
> tasks. It is, however, checking the flag on the current task rather than
> the given one. This is really confusing because freezing() can be called
> also on !current tasks. It would end up working correctly for its main
> purpose because __refrigerator will be always called on the current task
> so the oom victim will never get frozen. But it could lead to surprising
> results when a task which is freezing a cgroup got oom killed because
> only part of the cgroup would get frozen. This is highly unlikely but
> worth fixing as the resulting code would be more clear anyway.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Looks reasonable to me, so ACK.

> ---
>  kernel/freezer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index a8900a3bc27a..6f56a9e219fa 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -42,7 +42,7 @@ bool freezing_slow_path(struct task_struct *p)
>         if (p->flags & (PF_NOFREEZE | PF_SUSPEND_TASK))
>                 return false;
>
> -       if (test_thread_flag(TIF_MEMDIE))
> +       if (test_tsk_thread_flag(p, TIF_MEMDIE))
>                 return false;
>
>         if (pm_nosig_freezing || cgroup_freezing(p))
> --

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>