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
