From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752453AbbCZLYy (ORCPT ); Thu, 26 Mar 2015 07:24:54 -0400 Received: from gum.cmpxchg.org ([85.214.110.215]:51093 "EHLO gum.cmpxchg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751402AbbCZLYx (ORCPT ); Thu, 26 Mar 2015 07:24:53 -0400 Date: Thu, 26 Mar 2015 07:24:45 -0400 From: Johannes Weiner To: Tetsuo Handa Cc: linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, torvalds@linux-foundation.org, akpm@linux-foundation.org, ying.huang@intel.com, aarcange@redhat.com, david@fromorbit.com, mhocko@suse.cz, tytso@mit.edu Subject: Re: [patch 08/12] mm: page_alloc: wait for OOM killer progress before retrying Message-ID: <20150326112445.GC18560@cmpxchg.org> References: <1427264236-17249-1-git-send-email-hannes@cmpxchg.org> <1427264236-17249-9-git-send-email-hannes@cmpxchg.org> <201503252315.FBJ09847.FSOtOJQFOMLFVH@I-love.SAKURA.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201503252315.FBJ09847.FSOtOJQFOMLFVH@I-love.SAKURA.ne.jp> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Mar 25, 2015 at 11:15:48PM +0900, Tetsuo Handa wrote: > Johannes Weiner wrote: > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > > index 5cfda39b3268..e066ac7353a4 100644 > > --- a/mm/oom_kill.c > > +++ b/mm/oom_kill.c > > @@ -711,12 +711,15 @@ bool out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, > > killed = 1; > > } > > out: > > + if (test_thread_flag(TIF_MEMDIE)) > > + return true; > > /* > > - * Give the killed threads a good chance of exiting before trying to > > - * allocate memory again. > > + * Wait for any outstanding OOM victims to die. In rare cases > > + * victims can get stuck behind the allocating tasks, so the > > + * wait needs to be bounded. It's crude alright, but cheaper > > + * than keeping a global dependency tree between all tasks. > > */ > > - if (killed) > > - schedule_timeout_killable(1); > > + wait_event_timeout(oom_victims_wait, !atomic_read(&oom_victims), HZ); > > > > return true; > > } > > out_of_memory() returning true with bounded wait effectively means that > wait forever without choosing subsequent OOM victims when first OOM victim > failed to die. The system will lock up, won't it? The OOM killer already refuses to choose another victim as long as the first one hasn't exited, see oom_scan_process_thread(). That's why later patches in this series introduce a reserve for OOM-killing tasks and give nofail allocations access to emergency reserves, in case they themselves prevent that single OOM victim from exiting. But otherwise victims should be exiting eventually. > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index c1224ba45548..9ce9c4c083a0 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -2330,30 +2330,29 @@ void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...) > > } > > > > static inline struct page * > > -__alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, > > +__alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, int alloc_flags, > > const struct alloc_context *ac, unsigned long *did_some_progress) > > { > > - struct page *page; > > + struct page *page = NULL; > > > > *did_some_progress = 0; > > > > /* > > - * Acquire the oom lock. If that fails, somebody else is > > - * making progress for us. > > + * This allocating task can become the OOM victim itself at > > + * any point before acquiring the lock. In that case, exit > > + * quickly and don't block on the lock held by another task > > + * waiting for us to exit. > > */ > > - if (!mutex_trylock(&oom_lock)) { > > - *did_some_progress = 1; > > - schedule_timeout_uninterruptible(1); > > - return NULL; > > + if (test_thread_flag(TIF_MEMDIE) || mutex_lock_killable(&oom_lock)) { > > + alloc_flags |= ALLOC_NO_WATERMARKS; > > + goto alloc; > > } > > When a thread group has 1000 threads and most of them are doing memory allocation > request, all of them will get fatal_signal_pending() == true when one of them are > chosen by OOM killer. > This code will allow most of them to access memory reserves, won't it? Ah, good point! Only TIF_MEMDIE should get reserve access, not just any dying thread. Thanks, I'll fix it in v2. > > @@ -2383,12 +2382,20 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, > > if (gfp_mask & __GFP_THISNODE) > > goto out; > > } > > - /* Exhausted what can be done so it's blamo time */ > > - if (out_of_memory(ac->zonelist, gfp_mask, order, ac->nodemask, false) > > - || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) > > + > > + if (out_of_memory(ac->zonelist, gfp_mask, order, ac->nodemask, false)) { > > *did_some_progress = 1; > > + } else { > > + /* Oops, these shouldn't happen with the OOM killer disabled */ > > + if (WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) > > + *did_some_progress = 1; > > + } > > I think GFP_NOFAIL allocations need to involve OOM killer than > pretending as if forward progress is made. If all of in-flight > allocation requests are GFP_NOFAIL, the system will lock up. Hm? They do involve the OOM killer, but once userspace is frozen for suspend/hibernate we shouldn't kill and thaw random tasks anymore as that might corrupt the memory snapshot, so nofail allocations are a bug at this point. > After all, if we wait for OOM killer progress before retrying, I think > we should involve OOM killer after some bounded timeout regardless of > gfp flags, and let OOM killer kill more threads after another bounded > timeout. Otherwise, the corner cases will lock up the system. Giving nofail allocations access to emergency reserves targets this problem, but I agree with you that it's still possible for the system to lock up if they have been consumed and still no task made enough forward progress to release memory. It is unlikely but possible. I will probably come back to the OOM victim timeout patch some time in the future as that seems more robust. It would also drastically simplify memcg OOM handling. But that patch was controversial in the past and seemed beyond the scope of this patch set. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f50.google.com (mail-wg0-f50.google.com [74.125.82.50]) by kanga.kvack.org (Postfix) with ESMTP id DA2D76B0032 for ; Thu, 26 Mar 2015 07:24:52 -0400 (EDT) Received: by wgbcc7 with SMTP id cc7so60008453wgb.0 for ; Thu, 26 Mar 2015 04:24:52 -0700 (PDT) Received: from gum.cmpxchg.org (gum.cmpxchg.org. [85.214.110.215]) by mx.google.com with ESMTPS id m16si11583628wie.3.2015.03.26.04.24.50 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 26 Mar 2015 04:24:51 -0700 (PDT) Date: Thu, 26 Mar 2015 07:24:45 -0400 From: Johannes Weiner Subject: Re: [patch 08/12] mm: page_alloc: wait for OOM killer progress before retrying Message-ID: <20150326112445.GC18560@cmpxchg.org> References: <1427264236-17249-1-git-send-email-hannes@cmpxchg.org> <1427264236-17249-9-git-send-email-hannes@cmpxchg.org> <201503252315.FBJ09847.FSOtOJQFOMLFVH@I-love.SAKURA.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201503252315.FBJ09847.FSOtOJQFOMLFVH@I-love.SAKURA.ne.jp> Sender: owner-linux-mm@kvack.org List-ID: To: Tetsuo Handa Cc: linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, torvalds@linux-foundation.org, akpm@linux-foundation.org, ying.huang@intel.com, aarcange@redhat.com, david@fromorbit.com, mhocko@suse.cz, tytso@mit.edu On Wed, Mar 25, 2015 at 11:15:48PM +0900, Tetsuo Handa wrote: > Johannes Weiner wrote: > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > > index 5cfda39b3268..e066ac7353a4 100644 > > --- a/mm/oom_kill.c > > +++ b/mm/oom_kill.c > > @@ -711,12 +711,15 @@ bool out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask, > > killed = 1; > > } > > out: > > + if (test_thread_flag(TIF_MEMDIE)) > > + return true; > > /* > > - * Give the killed threads a good chance of exiting before trying to > > - * allocate memory again. > > + * Wait for any outstanding OOM victims to die. In rare cases > > + * victims can get stuck behind the allocating tasks, so the > > + * wait needs to be bounded. It's crude alright, but cheaper > > + * than keeping a global dependency tree between all tasks. > > */ > > - if (killed) > > - schedule_timeout_killable(1); > > + wait_event_timeout(oom_victims_wait, !atomic_read(&oom_victims), HZ); > > > > return true; > > } > > out_of_memory() returning true with bounded wait effectively means that > wait forever without choosing subsequent OOM victims when first OOM victim > failed to die. The system will lock up, won't it? The OOM killer already refuses to choose another victim as long as the first one hasn't exited, see oom_scan_process_thread(). That's why later patches in this series introduce a reserve for OOM-killing tasks and give nofail allocations access to emergency reserves, in case they themselves prevent that single OOM victim from exiting. But otherwise victims should be exiting eventually. > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index c1224ba45548..9ce9c4c083a0 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -2330,30 +2330,29 @@ void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...) > > } > > > > static inline struct page * > > -__alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, > > +__alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, int alloc_flags, > > const struct alloc_context *ac, unsigned long *did_some_progress) > > { > > - struct page *page; > > + struct page *page = NULL; > > > > *did_some_progress = 0; > > > > /* > > - * Acquire the oom lock. If that fails, somebody else is > > - * making progress for us. > > + * This allocating task can become the OOM victim itself at > > + * any point before acquiring the lock. In that case, exit > > + * quickly and don't block on the lock held by another task > > + * waiting for us to exit. > > */ > > - if (!mutex_trylock(&oom_lock)) { > > - *did_some_progress = 1; > > - schedule_timeout_uninterruptible(1); > > - return NULL; > > + if (test_thread_flag(TIF_MEMDIE) || mutex_lock_killable(&oom_lock)) { > > + alloc_flags |= ALLOC_NO_WATERMARKS; > > + goto alloc; > > } > > When a thread group has 1000 threads and most of them are doing memory allocation > request, all of them will get fatal_signal_pending() == true when one of them are > chosen by OOM killer. > This code will allow most of them to access memory reserves, won't it? Ah, good point! Only TIF_MEMDIE should get reserve access, not just any dying thread. Thanks, I'll fix it in v2. > > @@ -2383,12 +2382,20 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, > > if (gfp_mask & __GFP_THISNODE) > > goto out; > > } > > - /* Exhausted what can be done so it's blamo time */ > > - if (out_of_memory(ac->zonelist, gfp_mask, order, ac->nodemask, false) > > - || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) > > + > > + if (out_of_memory(ac->zonelist, gfp_mask, order, ac->nodemask, false)) { > > *did_some_progress = 1; > > + } else { > > + /* Oops, these shouldn't happen with the OOM killer disabled */ > > + if (WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) > > + *did_some_progress = 1; > > + } > > I think GFP_NOFAIL allocations need to involve OOM killer than > pretending as if forward progress is made. If all of in-flight > allocation requests are GFP_NOFAIL, the system will lock up. Hm? They do involve the OOM killer, but once userspace is frozen for suspend/hibernate we shouldn't kill and thaw random tasks anymore as that might corrupt the memory snapshot, so nofail allocations are a bug at this point. > After all, if we wait for OOM killer progress before retrying, I think > we should involve OOM killer after some bounded timeout regardless of > gfp flags, and let OOM killer kill more threads after another bounded > timeout. Otherwise, the corner cases will lock up the system. Giving nofail allocations access to emergency reserves targets this problem, but I agree with you that it's still possible for the system to lock up if they have been consumed and still no task made enough forward progress to release memory. It is unlikely but possible. I will probably come back to the OOM victim timeout patch some time in the future as that seems more robust. It would also drastically simplify memcg OOM handling. But that patch was controversial in the past and seemed beyond the scope of this patch set. -- 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: email@kvack.org