linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Minchan Kim <minchan@kernel.org>
To: David Rientjes <rientjes@google.com>
Cc: Mel Gorman <mgorman@suse.de>,
	Luigi Semenzato <semenzato@google.com>,
	linux-mm@kvack.org, Dan Magenheimer <dan.magenheimer@oracle.com>,
	KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>,
	Sonny Rao <sonnyrao@google.com>
Subject: Re: zram OOM behavior
Date: Thu, 1 Nov 2012 14:26:33 +0900	[thread overview]
Message-ID: <20121101052633.GF24883@bbox> (raw)
In-Reply-To: <alpine.DEB.2.00.1210312140090.17607@chino.kir.corp.google.com>

On Wed, Oct 31, 2012 at 09:48:57PM -0700, David Rientjes wrote:
> On Thu, 1 Nov 2012, Minchan Kim wrote:
> 
> > It's not true any more.
> > 3.6 includes following code in try_to_free_pages
> > 
> >         /*   
> >          * Do not enter reclaim if fatal signal is pending. 1 is returned so
> >          * that the page allocator does not consider triggering OOM
> >          */
> >         if (fatal_signal_pending(current))
> >                 return 1;
> > 
> > So the hunged task never go to the OOM path and could be looping forever.
> > 
> 
> Ah, interesting.  This is from commit 5515061d22f0 ("mm: throttle direct 
> reclaimers if PF_MEMALLOC reserves are low and swap is backed by network 
> storage").  Thanks for adding Mel to the cc.
> 
> The oom killer specifically has logic for this condition: when calling 
> out_of_memory() the first thing it does is
> 
> 	if (fatal_signal_pending(current))
> 		set_thread_flag(TIF_MEMDIE);
> 
> to allow it access to memory reserves so that it may exit if it's having 
> trouble.  But that ends up never happening because of the above code that 
> Minchan has identified.
> 
> So we either need to do set_thread_flag(TIF_MEMDIE) in try_to_free_pages() 
> as well or revert that early return entirely; there's no justification 
> given for it in the comment nor in the commit log.  I'd rather remove it 
> and allow the oom killer to trigger and grant access to memory reserves 
> itself if necessary.
> 
> Mel, how does commit 5515061d22f0 deal with threads looping forever if 
> they need memory in the exit path since the oom killer never gets called?
> 
> That aside, it doesn't seem like this is the issue that Luigi is reporting 
> since his patch that avoids deferring the oom killer presumably fixes the 
> issue for him.  So it turns out the oom killer must be getting called.

Exactly.

> 
> Luigi, can you try this instead?  It applies to the latest git but should 
> be easily modified to apply to any 3.x kernel you're running.
> ---
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -310,26 +310,13 @@ enum oom_scan_t oom_scan_process_thread(struct task_struct *task,
>  	if (!task->mm)
>  		return OOM_SCAN_CONTINUE;
>  
> -	if (task->flags & PF_EXITING) {
> +	if (task->flags & PF_EXITING && !force_kill) {
>  		/*
> -		 * If task is current and is in the process of releasing memory,
> -		 * allow the "kill" to set TIF_MEMDIE, which will allow it to
> -		 * access memory reserves.  Otherwise, it may stall forever.
> -		 *
> -		 * The iteration isn't broken here, however, in case other
> -		 * threads are found to have already been oom killed.
> +		 * If this task is not being ptraced on exit, then wait for it
> +		 * to finish before killing some other task unnecessarily.
>  		 */
> -		if (task == current)
> -			return OOM_SCAN_SELECT;
> -		else if (!force_kill) {
> -			/*
> -			 * If this task is not being ptraced on exit, then wait
> -			 * for it to finish before killing some other task
> -			 * unnecessarily.
> -			 */
> -			if (!(task->group_leader->ptrace & PT_TRACE_EXIT))
> -				return OOM_SCAN_ABORT;
> -		}
> +		if (!(task->group_leader->ptrace & PT_TRACE_EXIT))
> +			return OOM_SCAN_ABORT;
>  	}
>  	return OOM_SCAN_OK;
>  }
> @@ -706,11 +693,11 @@ void out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
>  		return;
>  
>  	/*
> -	 * 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 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 (fatal_signal_pending(current)) {
> +	if (fatal_signal_pending(current) || current->flags & PF_EXITING) {
>  		set_thread_flag(TIF_MEMDIE);
>  		return;
>  	}

Looks good to me.

-- 
Kind regards,
Minchan Kim

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

  reply	other threads:[~2012-11-01  5:20 UTC|newest]

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-28 17:32 zram OOM behavior Luigi Semenzato
2012-10-03 13:30 ` Konrad Rzeszutek Wilk
     [not found]   ` <CAA25o9SwO209DD6CUx-LzhMt9XU6niGJ-fBPmgwfcrUvf0BPWA@mail.gmail.com>
2012-10-12 23:30     ` Luigi Semenzato
2012-10-15 14:44 ` Minchan Kim
2012-10-15 18:54   ` Luigi Semenzato
2012-10-16  6:18     ` Minchan Kim
2012-10-16 17:36       ` Luigi Semenzato
2012-10-19 17:49         ` Luigi Semenzato
2012-10-22 23:53           ` Minchan Kim
2012-10-23  0:40             ` Luigi Semenzato
2012-10-23  6:03             ` David Rientjes
2012-10-29 18:26               ` Luigi Semenzato
2012-10-29 19:00                 ` David Rientjes
2012-10-29 22:36                   ` Luigi Semenzato
2012-10-29 22:52                     ` David Rientjes
2012-10-29 23:23                       ` Luigi Semenzato
2012-10-29 23:34                         ` Luigi Semenzato
2012-10-30  0:18                     ` Minchan Kim
2012-10-30  0:45                       ` Luigi Semenzato
2012-10-30  5:41                         ` David Rientjes
2012-10-30 19:12                           ` Luigi Semenzato
2012-10-30 20:30                             ` Luigi Semenzato
2012-10-30 22:32                               ` Luigi Semenzato
2012-10-31 18:42                                 ` David Rientjes
2012-10-30 22:37                               ` Sonny Rao
2012-10-31  4:46                               ` David Rientjes
2012-10-31  6:14                                 ` Luigi Semenzato
2012-10-31  6:28                                   ` Luigi Semenzato
2012-10-31 18:45                                     ` David Rientjes
2012-10-31  0:57                             ` Minchan Kim
2012-10-31  1:06                               ` Luigi Semenzato
2012-10-31  1:27                                 ` Minchan Kim
2012-10-31  3:49                                   ` Luigi Semenzato
2012-10-31  7:24                                     ` Minchan Kim
2012-10-31 16:07                                       ` Luigi Semenzato
2012-10-31 17:49                                         ` Mandeep Singh Baines
2012-10-31 18:54                               ` David Rientjes
2012-10-31 21:40                                 ` Luigi Semenzato
2012-11-01  2:11                                 ` Minchan Kim
2012-11-01  4:38                                   ` David Rientjes
2012-11-01  5:18                                     ` Minchan Kim
2012-11-01  2:43                                 ` Minchan Kim
2012-11-01  4:48                                   ` David Rientjes
2012-11-01  5:26                                     ` Minchan Kim [this message]
2012-11-01  8:28                                     ` Mel Gorman
2012-11-01 15:57                                       ` Luigi Semenzato
2012-11-01 15:58                                         ` Luigi Semenzato
2012-11-01 21:48                                           ` David Rientjes
2012-11-01 17:50                                     ` Luigi Semenzato
2012-11-01 21:50                                       ` David Rientjes
2012-11-01 21:58                                         ` [patch] mm, oom: allow exiting threads to have access to memory reserves David Rientjes
2012-11-01 22:43                                           ` Andrew Morton
2012-11-01 23:05                                             ` David Rientjes
2012-11-01 23:06                                             ` Luigi Semenzato
2012-11-01 22:04                                         ` zram OOM behavior Luigi Semenzato
2012-11-01 22:25                                           ` David Rientjes
  -- strict thread matches above, loose matches on Subject: below --
2012-11-02  6:39 Minchan Kim
2012-11-02  8:30 ` Mel Gorman
2012-11-02 22:36   ` Minchan Kim
2012-11-05 14:46     ` Mel Gorman
2012-11-06  0:25       ` Minchan Kim
2012-11-06  8:58         ` Mel Gorman
2012-11-06 10:17           ` Minchan Kim
2012-11-09  9:50             ` Mel Gorman
2012-11-12 13:32               ` Minchan Kim
2012-11-12 14:06                 ` Mel Gorman
2012-11-13 13:31                   ` Minchan Kim

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121101052633.GF24883@bbox \
    --to=minchan@kernel.org \
    --cc=dan.magenheimer@oracle.com \
    --cc=kosaki.motohiro@jp.fujitsu.com \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@suse.de \
    --cc=rientjes@google.com \
    --cc=semenzato@google.com \
    --cc=sonnyrao@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).