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

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>

What is oom_killer_disable() for?

[Tetsuo Handa]

I wonder what oom_killer_disable() wants to do.

(1) We need to save a consistent memory snapshot image when suspending,
    is this correct?

(2) To obtain a consistent memory snapshot image, we need to freeze all
    but current thread in order to avoid modifying on-memory data while
    saving to disk, is this correct?

(3) Then, what is the purpose of disabling the OOM killer? Why do we
    need to disable the OOM killer? Is it because the OOM killer thaws
    already frozen threads?

(4) Then, why do we wait for TIF_MEMDIE threads to terminate? We can
    freeze thawed threads again without waiting for TIF_MEMDIE threads,
    can't we? Is it because we need free memory for saving to disk?

(5) Then, why waiting for only TIF_MEMDIE threads is sufficient? There
    is no TIF_MEMDIE threads does not guarantee that we have free memory,
    for there might be !TIF_MEMDIE threads which are still sharing memory
    used by TIF_MEMDIE threads.

(6) Since oom_killer_disable() already disabled the OOM killer,
    !TIF_MEMDIE threads which are sharing memory used by TIF_MEMDIE
    threads cannot get TIF_MEMDIE by calling out_of_memory().
    Also, since out_of_memory() returns false after oom_killer_disable()
    disabled the OOM killer, allocation requests by these !TIF_MEMDIE
    threads start failing. Why do we need to give up with accepting
    undesirable errors (e.g. failure of syscalls which modify an object's
    attribute)? Why don't we abort suspend operation by marking that
    re-enabling of the OOM killer might caused modification of on-memory
    data (like patch shown below)? We can make final decision after memory
    image snapshot is saved to disk, can't we? We can reply users that
    "suspend operation was cancelled due to out of memory" and ask users
    to try again, instead of failing e.g. chmod() syscall and/or asking
    users to check serial console for errors after the OOM killer was
    disabled.

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index b7342a2..665a559 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -588,6 +588,8 @@ int hibernation_platform_enter(void)
 	return error;
 }
 
+extern bool oom_killer_disable_aborted;
+
 /**
  * power_down - Shut the machine down for hibernation.
  *
@@ -600,6 +602,10 @@ static void power_down(void)
 #ifdef CONFIG_SUSPEND
 	int error;
 #endif
+	if (oom_killer_disable_aborted) {
+		printk(KERN_ERR "PM: Suspend aborted due to out of memory\n");
+		return;
+	}
 
 	switch (hibernation_mode) {
 	case HIBERNATION_REBOOT:
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index bafa6b2..07ed44a 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -426,6 +426,7 @@ static atomic_t oom_victims = ATOMIC_INIT(0);
 static DECLARE_WAIT_QUEUE_HEAD(oom_victims_wait);
 
 bool oom_killer_disabled __read_mostly;
+bool oom_killer_disable_aborted __read_mostly;
 
 #ifdef CONFIG_MMU
 /*
@@ -618,6 +619,7 @@ bool oom_killer_disable(void)
 	if (mutex_lock_killable(&oom_lock))
 		return false;
 	oom_killer_disabled = true;
+	oom_killer_disable_aborted = false;
 	mutex_unlock(&oom_lock);
 
 	/* Do not wait forever in case existing victims got stuck. */
@@ -632,6 +634,7 @@ bool oom_killer_disable(void)
  */
 void oom_killer_enable(void)
 {
+	oom_killer_disable_aborted = false;
 	oom_killer_disabled = false;
 }
 
@@ -840,8 +843,10 @@ bool out_of_memory(struct oom_control *oc)
 	unsigned int uninitialized_var(points);
 	enum oom_constraint constraint = CONSTRAINT_NONE;
 
-	if (oom_killer_disabled)
-		return false;
+	if (oom_killer_disabled) {
+		oom_killer_disable_aborted = true;
+		oom_killer_disabled = false;
+	}
 
 	blocking_notifier_call_chain(&oom_notify_list, 0, &freed);
 	if (freed > 0)

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

Re: What is oom_killer_disable() for?

[Michal Hocko]

[CCing Rafael]

On Sun 10-01-16 02:02:32, Tetsuo Handa wrote:
> I wonder what oom_killer_disable() wants to do.
> 
> (1) We need to save a consistent memory snapshot image when suspending,
>     is this correct?

Yes

> (2) To obtain a consistent memory snapshot image, we need to freeze all
>     but current thread in order to avoid modifying on-memory data while
>     saving to disk, is this correct?

Yes, the system has to enter a quiescent state where no further user
space activity can interfere with the PM suspend.
 
> (3) Then, what is the purpose of disabling the OOM killer? Why do we
>     need to disable the OOM killer? Is it because the OOM killer thaws
>     already frozen threads?

Yes. We have to be able to thaw frozen tasks if they are chosen as an
OOM victim otherwise you could hide processes into the fridge and lockup
the system. oom_killer_disable is then needed for pm freezer because the
OOM killer might be invoked even after all the userspace is considered
in the quiescent state. We cannot wake any task anymore during the later
pm freezer processing AFAIU. oom_killer_disable then acts as a hard
barrier after which we know that even OOM killer won't wake any user
space tasks.

> (4) Then, why do we wait for TIF_MEMDIE threads to terminate? We can
>     freeze thawed threads again without waiting for TIF_MEMDIE threads,
>     can't we? Is it because we need free memory for saving to disk?

It is preferable to finish the OOM killer handling before entering the
quiescent state. As only TIF_MEMDIE tasks are thawed we do not wait for
all killed task. If this alone doesn't help to resolve the OOM condition
then we will likely fail later in the process when a memory allocation
is required and ENOMEM terminate the whole process.

> (5) Then, why waiting for only TIF_MEMDIE threads is sufficient? There
>     is no TIF_MEMDIE threads does not guarantee that we have free memory,
>     for there might be !TIF_MEMDIE threads which are still sharing memory
>     used by TIF_MEMDIE threads.

see above. We are trying to be as good as possible but not perfect. The
only hard requirement is that an unexpected OOM victim won't interfere
with a code which doesn't expect userspace to run.

> (6) Since oom_killer_disable() already disabled the OOM killer,
>     !TIF_MEMDIE threads which are sharing memory used by TIF_MEMDIE
>     threads cannot get TIF_MEMDIE by calling out_of_memory().

They shouldn't be running at all because the whole userspace has been
frozen. Only TIF_MEMDIE tasks are running at the time we are disabling
the oom killer and we are waiting for them. Please go and read through
c32b3cbe0d06 ("oom, PM: make OOM detection in the freezer path raceless")

>     Also, since out_of_memory() returns false after oom_killer_disable()
>     disabled the OOM killer, allocation requests by these !TIF_MEMDIE
>     threads start failing. Why do we need to give up with accepting
>     undesirable errors (e.g. failure of syscalls which modify an object's
>     attribute)?

Userspace shouldn't see unexpected errors due to OOM being disabled
because it is not runable at that time.

>     Why don't we abort suspend operation by marking that
>     re-enabling of the OOM killer might caused modification of on-memory
>     data (like patch shown below)? We can make final decision after memory
>     image snapshot is saved to disk, can't we?

I am not sure I am following you here but how do you detect that the
userspace has corrupted your image or accesses an already (half)
suspended device or something similar?

I am not saying disabling the OOM killer is the greatest solution. It
took quite some time to make it work reliably. But I fail to see how can
we guarantee no userspace interference when an OOM victim might be woken
by any allocation deep inside the PM code path. I believe we simply
need a point of no userspace activity to proceed with further PM steps
reasonably.
-- 
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: What is oom_killer_disable() for?

[Tetsuo Handa]

Michal Hocko write:
> As only TIF_MEMDIE tasks are thawed we do not wait for all killed task.

Ah, I see.

I thought that TIF_MEMDIE && SIGKILL && PF_FROZEN tasks are woken by
try_to_wake_up(p, TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE, 0) via __thaw_task(p),
but SIGKILL tasks are anyway (i.e. regardless of TIF_MEMDIE and PF_FROZEN flags)
woken by try_to_wake_up(p, TASK_WAKEKILL | TASK_INTERRUPTIBLE, 0) via
do_send_sig_info(p).

