From: Michal Hocko <mhocko@suse.cz> To: Johannes Weiner <hannes@cmpxchg.org> Cc: linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds <torvalds@linux-foundation.org>, Andrew Morton <akpm@linux-foundation.org>, Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>, Huang Ying <ying.huang@intel.com>, Andrea Arcangeli <aarcange@redhat.com>, Dave Chinner <david@fromorbit.com>, "Theodore Ts'o" <tytso@mit.edu> Subject: Re: [patch 04/12] mm: oom_kill: remove unnecessary locking in exit_oom_victim() Date: Thu, 26 Mar 2015 13:53:48 +0100 [thread overview] Message-ID: <20150326125348.GF15257@dhcp22.suse.cz> (raw) In-Reply-To: <1427264236-17249-5-git-send-email-hannes@cmpxchg.org> On Wed 25-03-15 02:17:08, Johannes Weiner wrote: > Disabling the OOM killer needs to exclude allocators from entering, > not existing victims from exiting. The idea was that exit_oom_victim doesn't miss a waiter. exit_oom_victim is doing atomic_dec_return(&oom_victims) && oom_killer_disabled) so there is a full (implicit) memory barrier befor oom_killer_disabled check. The other part is trickier. oom_killer_disable does: oom_killer_disabled = true; up_write(&oom_sem); wait_event(oom_victims_wait, !atomic_read(&oom_victims)); up_write doesn't guarantee a full memory barrier AFAICS in Documentation/memory-barriers.txt (although the generic and x86 implementations seem to implement it as a full barrier) but wait_event implies the full memory barrier (prepare_to_wait_event does spin lock&unlock) before checking the condition in the slow path. This should be sufficient and docummented... /* * We do not need to hold oom_sem here because oom_killer_disable * guarantees that oom_killer_disabled chage is visible before * the waiter is put into sleep (prepare_to_wait_event) so * we cannot miss a wake up. */ in unmark_oom_victim() > Right now the only waiter is suspend code, which achieves quiescence > by disabling the OOM killer. But later on we want to add waits that > hold the lock instead to stop new victims from showing up. It is not entirely clear what you mean by this from the current context. exit_oom_victim is not called from any context which would be locked by any OOM internals so it should be safe to use the locking. > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> I have nothing against the change as it seems correct but it would be good to get a better clarification and also document the implicit memory barriers. Acked-by: Michal Hocko <mhocko@suse.cz> > --- > mm/oom_kill.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 4b9547be9170..88aa9ba40fa5 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -437,14 +437,12 @@ void exit_oom_victim(void) > { > clear_thread_flag(TIF_MEMDIE); > > - down_read(&oom_sem); > /* > * There is no need to signal the lasst oom_victim if there > * is nobody who cares. > */ > if (!atomic_dec_return(&oom_victims) && oom_killer_disabled) > wake_up_all(&oom_victims_wait); > - up_read(&oom_sem); > } > > /** > -- > 2.3.3 > -- Michal Hocko SUSE Labs
WARNING: multiple messages have this Message-ID (diff)
From: Michal Hocko <mhocko@suse.cz> To: Johannes Weiner <hannes@cmpxchg.org> Cc: linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds <torvalds@linux-foundation.org>, Andrew Morton <akpm@linux-foundation.org>, Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>, Huang Ying <ying.huang@intel.com>, Andrea Arcangeli <aarcange@redhat.com>, Dave Chinner <david@fromorbit.com>, Theodore Ts'o <tytso@mit.edu> Subject: Re: [patch 04/12] mm: oom_kill: remove unnecessary locking in exit_oom_victim() Date: Thu, 26 Mar 2015 13:53:48 +0100 [thread overview] Message-ID: <20150326125348.GF15257@dhcp22.suse.cz> (raw) In-Reply-To: <1427264236-17249-5-git-send-email-hannes@cmpxchg.org> On Wed 25-03-15 02:17:08, Johannes Weiner wrote: > Disabling the OOM killer needs to exclude allocators from entering, > not existing victims from exiting. The idea was that exit_oom_victim doesn't miss a waiter. exit_oom_victim is doing atomic_dec_return(&oom_victims) && oom_killer_disabled) so there is a full (implicit) memory barrier befor oom_killer_disabled check. The other part is trickier. oom_killer_disable does: oom_killer_disabled = true; up_write(&oom_sem); wait_event(oom_victims_wait, !atomic_read(&oom_victims)); up_write doesn't guarantee a full memory barrier AFAICS in Documentation/memory-barriers.txt (although the generic and x86 implementations seem to implement it as a full barrier) but wait_event implies the full memory barrier (prepare_to_wait_event does spin lock&unlock) before checking the condition in the slow path. This should be sufficient and docummented... /* * We do not need to hold oom_sem here because oom_killer_disable * guarantees that oom_killer_disabled chage is visible before * the waiter is put into sleep (prepare_to_wait_event) so * we cannot miss a wake up. */ in unmark_oom_victim() > Right now the only waiter is suspend code, which achieves quiescence > by disabling the OOM killer. But later on we want to add waits that > hold the lock instead to stop new victims from showing up. It is not entirely clear what you mean by this from the current context. exit_oom_victim is not called from any context which would be locked by any OOM internals so it should be safe to use the locking. > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> I have nothing against the change as it seems correct but it would be good to get a better clarification and also document the implicit memory barriers. Acked-by: Michal Hocko <mhocko@suse.cz> > --- > mm/oom_kill.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 4b9547be9170..88aa9ba40fa5 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -437,14 +437,12 @@ void exit_oom_victim(void) > { > clear_thread_flag(TIF_MEMDIE); > > - down_read(&oom_sem); > /* > * There is no need to signal the lasst oom_victim if there > * is nobody who cares. > */ > if (!atomic_dec_return(&oom_victims) && oom_killer_disabled) > wake_up_all(&oom_victims_wait); > - up_read(&oom_sem); > } > > /** > -- > 2.3.3 > -- 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>
next prev parent reply other threads:[~2015-03-26 12:53 UTC|newest] Thread overview: 138+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-03-25 6:17 [patch 00/12] mm: page_alloc: improve OOM mechanism and policy Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-25 6:17 ` [patch 01/12] mm: oom_kill: remove unnecessary locking in oom_enable() Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-26 0:51 ` David Rientjes 2015-03-26 0:51 ` David Rientjes 2015-03-26 11:51 ` Michal Hocko 2015-03-26 11:51 ` Michal Hocko 2015-03-26 13:18 ` Michal Hocko 2015-03-26 13:18 ` Michal Hocko 2015-03-26 19:30 ` David Rientjes 2015-03-26 19:30 ` David Rientjes 2015-03-26 11:43 ` Michal Hocko 2015-03-26 11:43 ` Michal Hocko 2015-03-26 20:05 ` David Rientjes 2015-03-26 20:05 ` David Rientjes 2015-03-25 6:17 ` [patch 02/12] mm: oom_kill: clean up victim marking and exiting interfaces Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-26 3:34 ` David Rientjes 2015-03-26 3:34 ` David Rientjes 2015-03-26 11:54 ` Michal Hocko 2015-03-26 11:54 ` Michal Hocko 2015-03-25 6:17 ` [patch 03/12] mm: oom_kill: switch test-and-clear of known TIF_MEMDIE to clear Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-26 3:31 ` David Rientjes 2015-03-26 3:31 ` David Rientjes 2015-03-26 11:05 ` Johannes Weiner 2015-03-26 11:05 ` Johannes Weiner 2015-03-26 19:50 ` David Rientjes 2015-03-26 19:50 ` David Rientjes 2015-03-30 14:48 ` Michal Hocko 2015-03-30 14:48 ` Michal Hocko 2015-04-02 23:01 ` [patch] android, lmk: avoid setting TIF_MEMDIE if process has already exited David Rientjes 2015-04-02 23:01 ` David Rientjes 2015-04-28 22:50 ` [patch resend] " David Rientjes 2015-04-28 22:50 ` David Rientjes 2015-03-26 11:57 ` [patch 03/12] mm: oom_kill: switch test-and-clear of known TIF_MEMDIE to clear Michal Hocko 2015-03-26 11:57 ` Michal Hocko 2015-03-25 6:17 ` [patch 04/12] mm: oom_kill: remove unnecessary locking in exit_oom_victim() Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-26 12:53 ` Michal Hocko [this message] 2015-03-26 12:53 ` Michal Hocko 2015-03-26 13:01 ` Michal Hocko 2015-03-26 13:01 ` Michal Hocko 2015-03-26 15:10 ` Johannes Weiner 2015-03-26 15:10 ` Johannes Weiner 2015-03-26 15:04 ` Johannes Weiner 2015-03-26 15:04 ` Johannes Weiner 2015-03-25 6:17 ` [patch 05/12] mm: oom_kill: generalize OOM progress waitqueue Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-26 13:03 ` Michal Hocko 2015-03-26 13:03 ` Michal Hocko 2015-03-25 6:17 ` [patch 06/12] mm: oom_kill: simplify OOM killer locking Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-26 13:31 ` Michal Hocko 2015-03-26 13:31 ` Michal Hocko 2015-03-26 15:17 ` Johannes Weiner 2015-03-26 15:17 ` Johannes Weiner 2015-03-26 16:07 ` Michal Hocko 2015-03-26 16:07 ` Michal Hocko 2015-03-25 6:17 ` [patch 07/12] mm: page_alloc: inline should_alloc_retry() Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-26 14:11 ` Michal Hocko 2015-03-26 14:11 ` Michal Hocko 2015-03-26 15:18 ` Johannes Weiner 2015-03-26 15:18 ` Johannes Weiner 2015-03-25 6:17 ` [patch 08/12] mm: page_alloc: wait for OOM killer progress before retrying Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-25 14:15 ` Tetsuo Handa 2015-03-25 14:15 ` Tetsuo Handa 2015-03-25 17:01 ` Vlastimil Babka 2015-03-25 17:01 ` Vlastimil Babka 2015-03-26 11:28 ` Johannes Weiner 2015-03-26 11:28 ` Johannes Weiner 2015-03-26 11:24 ` Johannes Weiner 2015-03-26 11:24 ` Johannes Weiner 2015-03-26 14:32 ` Michal Hocko 2015-03-26 14:32 ` Michal Hocko 2015-03-26 15:23 ` Johannes Weiner 2015-03-26 15:23 ` Johannes Weiner 2015-03-26 15:38 ` Michal Hocko 2015-03-26 15:38 ` Michal Hocko 2015-03-26 18:17 ` Johannes Weiner 2015-03-26 18:17 ` Johannes Weiner 2015-03-27 14:01 ` [patch 08/12] mm: page_alloc: wait for OOM killer progressbefore retrying Tetsuo Handa 2015-03-27 14:01 ` Tetsuo Handa 2015-03-26 15:58 ` [patch 08/12] mm: page_alloc: wait for OOM killer progress before retrying Michal Hocko 2015-03-26 15:58 ` Michal Hocko 2015-03-26 18:23 ` Johannes Weiner 2015-03-26 18:23 ` Johannes Weiner 2015-03-25 6:17 ` [patch 09/12] mm: page_alloc: private memory reserves for OOM-killing allocations Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-04-14 16:49 ` Michal Hocko 2015-04-14 16:49 ` Michal Hocko 2015-04-24 19:13 ` Johannes Weiner 2015-04-24 19:13 ` Johannes Weiner 2015-03-25 6:17 ` [patch 10/12] mm: page_alloc: emergency reserve access for __GFP_NOFAIL allocations Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-04-14 16:55 ` Michal Hocko 2015-04-14 16:55 ` Michal Hocko 2015-03-25 6:17 ` [patch 11/12] mm: page_alloc: do not lock up GFP_NOFS allocations upon OOM Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-26 14:50 ` Michal Hocko 2015-03-26 14:50 ` Michal Hocko 2015-03-25 6:17 ` [patch 12/12] mm: page_alloc: do not lock up low-order " Johannes Weiner 2015-03-25 6:17 ` Johannes Weiner 2015-03-26 15:32 ` Michal Hocko 2015-03-26 15:32 ` Michal Hocko 2015-03-26 19:58 ` [patch 00/12] mm: page_alloc: improve OOM mechanism and policy Dave Chinner 2015-03-26 19:58 ` Dave Chinner 2015-03-27 15:05 ` Johannes Weiner 2015-03-27 15:05 ` Johannes Weiner 2015-03-30 0:32 ` Dave Chinner 2015-03-30 0:32 ` Dave Chinner 2015-03-30 19:31 ` Johannes Weiner 2015-03-30 19:31 ` Johannes Weiner 2015-04-01 15:19 ` Michal Hocko 2015-04-01 15:19 ` Michal Hocko 2015-04-01 21:39 ` Dave Chinner 2015-04-01 21:39 ` Dave Chinner 2015-04-02 7:29 ` Michal Hocko 2015-04-02 7:29 ` Michal Hocko 2015-04-07 14:18 ` Johannes Weiner 2015-04-07 14:18 ` Johannes Weiner 2015-04-11 7:29 ` Tetsuo Handa 2015-04-11 7:29 ` Tetsuo Handa 2015-04-13 12:49 ` Michal Hocko 2015-04-13 12:49 ` Michal Hocko 2015-04-13 12:46 ` Michal Hocko 2015-04-13 12:46 ` Michal Hocko 2015-04-14 0:11 ` Dave Chinner 2015-04-14 0:11 ` Dave Chinner 2015-04-14 7:20 ` Michal Hocko 2015-04-14 7:20 ` Michal Hocko 2015-04-14 10:36 ` Johannes Weiner 2015-04-14 10:36 ` Johannes Weiner 2015-04-14 14:23 ` Michal Hocko 2015-04-14 14:23 ` Michal Hocko
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=20150326125348.GF15257@dhcp22.suse.cz \ --to=mhocko@suse.cz \ --cc=aarcange@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=david@fromorbit.com \ --cc=hannes@cmpxchg.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=penguin-kernel@I-love.SAKURA.ne.jp \ --cc=torvalds@linux-foundation.org \ --cc=tytso@mit.edu \ --cc=ying.huang@intel.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.