From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752833AbbCZNSm (ORCPT ); Thu, 26 Mar 2015 09:18:42 -0400 Received: from cantor2.suse.de ([195.135.220.15]:45814 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751797AbbCZNSl (ORCPT ); Thu, 26 Mar 2015 09:18:41 -0400 Date: Thu, 26 Mar 2015 14:18:39 +0100 From: Michal Hocko To: David Rientjes Cc: Johannes Weiner , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Linus Torvalds , Andrew Morton , Tetsuo Handa , Huang Ying , Andrea Arcangeli , Dave Chinner , "Theodore Ts'o" Subject: Re: [patch 01/12] mm: oom_kill: remove unnecessary locking in oom_enable() Message-ID: <20150326131839.GI15257@dhcp22.suse.cz> References: <1427264236-17249-1-git-send-email-hannes@cmpxchg.org> <1427264236-17249-2-git-send-email-hannes@cmpxchg.org> <20150326115140.GC15257@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150326115140.GC15257@dhcp22.suse.cz> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 26-03-15 12:51:40, Michal Hocko wrote: > On Wed 25-03-15 17:51:31, David Rientjes wrote: > > On Wed, 25 Mar 2015, Johannes Weiner wrote: > > > > > Setting oom_killer_disabled to false is atomic, there is no need for > > > further synchronization with ongoing allocations trying to OOM-kill. > > > > > > Signed-off-by: Johannes Weiner > > > --- > > > mm/oom_kill.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > > > index 2b665da1b3c9..73763e489e86 100644 > > > --- a/mm/oom_kill.c > > > +++ b/mm/oom_kill.c > > > @@ -488,9 +488,7 @@ bool oom_killer_disable(void) > > > */ > > > void oom_killer_enable(void) > > > { > > > - down_write(&oom_sem); > > > oom_killer_disabled = false; > > > - up_write(&oom_sem); > > > } > > > > > > #define K(x) ((x) << (PAGE_SHIFT-10)) > > > > I haven't looked through the new disable-oom-killer-for-pm patchset that > > was merged, but this oom_killer_disabled thing already looks improperly > > handled. I think any correctness or cleanups in this area would be very > > helpful. > > > > I think mark_tsk_oom_victim() in mem_cgroup_out_of_memory() is just > > luckily not racing with a call to oom_killer_enable() and triggering the > ^^^^^^^^^^ > oom_killer_disable? > > > WARN_ON(oom_killer_disabled) since there's no "oom_sem" held here, and > > it's an improper context based on the comment of mark_tsk_oom_victim(). > > OOM killer is disabled only _after_ all user tasks have been frozen. So > we cannot get any page fault and a race. So the semaphore is not needed > in this path although the comment says otherwise. I can add a comment > clarifying this... I am wrong here! pagefault_out_of_memory takes the lock and so the whole mem_cgroup_out_of_memory is called under the same lock. -- Michal Hocko SUSE Labs