linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: David Rientjes <rientjes@google.com>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Rik van Riel <riel@redhat.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [patch 2/3] mm, oom: remove unnecessary check for NULL zonelist
Date: Mon, 4 Aug 2014 17:18:42 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.02.1408041710070.23228@chino.kir.corp.google.com> (raw)
In-Reply-To: <20140802181327.GL9952@cmpxchg.org>

On Sat, 2 Aug 2014, Johannes Weiner wrote:

> > I see one concern: that panic_on_oom == 1 will not trigger on pagefault 
> > when constrained by cpusets.  To address that, I'll state that, since 
> > cpuset-constrained allocations are the allocation context for pagefaults,
> > panic_on_oom == 1 should not trigger on pagefault when constrained by 
> > cpusets.
> 
> I expressed my concern pretty clearly above: out_of_memory() wants the
> zonelist that was used during the failed allocation, you are passing a
> non-sensical value in there that only happens to have the same type.
> 

It's certainly meaningful, the particular zonelist chosen isn't important 
because we don't care about the ordering and pagefaults are not going to 
be using __GFP_THISNODE.  In this context, we only need to pass a zonelist 
that includes all zones because constrained_alloc() tests if the 
allocation is cpuset-constrained based on the gfp flags.  We'll get 
CONSTRAINT_CPUSET in that case.

This is important because the behavior of panic_on_oom differs, as you 
pointed out, depending on the constraint.  pagefault_out_of_memory(), with 
my patch, will always get CONSTRAINT_CPUSET when needed and 
check_panic_on_oom() will behave correctly now for cpusets.

> We simply don't have the right information at the end of the page
> fault handler to respect constrained allocations.  Case in point:
> nodemask is unset from pagefault_out_of_memory(), so we still kill
> based on mempolicy even though check_panic_on_oom() says it wouldn't.
> 

That is, in fact, the only last bit of information we need in the 
pagefault handler to make correct decisions.  It's important, too, since 
if the vma of the faulting address is constrained by a mempolicy, we want 
to avoid needless killing a process that has a mempolicy with a disjoint 
set of nodes.

> The code change is not an adequate solution for the problem we have
> here and the changelog is an insult to everybody who wants to make
> sense of this from the git history later on.
> 

We can also address mempolicies by modifying the page fault handler and 
passing the vma and faulting address to make the correct panic_on_oom 
decisions but also filter processes that have mempolicies that consist 
solely of a disjoint set of nodes.  I'll post that patch series as well.

--
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>

  reply	other threads:[~2014-08-05  0:18 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-07-24  1:16 [patch 1/3] mm, oom: ensure memoryless node zonelist always includes zones David Rientjes
2014-07-24  1:16 ` [patch 2/3] mm, oom: remove unnecessary check for NULL zonelist David Rientjes
2014-07-31 15:26   ` Johannes Weiner
2014-08-01  9:10     ` David Rientjes
2014-08-01 13:34       ` Johannes Weiner
2014-08-01 21:42         ` David Rientjes
2014-08-02 18:13           ` Johannes Weiner
2014-08-05  0:18             ` David Rientjes [this message]
2014-07-24  1:16 ` [patch 3/3] mm, oom: rename zonelist locking functions David Rientjes

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=alpine.DEB.2.02.1408041710070.23228@chino.kir.corp.google.com \
    --to=rientjes@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=hannes@cmpxchg.org \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=riel@redhat.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: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox