Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1673034 > unrolled thread

Re: [v3 1/6] mm, oom: use oom_victims counter to synchronize oom victim selection

Started byTetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
First post2017-06-22 22:40 +0200
Last post2017-06-29 22:20 +0200
Articles 3 — 1 participant

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [v3 1/6] mm, oom: use oom_victims counter to synchronize oom victim selection Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-22 22:40 +0200
    Re: [v3 1/6] mm, oom: use oom_victims counter to synchronize oom victim selection Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-23 00:00 +0200
      Re: [v3 1/6] mm, oom: use oom_victims counter to synchronize oom victim selection Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-29 22:20 +0200

#1673034 — Re: [v3 1/6] mm, oom: use oom_victims counter to synchronize oom victim selection

FromTetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date2017-06-22 22:40 +0200
SubjectRe: [v3 1/6] mm, oom: use oom_victims counter to synchronize oom victim selection
Message-ID<tVh3s-18t-7@gated-at.bofh.it>
Roman Gushchin wrote:
> On Thu, Jun 22, 2017 at 09:40:28AM +0900, Tetsuo Handa wrote:
> > Roman Gushchin wrote:
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -992,6 +992,13 @@ bool out_of_memory(struct oom_control *oc)
> > >  	if (oom_killer_disabled)
> > >  		return false;
> > >  
> > > +	/*
> > > +	 * If there are oom victims in flight, we don't need to select
> > > +	 * a new victim.
> > > +	 */
> > > +	if (atomic_read(&oom_victims) > 0)
> > > +		return true;
> > > +
> > >  	if (!is_memcg_oom(oc)) {
> > >  		blocking_notifier_call_chain(&oom_notify_list, 0, &freed);
> > >  		if (freed > 0)
> > 
> > Above in this patch and below in patch 5 are wrong.
> > 
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -665,7 +672,13 @@ static void mark_oom_victim(struct task_struct *tsk)
> > >  	 * that TIF_MEMDIE tasks should be ignored.
> > >  	 */
> > >  	__thaw_task(tsk);
> > > -	atomic_inc(&oom_victims);
> > > +
> > > +	/*
> > > +	 * If there are no oom victims in flight,
> > > +	 * give the task an access to the memory reserves.
> > > +	 */
> > > +	if (atomic_inc_return(&oom_victims) == 1)
> > > +		set_tsk_thread_flag(tsk, TIF_MEMDIE);
> > >  }
> > >  
> > >  /**
> > 
> > The OOM reaper is not available for CONFIG_MMU=n kernels, and timeout based
> > giveup is not permitted, but a multithreaded process might be selected as
> > an OOM victim. Not setting TIF_MEMDIE to all threads sharing an OOM victim's
> > mm increases possibility of preventing some OOM victim thread from terminating
> > (e.g. one of them cannot leave __alloc_pages_slowpath() with mmap_sem held for
> > write due to waiting for the TIF_MEMDIE thread to call exit_oom_victim() when
> > the TIF_MEMDIE thread is waiting for the thread with mmap_sem held for write).
> 
> I agree, that CONFIG_MMU=n is a special case, and the proposed approach can't
> be used directly. But can you, please, why do you find the first  chunk wrong?

Since you are checking oom_victims before checking task_will_free_mem(current),
only one thread can get TIF_MEMDIE. This is where a multithreaded OOM victim without
the OOM reaper can get stuck forever.

> Basically, it's exactly what we do now: we increment oom_victims for every oom
> victim, and every process decrements this counter during it's exit path.
> If there is at least one existing victim, we will select it again, so it's just
> an optimization. Am I missing something? Why should we start new victim selection
> if there processes that will likely quit and release memory soon?

If you check oom_victims like
http://lkml.kernel.org/r/201706220053.v5M0rmOU078764@www262.sakura.ne.jp does,
all threads in a multithreaded OOM victim will have a chance to get TIF_MEMDIE.



By the way, below in patch 5 is also wrong.
You are not permitted to set TIF_MEMDIE if oom_killer_disabled == true.

--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -556,8 +556,18 @@ static void oom_reap_task(struct task_struct *tsk)
 	struct mm_struct *mm = tsk->signal->oom_mm;
 
 	/* Retry the down_read_trylock(mmap_sem) a few times */
-	while (attempts++ < MAX_OOM_REAP_RETRIES && !__oom_reap_task_mm(tsk, mm))
+	while (attempts++ < MAX_OOM_REAP_RETRIES &&
+	       !__oom_reap_task_mm(tsk, mm)) {
+
+		/*
+		 * If the task has no access to the memory reserves,
+		 * grant it to help the task to exit.
+		 */
+		if (!test_tsk_thread_flag(tsk, TIF_MEMDIE))
+			set_tsk_thread_flag(tsk, TIF_MEMDIE);
+
 		schedule_timeout_idle(HZ/10);
+	}
 
 	if (attempts <= MAX_OOM_REAP_RETRIES)
 		goto done;

[toc] | [next] | [standalone]

#1673079

FromTetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date2017-06-23 00:00 +0200
Message-ID<tViiR-1PL-5@gated-at.bofh.it>
In reply to#1673034
Tetsuo Handa wrote:
> Roman Gushchin wrote:
> > On Thu, Jun 22, 2017 at 09:40:28AM +0900, Tetsuo Handa wrote:
> > > Roman Gushchin wrote:
> > > > --- a/mm/oom_kill.c
> > > > +++ b/mm/oom_kill.c
> > > > @@ -992,6 +992,13 @@ bool out_of_memory(struct oom_control *oc)
> > > >  	if (oom_killer_disabled)
> > > >  		return false;
> > > >  
> > > > +	/*
> > > > +	 * If there are oom victims in flight, we don't need to select
> > > > +	 * a new victim.
> > > > +	 */
> > > > +	if (atomic_read(&oom_victims) > 0)
> > > > +		return true;
> > > > +
> > > >  	if (!is_memcg_oom(oc)) {
> > > >  		blocking_notifier_call_chain(&oom_notify_list, 0, &freed);
> > > >  		if (freed > 0)
> > > 
> > > The OOM reaper is not available for CONFIG_MMU=n kernels, and timeout based
> > > giveup is not permitted, but a multithreaded process might be selected as
> > > an OOM victim. Not setting TIF_MEMDIE to all threads sharing an OOM victim's
> > > mm increases possibility of preventing some OOM victim thread from terminating
> > > (e.g. one of them cannot leave __alloc_pages_slowpath() with mmap_sem held for
> > > write due to waiting for the TIF_MEMDIE thread to call exit_oom_victim() when
> > > the TIF_MEMDIE thread is waiting for the thread with mmap_sem held for write).
> > 
> > I agree, that CONFIG_MMU=n is a special case, and the proposed approach can't
> > be used directly. But can you, please, why do you find the first  chunk wrong?
> 
> Since you are checking oom_victims before checking task_will_free_mem(current),
> only one thread can get TIF_MEMDIE. This is where a multithreaded OOM victim without
> the OOM reaper can get stuck forever.

Oops, I misinterpreted. This is where a multithreaded OOM victim with or without
the OOM reaper can get stuck forever. Think about a process with two threads is
selected by the OOM killer and only one of these two threads can get TIF_MEMDIE.

  Thread-1                 Thread-2                 The OOM killer           The OOM reaper

                           Calls down_write(&current->mm->mmap_sem).
  Enters __alloc_pages_slowpath().
                           Enters __alloc_pages_slowpath().
  Takes oom_lock.
  Calls out_of_memory().
                                                    Selects Thread-1 as an OOM victim.
  Gets SIGKILL.            Gets SIGKILL.
  Gets TIF_MEMDIE.
  Releases oom_lock.
  Leaves __alloc_pages_slowpath() because Thread-1 has TIF_MEMDIE.
                                                                             Takes oom_lock.
                                                                             Will do nothing because down_read_trylock() fails.
                                                                             Releases oom_lock.
                                                                             Gives up and sets MMF_OOM_SKIP after one second.
                           Takes oom_lock.
                           Calls out_of_memory().
                           Will not check MMF_OOM_SKIP because Thread-1 still has TIF_MEMDIE. // <= get stuck waiting for Thread-1.
                           Releases oom_lock.
                           Will not leave __alloc_pages_slowpath() because Thread-2 does not have TIF_MEMDIE.
                           Will not call up_write(&current->mm->mmap_sem).
  Reaches do_exit().
  Calls down_read(&current->mm->mmap_sem) in exit_mm() in do_exit(). // <= get stuck waiting for Thread-2.
  Will not call up_read(&current->mm->mmap_sem) in exit_mm() in do_exit().
  Will not clear TIF_MEMDIE in exit_oom_victim() in exit_mm() in do_exit().

[toc] | [prev] | [next] | [standalone]

#1678159

FromTetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date2017-06-29 22:20 +0200
Message-ID<tXO4W-3Qo-11@gated-at.bofh.it>
In reply to#1673079
Roman Gushchin wrote:
> On Fri, Jun 23, 2017 at 06:52:20AM +0900, Tetsuo Handa wrote:
> > Tetsuo Handa wrote:
> > Oops, I misinterpreted. This is where a multithreaded OOM victim with or without
> > the OOM reaper can get stuck forever. Think about a process with two threads is
> > selected by the OOM killer and only one of these two threads can get TIF_MEMDIE.
> > 
> >   Thread-1                 Thread-2                 The OOM killer           The OOM reaper
> > 
> >                            Calls down_write(&current->mm->mmap_sem).
> >   Enters __alloc_pages_slowpath().
> >                            Enters __alloc_pages_slowpath().
> >   Takes oom_lock.
> >   Calls out_of_memory().
> >                                                     Selects Thread-1 as an OOM victim.
> >   Gets SIGKILL.            Gets SIGKILL.
> >   Gets TIF_MEMDIE.
> >   Releases oom_lock.
> >   Leaves __alloc_pages_slowpath() because Thread-1 has TIF_MEMDIE.
> >                                                                              Takes oom_lock.
> >                                                                              Will do nothing because down_read_trylock() fails.
> >                                                                              Releases oom_lock.
> >                                                                              Gives up and sets MMF_OOM_SKIP after one second.
> >                            Takes oom_lock.
> >                            Calls out_of_memory().
> >                            Will not check MMF_OOM_SKIP because Thread-1 still has TIF_MEMDIE. // <= get stuck waiting for Thread-1.
> >                            Releases oom_lock.
> >                            Will not leave __alloc_pages_slowpath() because Thread-2 does not have TIF_MEMDIE.
> >                            Will not call up_write(&current->mm->mmap_sem).
> >   Reaches do_exit().
> >   Calls down_read(&current->mm->mmap_sem) in exit_mm() in do_exit(). // <= get stuck waiting for Thread-2.
> >   Will not call up_read(&current->mm->mmap_sem) in exit_mm() in do_exit().
> >   Will not clear TIF_MEMDIE in exit_oom_victim() in exit_mm() in do_exit().
> 
> That's interesting... Does it mean, that we have to give an access to the reserves
> to all threads to guarantee the forward progress?

Yes, for we don't have __GFP_KILLABLE flag.

> 
> What do you think about Michal's approach? He posted a link in the thread.

Please read that thread.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web