[PATCH] mm: memcontrol: drop unnecessary task_will_free_mem() check.

[PATCH] mm: memcontrol: drop unnecessary task_will_free_mem() check.

[Tetsuo Handa]

Since mem_cgroup_out_of_memory() is called by
mem_cgroup_oom_synchronize(true) via pagefault_out_of_memory() via
page fault, and possible allocations between setting PF_EXITING and
calling exit_mm() are tty_audit_exit() and taskstats_exit() which will
not trigger page fault, task_will_free_mem(current) in
mem_cgroup_out_of_memory() is never true.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 mm/memcontrol.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ae8b81c..701bef1 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1254,11 +1254,11 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	mutex_lock(&oom_lock);
 
 	/*
-	 * If current has a pending SIGKILL or is exiting, then automatically
-	 * select it.  The goal is to allow it to allocate so that it may
-	 * quickly exit and free its memory.
+	 * If current has a pending SIGKILL, then automatically select it.
+	 * The goal is to allow it to allocate so that it may quickly exit
+	 * and free its memory.
 	 */
-	if (fatal_signal_pending(current) || task_will_free_mem(current)) {
+	if (fatal_signal_pending(current)) {
 		mark_oom_victim(current);
 		goto unlock;
 	}
-- 
1.8.3.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] mm: memcontrol: drop unnecessary task_will_free_mem() check.

[Johannes Weiner]

On Wed, Mar 09, 2016 at 12:15:10AM +0900, Tetsuo Handa wrote:
> Since mem_cgroup_out_of_memory() is called by
> mem_cgroup_oom_synchronize(true) via pagefault_out_of_memory() via
> page fault, and possible allocations between setting PF_EXITING and
> calling exit_mm() are tty_audit_exit() and taskstats_exit() which will
> not trigger page fault, task_will_free_mem(current) in
> mem_cgroup_out_of_memory() is never true.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

This opens us up to subtle bugs when somebody later changes the order
and adds new possible allocation sites between the sequence points you
describe above, or maybe adds other mem_cgroup_out_of_memory() callers.

It looks like a simplification, but it actually complicates things.

--
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] mm: memcontrol: drop unnecessary task_will_free_mem() check.

[Tetsuo Handa]

Johannes Weiner wrote:
> On Wed, Mar 09, 2016 at 12:15:10AM +0900, Tetsuo Handa wrote:
> > Since mem_cgroup_out_of_memory() is called by
> > mem_cgroup_oom_synchronize(true) via pagefault_out_of_memory() via
> > page fault, and possible allocations between setting PF_EXITING and
> > calling exit_mm() are tty_audit_exit() and taskstats_exit() which will
> > not trigger page fault, task_will_free_mem(current) in
> > mem_cgroup_out_of_memory() is never true.
> > 
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> 
> This opens us up to subtle bugs when somebody later changes the order
> and adds new possible allocation sites between the sequence points you
> describe above, or maybe adds other mem_cgroup_out_of_memory() callers.
> 
> It looks like a simplification, but it actually complicates things.
> 

If currently not needed, it should be removed. This is for a clarification.

Also, what is the reason we do not need below change?
I think there is a small race window because oom_killer_disabled needs to be
checked after oom_killer_disable() held oom_lock. Is it because all userspace
processes except current are frozen before oom_killer_disable() is called and
not-yet frozen threads (i.e. kernel threads) never call mem_cgroup_out_of_memory() ?

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ae8b81c..521cd33 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1253,6 +1253,10 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
 
 	mutex_lock(&oom_lock);
 
+	/* Check if we raced with oom_killer_disable(). */
+	if (oom_killer_disabled)
+		goto unlock;
+
 	/*
 	 * If current has a pending SIGKILL or is exiting, then automatically
 	 * select it.  The goal is to allow it to allocate so that it may

--
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] mm: memcontrol: drop unnecessary task_will_free_mem() check.

[Michal Hocko]

On Wed 09-03-16 08:05:11, Tetsuo Handa wrote:
[...]
> Also, what is the reason we do not need below change?
> I think there is a small race window because oom_killer_disabled needs to be
> checked after oom_killer_disable() held oom_lock. Is it because all userspace
> processes except current are frozen before oom_killer_disable() is called and
> not-yet frozen threads (i.e. kernel threads) never call mem_cgroup_out_of_memory() ?

Please refer to c32b3cbe0d06 ("oom, PM: make OOM detection in the
freezer path raceless"). It should explain this.
-- 
Michal Hocko
SUSE Labs

--
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] mm: memcontrol: drop unnecessary task_will_free_mem() check.

[Michal Hocko]

On Wed 09-03-16 11:08:14, Michal Hocko wrote:
> On Wed 09-03-16 08:05:11, Tetsuo Handa wrote:
> [...]
> > Also, what is the reason we do not need below change?
> > I think there is a small race window because oom_killer_disabled needs to be
> > checked after oom_killer_disable() held oom_lock. Is it because all userspace
> > processes except current are frozen before oom_killer_disable() is called and
> > not-yet frozen threads (i.e. kernel threads) never call mem_cgroup_out_of_memory() ?
> 
> Please refer to c32b3cbe0d06 ("oom, PM: make OOM detection in the
> freezer path raceless"). It should explain this.

And for the N+1 time, do not conflate different topics into a single
email thread. This is really annoying.
-- 
Michal Hocko
SUSE Labs

--
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] mm: memcontrol: drop unnecessary task_will_free_mem() check.

[Michal Hocko]

On Wed 09-03-16 00:15:10, Tetsuo Handa wrote:
> Since mem_cgroup_out_of_memory() is called by
> mem_cgroup_oom_synchronize(true) via pagefault_out_of_memory() via
> page fault, and possible allocations between setting PF_EXITING and
> calling exit_mm() are tty_audit_exit() and taskstats_exit() which will
> not trigger page fault, task_will_free_mem(current) in
> mem_cgroup_out_of_memory() is never true.

What about exit_robust_list called from mm_release?

Anyway I guess we can indeed remove the check because try_charge will
bypass the charge if we are exiting so we shouldn't even reach this path
with PF_EXITING. But I haven't double checked. The above changelog seems
to be incorrect, though.

> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  mm/memcontrol.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ae8b81c..701bef1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1254,11 +1254,11 @@ static void mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask,
> 	mutex_lock(&oom_lock);
>  
>  	/*
> -	 * If current has a pending SIGKILL or is exiting, then automatically
> -	 * select it.  The goal is to allow it to allocate so that it may
> -	 * quickly exit and free its memory.
> +	 * If current has a pending SIGKILL, then automatically select it.
> +	 * The goal is to allow it to allocate so that it may quickly exit
> +	 * and free its memory.
>  	 */
> -	if (fatal_signal_pending(current) || task_will_free_mem(current)) {
> +	if (fatal_signal_pending(current)) {
>  		mark_oom_victim(current);
>  		goto unlock;
>  	}
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

--
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] mm: memcontrol: drop unnecessary task_will_free_mem() check.

[Tetsuo Handa]

Michal Hocko wrote:
> On Wed 09-03-16 00:15:10, Tetsuo Handa wrote:
> > Since mem_cgroup_out_of_memory() is called by
> > mem_cgroup_oom_synchronize(true) via pagefault_out_of_memory() via
> > page fault, and possible allocations between setting PF_EXITING and
> > calling exit_mm() are tty_audit_exit() and taskstats_exit() which will
> > not trigger page fault, task_will_free_mem(current) in
> > mem_cgroup_out_of_memory() is never true.
> 
> What about exit_robust_list called from mm_release?
> 
> Anyway I guess we can indeed remove the check because try_charge will
> bypass the charge if we are exiting so we shouldn't even reach this path
> with PF_EXITING. But I haven't double checked. The above changelog seems
> to be incorrect, though.
> 

Indeed. do_exit()->exit_mm()->mm_release()->exit_robust_list()->get_user()
can trigger page fault.

--
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>