----------
mark_oom_victim(struct task_struct *tsk) {
  __thaw_task(tsk) {
    if (frozen(p)) /* p->flags & PF_FROZEN */
      wake_up_process(p) {
        try_to_wake_up(p, TASK_NORMAL, 0) { /* TASK_NORMAL is TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE */
          if (!(p->state & state))
            goto out;
          success = 1; /* we're going to change ->state */
        }
      }
  }
}

do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true) {
  send_signal(sig, info, p, group) {
    __send_signal(sig, info, t, group, from_ancestor_ns) {
      if (info == SEND_SIG_FORCED) /* info is SEND_SIG_FORCED */
        goto out_set;
      out_set:
        complete_signal(sig, t, group) {
          signal_wake_up(t, sig == SIGKILL) {
            signal_wake_up_state(t, resume ? TASK_WAKEKILL : 0) { /* resume is 1 */
              wake_up_state(t, state | TASK_INTERRUPTIBLE) { /* state is TASK_WAKEKILL */
                try_to_wake_up(p, state, 0) { /* state is TASK_WAKEKILL | TASK_INTERRUPTIBLE */
                  if (!(p->state & state))
                    goto out;
                  success = 1; /* we're going to change ->state */
                }
              }
            }
          }
        }
    }
  }
}
----------

But frozen tasks are looping inside for(;;) { ... } at __refrigerator(),
and only TIF_MEMDIE tasks can escape this loop by

  if (test_thread_flag(TIF_MEMDIE))
    return false;

in freezing_slow_path().

Then, assuming that any task which is looping inside this loop has no
locks held, current oom_killer_disable() function which assumes that

  wait_event(oom_victims_wait, !atomic_read(&oom_victims));

is guaranteed to return shortly is unsafe if TIF_MEMDIE tasks are
waiting for locks held by not-yet-frozen kernel threads?



> >     Why don't we abort suspend operation by marking that
> >     re-enabling of the OOM killer might caused modification of on-memory
> >     data (like patch shown below)? We can make final decision after memory
> >     image snapshot is saved to disk, can't we?
>
> I am not sure I am following you here but how do you detect that the
> userspace has corrupted your image or accesses an already (half)
> suspended device or something similar?

Can't we determine whether the OOM killer might have corrupted our image
by checking whether oom_killer_disabled is kept true until the point of
final decision?

To me, satisfying allocation requests by kernel threads by invoking the
OOM killer and aborting suspend operation if the OOM killer was invoked
sounds cleaner than forcing !__GFP_NOFAIL allocation requests to fail.

--
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: What is oom_killer_disable() for?

[Rafael J. Wysocki]

On Monday, January 11, 2016 03:49:24 PM Michal Hocko wrote:
> [CCing Rafael]
> 
> On Sun 10-01-16 02:02:32, Tetsuo Handa wrote:
> > I wonder what oom_killer_disable() wants to do.
> > 
> > (1) We need to save a consistent memory snapshot image when suspending,
> >     is this correct?
> 
> Yes
> 
> > (2) To obtain a consistent memory snapshot image, we need to freeze all
> >     but current thread in order to avoid modifying on-memory data while
> >     saving to disk, is this correct?
> 
> Yes, the system has to enter a quiescent state where no further user
> space activity can interfere with the PM suspend.

The system has to be quiescent during the image creation only.

After the image has been created, it may do whatever it likes so long as
the contents of persistent storage is consistent with the information
related to it included in the image.

> > (3) Then, what is the purpose of disabling the OOM killer? Why do we
> >     need to disable the OOM killer? Is it because the OOM killer thaws
> >     already frozen threads?
> 
> Yes. We have to be able to thaw frozen tasks if they are chosen as an
> OOM victim otherwise you could hide processes into the fridge and lockup
> the system. oom_killer_disable is then needed for pm freezer because the
> OOM killer might be invoked even after all the userspace is considered
> in the quiescent state. We cannot wake any task anymore during the later
> pm freezer processing AFAIU. oom_killer_disable then acts as a hard
> barrier after which we know that even OOM killer won't wake any user
> space tasks.

