[PATCH] mm,oom: make oom_killer_disable() killable.

[PATCH] mm,oom: make oom_killer_disable() killable.

[Tetsuo Handa]

While oom_killer_disable() is called by freeze_processes() after all user
threads except the current thread are frozen, it is possible that kernel
threads invoke the OOM killer and sends SIGKILL to the current thread due
to sharing the thawed victim's memory. Therefore, checking for SIGKILL is
preferable than TIF_MEMDIE.

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

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 6ebc0351..914451a 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -613,15 +613,11 @@ void exit_oom_victim(struct task_struct *tsk)
 bool oom_killer_disable(void)
 {
 	/*
-	 * Make sure to not race with an ongoing OOM killer
-	 * and that the current is not the victim.
+	 * Make sure to not race with an ongoing OOM killer. Check that the
+	 * current is not killed (possibly due to sharing the victim's memory).
 	 */
-	mutex_lock(&oom_lock);
-	if (test_thread_flag(TIF_MEMDIE)) {
-		mutex_unlock(&oom_lock);
+	if (mutex_lock_killable(&oom_lock))
 		return false;
-	}
-
 	oom_killer_disabled = true;
 	mutex_unlock(&oom_lock);
 
-- 
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,oom: make oom_killer_disable() killable.

[Michal Hocko]

[CCing Andrew]

On Sun 24-01-16 00:47:20, Tetsuo Handa wrote:
> While oom_killer_disable() is called by freeze_processes() after all user
> threads except the current thread are frozen, it is possible that kernel
> threads invoke the OOM killer and sends SIGKILL to the current thread due
> to sharing the thawed victim's memory. Therefore, checking for SIGKILL is
> preferable than TIF_MEMDIE.

This alone wouldn't cause any issue wrt. oom_killer_disable expected
behavior ATM because the only user of this functionality is the PM
freezer itself and that is a sync context so it cannot unexpectedly
race with the suspend progress but it is true that this check is more
robust. Even though this is _really_ unlikely because the suspend
usually doesn't share the mm with a large task it doesn't make any sense
to continue with the suspend if the trigger itself has been killed by
the OOM killer so this is a minor improvement.
 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/oom_kill.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 6ebc0351..914451a 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -613,15 +613,11 @@ void exit_oom_victim(struct task_struct *tsk)
>  bool oom_killer_disable(void)
>  {
>  	/*
> -	 * Make sure to not race with an ongoing OOM killer
> -	 * and that the current is not the victim.
> +	 * Make sure to not race with an ongoing OOM killer. Check that the
> +	 * current is not killed (possibly due to sharing the victim's memory).
>  	 */
> -	mutex_lock(&oom_lock);
> -	if (test_thread_flag(TIF_MEMDIE)) {
> -		mutex_unlock(&oom_lock);
> +	if (mutex_lock_killable(&oom_lock))
>  		return false;
> -	}
> -
>  	oom_killer_disabled = true;
>  	mutex_unlock(&oom_lock);
>  
> -- 
> 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>

-- 
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,oom: make oom_killer_disable() killable.

[David Rientjes]

On Sun, 24 Jan 2016, Tetsuo Handa wrote:

> While oom_killer_disable() is called by freeze_processes() after all user
> threads except the current thread are frozen, it is possible that kernel
> threads invoke the OOM killer and sends SIGKILL to the current thread due
> to sharing the thawed victim's memory. Therefore, checking for SIGKILL is
> preferable than TIF_MEMDIE.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: David Rientjes <rientjes@google.com>

--
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,oom: make oom_killer_disable() killable.

[Michal Hocko]

On Sat 09-01-16 20:04:45, Tetsuo Handa wrote:
> While oom_killer_disable() is called by freeze_processes() after all user
> threads except the current thread are frozen, it is possible that kernel
> threads invoke the OOM killer and sends SIGKILL to the current thread due
> to sharing the thawed victim's memory. Therefore, checking for SIGKILL is
> preferable than TIF_MEMDIE.

OK, this sounds like a good idea. Reducing TIF_MEMDIE usage outside of
the oom/mm proper is indeed desirable.

> Also, it is possible that the thawed victim fails to terminate due to
> invisible dependency. Therefore, waiting with timeout is preferable.
> The timeout is copied from __usermodehelper_disable() called by
> freeze_processes().

__usermodehelper_disable and oom_killer_disable do follow a similar
pattern so it is probably a good idea to handle them the same way. I
would just ask this to be in a separate patch and ideally share the same
constant rather than a magic constant.

Thanks!

> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  mm/oom_kill.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index b8a4210..bafa6b2 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -612,21 +612,19 @@ void exit_oom_victim(void)
>  bool oom_killer_disable(void)
>  {
>  	/*
> -	 * Make sure to not race with an ongoing OOM killer
> -	 * and that the current is not the victim.
> +	 * Make sure to not race with an ongoing OOM killer. Check that the
> +	 * current is not killed (possibly due to sharing the victim's memory).
>  	 */
> -	mutex_lock(&oom_lock);
> -	if (test_thread_flag(TIF_MEMDIE)) {
> -		mutex_unlock(&oom_lock);
> +	if (mutex_lock_killable(&oom_lock))
>  		return false;
> -	}
> -
>  	oom_killer_disabled = true;
>  	mutex_unlock(&oom_lock);
>  
> -	wait_event(oom_victims_wait, !atomic_read(&oom_victims));
> -
> -	return true;
> +	/* Do not wait forever in case existing victims got stuck. */
> +	if (!wait_event_timeout(oom_victims_wait, !atomic_read(&oom_victims),
> +				5 * HZ))
> +		oom_killer_disabled = false;
> +	return oom_killer_disabled;
>  }
>  
>  /**
> -- 
> 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>

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

[PATCH] mm,oom: make oom_killer_disable() killable.

[Tetsuo Handa]

While oom_killer_disable() is called by freeze_processes() after all user
threads except the current thread are frozen, it is possible that kernel
threads invoke the OOM killer and sends SIGKILL to the current thread due
to sharing the thawed victim's memory. Therefore, checking for SIGKILL is
preferable than TIF_MEMDIE.

Also, it is possible that the thawed victim fails to terminate due to
invisible dependency. Therefore, waiting with timeout is preferable.
The timeout is copied from __usermodehelper_disable() called by
freeze_processes().

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

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index b8a4210..bafa6b2 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -612,21 +612,19 @@ void exit_oom_victim(void)
 bool oom_killer_disable(void)
 {
 	/*
-	 * Make sure to not race with an ongoing OOM killer
-	 * and that the current is not the victim.
+	 * Make sure to not race with an ongoing OOM killer. Check that the
+	 * current is not killed (possibly due to sharing the victim's memory).
 	 */
-	mutex_lock(&oom_lock);
-	if (test_thread_flag(TIF_MEMDIE)) {
-		mutex_unlock(&oom_lock);
+	if (mutex_lock_killable(&oom_lock))
 		return false;
-	}
-
 	oom_killer_disabled = true;
 	mutex_unlock(&oom_lock);
 
-	wait_event(oom_victims_wait, !atomic_read(&oom_victims));
-
-	return true;
+	/* Do not wait forever in case existing victims got stuck. */
+	if (!wait_event_timeout(oom_victims_wait, !atomic_read(&oom_victims),
+				5 * HZ))
+		oom_killer_disabled = false;
+	return oom_killer_disabled;
 }
 
 /**
-- 
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>