From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 3 Apr 2018 14:08:31 +0200 From: Michal Hocko To: Matthew Wilcox Cc: Andrew Morton , Tetsuo Handa , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, Alexander Viro , "Kirill A. Shutemov" , Rik van Riel Subject: Re: [PATCH] mm: Check for SIGKILL inside dup_mmap() loop. Message-ID: <20180403120831.GT5501@dhcp22.suse.cz> References: <1522322870-4335-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp> <20180329143003.c52ada618be599c5358e8ca2@linux-foundation.org> <20180403115857.GC5832@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180403115857.GC5832@bombadil.infradead.org> Sender: owner-linux-mm@kvack.org List-ID: On Tue 03-04-18 04:58:57, Matthew Wilcox wrote: > On Thu, Mar 29, 2018 at 02:30:03PM -0700, Andrew Morton wrote: > > > @@ -440,6 +440,10 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm, > > > continue; > > > } > > > charge = 0; > > > + if (fatal_signal_pending(current)) { > > > + retval = -EINTR; > > > + goto out; > > > + } > > > if (mpnt->vm_flags & VM_ACCOUNT) { > > > unsigned long len = vma_pages(mpnt); > > > > I think a comment explaining why we're doing this would help. > > > > Better would be to add a new function "current_is_oom_killed()" or > > such, which becomes self-documenting. Because there are other reasons > > why a task may have a fatal signal pending. > > I disagree that we need a comment here, or to create an alias. Someone > who knows nothing of the oom-killer (like, er, me) reading that code sees > "Oh, we're checking for fatal signals here. I guess it doesn't make sense > to continue forking a process if it's already received a fatal signal." > > One might speculate about the causes of the fatal signal having been > received and settle on reasons which make sense even without thinking > of the OOM case. Because it's why it was introduced, I always think > about a task blocked on a dead NFS mount. If it's multithreaded and > one of the threads called fork() while another thread was blocked on a > page fault and the dup_mmap() had to wait for the page fault to finish > ... that would make some kind of sense. I completely agree. If the check is really correct then it should be pretty much self explanatory like many other checks. There is absolutely zero oom specific in there. If a check _is_ oom specific then there is something fishy going on. -- Michal Hocko SUSE Labs