So the reason(s) why user space tasks are frozen is not specific to hibernation.

The main reason is to take user space out of the way so that device drivers
don't need to worry about possible interactions with user space while the
devices are being suspended and resumed.  IOW, this allows user space to
see a consistent state of the system before and after suspend/resume without
being exposed to itermediate, possibly inconsistent, states.

> > (4) Then, why do we wait for TIF_MEMDIE threads to terminate? We can
> >     freeze thawed threads again without waiting for TIF_MEMDIE threads,
> >     can't we? Is it because we need free memory for saving to disk?
> 
> It is preferable to finish the OOM killer handling before entering the
> quiescent state. As only TIF_MEMDIE tasks are thawed we do not wait for
> all killed task. If this alone doesn't help to resolve the OOM condition
> then we will likely fail later in the process when a memory allocation
> is required and ENOMEM terminate the whole process.
> 
> > (5) Then, why waiting for only TIF_MEMDIE threads is sufficient? There
> >     is no TIF_MEMDIE threads does not guarantee that we have free memory,
> >     for there might be !TIF_MEMDIE threads which are still sharing memory
> >     used by TIF_MEMDIE threads.
> 
> see above. We are trying to be as good as possible but not perfect. The
> only hard requirement is that an unexpected OOM victim won't interfere
> with a code which doesn't expect userspace to run.

Right.

In particular, it must not result in an access to a suspended device.

> > (6) Since oom_killer_disable() already disabled the OOM killer,
> >     !TIF_MEMDIE threads which are sharing memory used by TIF_MEMDIE
> >     threads cannot get TIF_MEMDIE by calling out_of_memory().
> 
> They shouldn't be running at all because the whole userspace has been
> frozen. Only TIF_MEMDIE tasks are running at the time we are disabling
> the oom killer and we are waiting for them. Please go and read through
> c32b3cbe0d06 ("oom, PM: make OOM detection in the freezer path raceless")
> 
> >     Also, since out_of_memory() returns false after oom_killer_disable()
> >     disabled the OOM killer, allocation requests by these !TIF_MEMDIE
> >     threads start failing. Why do we need to give up with accepting
> >     undesirable errors (e.g. failure of syscalls which modify an object's
> >     attribute)?
> 
> Userspace shouldn't see unexpected errors due to OOM being disabled
> because it is not runable at that time.
> 
> >     Why don't we abort suspend operation by marking that
> >     re-enabling of the OOM killer might caused modification of on-memory
> >     data (like patch shown below)? We can make final decision after memory
> >     image snapshot is saved to disk, can't we?
> 
> I am not sure I am following you here but how do you detect that the
> userspace has corrupted your image or accesses an already (half)
> suspended device or something similar?

Right.

As I said above, the (main) reason for user space freezing is not specific
to hibernation and it is only indirectly related to the creation of the memory
snapshot.

> I am not saying disabling the OOM killer is the greatest solution. It
> took quite some time to make it work reliably. But I fail to see how can
> we guarantee no userspace interference when an OOM victim might be woken
> by any allocation deep inside the PM code path. I believe we simply
> need a point of no userspace activity to proceed with further PM steps
> reasonably.

Agreed.

Thanks,
Rafael

--
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: What is oom_killer_disable() for?

[Rafael J. Wysocki]

On Tuesday, January 12, 2016 07:17:19 PM Tetsuo Handa wrote:
> Michal Hocko write:

[cut]

> > I am not sure I am following you here but how do you detect that the
> > userspace has corrupted your image or accesses an already (half)
> > suspended device or something similar?
> 
> Can't we determine whether the OOM killer might have corrupted our image
> by checking whether oom_killer_disabled is kept true until the point of
> final decision?

The freezing is really not about keeping the image consistent etc.  It is
not about hibernation specifically even.

> To me, satisfying allocation requests by kernel threads by invoking the
> OOM killer and aborting suspend operation if the OOM killer was invoked
> sounds cleaner than forcing !__GFP_NOFAIL allocation requests to fail.

What if the suspend is on emergency, like low battery or thermal?

Thanks,
Rafael

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