[patch 1/3] mm, oom: ensure memoryless node zonelist always includes zones

[patch 1/3] mm, oom: ensure memoryless node zonelist always includes zones

[David Rientjes]

With memoryless node support being worked on, it's possible that for 
optimizations that a node may not have a non-NULL zonelist.  When CONFIG_NUMA is 
enabled and node 0 is memoryless, this means the zonelist for first_online_node 
may become NULL.

The oom killer requires a zonelist that includes all memory zones for the sysrq 
trigger and pagefault out of memory handler.

Ensure that a non-NULL zonelist is always passed to the oom killer.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 drivers/tty/sysrq.c      |  2 +-
 include/linux/nodemask.h | 10 +++++++++-
 mm/oom_kill.c            |  2 +-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -355,7 +355,7 @@ static struct sysrq_key_op sysrq_term_op = {
 
 static void moom_callback(struct work_struct *ignored)
 {
-	out_of_memory(node_zonelist(first_online_node, GFP_KERNEL), GFP_KERNEL,
+	out_of_memory(node_zonelist(first_memory_node, GFP_KERNEL), GFP_KERNEL,
 		      0, NULL, true);
 }
 
diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
--- a/include/linux/nodemask.h
+++ b/include/linux/nodemask.h
@@ -430,7 +430,15 @@ static inline int num_node_state(enum node_states state)
 	for_each_node_mask((__node), node_states[__state])
 
 #define first_online_node	first_node(node_states[N_ONLINE])
-#define next_online_node(nid)	next_node((nid), node_states[N_ONLINE])
+#define first_memory_node	first_node(node_states[N_MEMORY])
+static inline int next_online_node(int nid)
+{
+	return next_node(nid, node_states[N_ONLINE]);
+}
+static inline int next_memory_node(int nid)
+{
+	return next_node(nid, node_states[N_MEMORY]);
+}
 
 extern int nr_node_ids;
 extern int nr_online_nodes;
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -694,7 +694,7 @@ void pagefault_out_of_memory(void)
 	if (mem_cgroup_oom_synchronize(true))
 		return;
 
-	zonelist = node_zonelist(first_online_node, GFP_KERNEL);
+	zonelist = node_zonelist(first_memory_node, GFP_KERNEL);
 	if (try_set_zonelist_oom(zonelist, GFP_KERNEL)) {
 		out_of_memory(NULL, 0, 0, NULL, false);
 		clear_zonelist_oom(zonelist, GFP_KERNEL);

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

[patch 2/3] mm, oom: remove unnecessary check for NULL zonelist

[David Rientjes]

If the pagefault handler is modified to pass a non-NULL zonelist then an 
unnecessary check for a NULL zonelist in constrained_alloc() can be removed.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/oom_kill.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -208,8 +208,6 @@ static enum oom_constraint constrained_alloc(struct zonelist *zonelist,
 	/* Default to all available memory */
 	*totalpages = totalram_pages + total_swap_pages;
 
-	if (!zonelist)
-		return CONSTRAINT_NONE;
 	/*
 	 * Reach here only when __GFP_NOFAIL is used. So, we should avoid
 	 * to kill current.We have to random task kill in this case.
@@ -696,7 +694,7 @@ void pagefault_out_of_memory(void)
 
 	zonelist = node_zonelist(first_memory_node, GFP_KERNEL);
 	if (try_set_zonelist_oom(zonelist, GFP_KERNEL)) {
-		out_of_memory(NULL, 0, 0, NULL, false);
+		out_of_memory(zonelist, 0, 0, NULL, false);
 		clear_zonelist_oom(zonelist, GFP_KERNEL);
 	}
 }

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

Re: [patch 2/3] mm, oom: remove unnecessary check for NULL zonelist

[Johannes Weiner]

On Wed, Jul 23, 2014 at 06:16:32PM -0700, David Rientjes wrote:
> If the pagefault handler is modified to pass a non-NULL zonelist then an 
> unnecessary check for a NULL zonelist in constrained_alloc() can be removed.
>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  mm/oom_kill.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -208,8 +208,6 @@ static enum oom_constraint constrained_alloc(struct zonelist *zonelist,
>  	/* Default to all available memory */
>  	*totalpages = totalram_pages + total_swap_pages;
>  
> -	if (!zonelist)
> -		return CONSTRAINT_NONE;
>  	/*
>  	 * Reach here only when __GFP_NOFAIL is used. So, we should avoid
>  	 * to kill current.We have to random task kill in this case.
> @@ -696,7 +694,7 @@ void pagefault_out_of_memory(void)
>  
>  	zonelist = node_zonelist(first_memory_node, GFP_KERNEL);
>  	if (try_set_zonelist_oom(zonelist, GFP_KERNEL)) {
> -		out_of_memory(NULL, 0, 0, NULL, false);
> +		out_of_memory(zonelist, 0, 0, NULL, false);

out_of_memory() wants the zonelist that was used during allocation,
not just the random first node's zonelist that's simply picked to
serialize page fault OOM kills system-wide.

This would even change how panic_on_oom behaves for page fault OOMs
(in a completely unpredictable way) if we get CONSTRAINED_CPUSET.

This change makes no sense to me.

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

Re: [patch 2/3] mm, oom: remove unnecessary check for NULL zonelist

[David Rientjes]

On Thu, 31 Jul 2014, Johannes Weiner wrote:

> out_of_memory() wants the zonelist that was used during allocation,
> not just the random first node's zonelist that's simply picked to
> serialize page fault OOM kills system-wide.
> 
> This would even change how panic_on_oom behaves for page fault OOMs
> (in a completely unpredictable way) if we get CONSTRAINED_CPUSET.
> 
> This change makes no sense to me.
> 

Allocations during fault will be constrained by the cpuset's mems, if we 
are oom then why would we panic when panic_on_oom == 1?

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

Re: [patch 2/3] mm, oom: remove unnecessary check for NULL zonelist

[Johannes Weiner]

On Fri, Aug 01, 2014 at 02:10:37AM -0700, David Rientjes wrote:
> On Thu, 31 Jul 2014, Johannes Weiner wrote:
> 
> > out_of_memory() wants the zonelist that was used during allocation,
> > not just the random first node's zonelist that's simply picked to
> > serialize page fault OOM kills system-wide.
> > 
> > This would even change how panic_on_oom behaves for page fault OOMs
> > (in a completely unpredictable way) if we get CONSTRAINED_CPUSET.
> > 
> > This change makes no sense to me.
> > 
> 
> Allocations during fault will be constrained by the cpuset's mems, if we 
> are oom then why would we panic when panic_on_oom == 1?

Can you please address the concerns I raised?

And please describe user-visible changes in the changelog.

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

Re: [patch 2/3] mm, oom: remove unnecessary check for NULL zonelist

[David Rientjes]

On Fri, 1 Aug 2014, Johannes Weiner wrote:

> > > out_of_memory() wants the zonelist that was used during allocation,
> > > not just the random first node's zonelist that's simply picked to
> > > serialize page fault OOM kills system-wide.
> > > 
> > > This would even change how panic_on_oom behaves for page fault OOMs
> > > (in a completely unpredictable way) if we get CONSTRAINED_CPUSET.
> > > 
> > > This change makes no sense to me.
> > > 
> > 
> > Allocations during fault will be constrained by the cpuset's mems, if we 
> > are oom then why would we panic when panic_on_oom == 1?
> 
> Can you please address the concerns I raised?
> 

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.

> And please describe user-visible changes in the changelog.
> 

Ok, Andrew please annotate the changelog for 
mm-oom-remove-unnecessary-check-for-null-zonelist.patch by including:

This also causes panic_on_oom == 1 to not panic the machine when the 
pagefault is constrained by the mems of current's cpuset.  That behavior 
agrees with the semantics of the sysctl in Documentation/sysctl/vm.txt.

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

Re: [patch 2/3] mm, oom: remove unnecessary check for NULL zonelist

[Johannes Weiner]

On Fri, Aug 01, 2014 at 02:42:19PM -0700, David Rientjes wrote:
> On Fri, 1 Aug 2014, Johannes Weiner wrote:
> 
> > > > out_of_memory() wants the zonelist that was used during allocation,
> > > > not just the random first node's zonelist that's simply picked to
> > > > serialize page fault OOM kills system-wide.
> > > > 
> > > > This would even change how panic_on_oom behaves for page fault OOMs
> > > > (in a completely unpredictable way) if we get CONSTRAINED_CPUSET.
> > > > 
> > > > This change makes no sense to me.
> > > > 
> > > 
> > > Allocations during fault will be constrained by the cpuset's mems, if we 
> > > are oom then why would we panic when panic_on_oom == 1?
> > 
> > Can you please address the concerns I raised?
> > 
> 
> 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.

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.

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.

But the much bigger problem is that you continue to fail to address
even basic feedback and instead consistently derail discussions with
unrelated drivel and circular arguments.  As long as you continue to
do that I don't think we should be merging any of your patches.

> > And please describe user-visible changes in the changelog.
> > 
> 
> Ok, Andrew please annotate the changelog for 
> mm-oom-remove-unnecessary-check-for-null-zonelist.patch by including:
> 
> This also causes panic_on_oom == 1 to not panic the machine when the 
> pagefault is constrained by the mems of current's cpuset.  That behavior 
> agrees with the semantics of the sysctl in Documentation/sysctl/vm.txt.

Great, now we have a cleanup patch with the side-effect of changing
user-visible behavior and introducing non-sensical code semantics.

Nacked-by: Johannes Weiner <hannes@cmpxchg.org>

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

Re: [patch 2/3] mm, oom: remove unnecessary check for NULL zonelist

[David Rientjes]

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>

[patch 3/3] mm, oom: rename zonelist locking functions

[David Rientjes]

try_set_zonelist_oom() and clear_zonelist_oom() are not named properly to imply 
that they require locking semantics to avoid out_of_memory() being reordered.

zone_scan_lock is required for both functions to ensure that there is proper 
locking synchronization.

Rename try_set_zonelist_oom() to oom_zonelist_trylock() and rename 
clear_zonelist_oom() to oom_zonelist_unlock() to imply there is proper locking 
semantics.

At the same time, convert oom_zonelist_trylock() to return bool instead of int 
since only success and failure are tested.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/oom.h |  4 ++--
 mm/oom_kill.c       | 30 +++++++++++++-----------------
 mm/page_alloc.c     |  6 +++---
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/include/linux/oom.h b/include/linux/oom.h
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -55,8 +55,8 @@ extern void oom_kill_process(struct task_struct *p, gfp_t gfp_mask, int order,
 			     struct mem_cgroup *memcg, nodemask_t *nodemask,
 			     const char *message);
 
-extern int try_set_zonelist_oom(struct zonelist *zonelist, gfp_t gfp_flags);
-extern void clear_zonelist_oom(struct zonelist *zonelist, gfp_t gfp_flags);
+extern bool oom_zonelist_trylock(struct zonelist *zonelist, gfp_t gfp_flags);
+extern void oom_zonelist_unlock(struct zonelist *zonelist, gfp_t gfp_flags);
 
 extern void check_panic_on_oom(enum oom_constraint constraint, gfp_t gfp_mask,
 			       int order, const nodemask_t *nodemask);
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -557,28 +557,25 @@ EXPORT_SYMBOL_GPL(unregister_oom_notifier);
  * if a parallel OOM killing is already taking place that includes a zone in
  * the zonelist.  Otherwise, locks all zones in the zonelist and returns 1.
  */
-int try_set_zonelist_oom(struct zonelist *zonelist, gfp_t gfp_mask)
+bool oom_zonelist_trylock(struct zonelist *zonelist, gfp_t gfp_mask)
 {
 	struct zoneref *z;
 	struct zone *zone;
-	int ret = 1;
+	bool ret = true;
 
 	spin_lock(&zone_scan_lock);
-	for_each_zone_zonelist(zone, z, zonelist, gfp_zone(gfp_mask)) {
+	for_each_zone_zonelist(zone, z, zonelist, gfp_zone(gfp_mask))
 		if (zone_is_oom_locked(zone)) {
-			ret = 0;
+			ret = false;
 			goto out;
 		}
-	}
 
-	for_each_zone_zonelist(zone, z, zonelist, gfp_zone(gfp_mask)) {
-		/*
-		 * Lock each zone in the zonelist under zone_scan_lock so a
-		 * parallel invocation of try_set_zonelist_oom() doesn't succeed
-		 * when it shouldn't.
-		 */
+	/*
+	 * Lock each zone in the zonelist under zone_scan_lock so a parallel
+	 * call to oom_zonelist_trylock() doesn't succeed when it shouldn't.
+	 */
+	for_each_zone_zonelist(zone, z, zonelist, gfp_zone(gfp_mask))
 		zone_set_flag(zone, ZONE_OOM_LOCKED);
-	}
 
 out:
 	spin_unlock(&zone_scan_lock);
@@ -590,15 +587,14 @@ out:
  * allocation attempts with zonelists containing them may now recall the OOM
  * killer, if necessary.
  */
-void clear_zonelist_oom(struct zonelist *zonelist, gfp_t gfp_mask)
+void oom_zonelist_unlock(struct zonelist *zonelist, gfp_t gfp_mask)
 {
 	struct zoneref *z;
 	struct zone *zone;
 
 	spin_lock(&zone_scan_lock);
-	for_each_zone_zonelist(zone, z, zonelist, gfp_zone(gfp_mask)) {
+	for_each_zone_zonelist(zone, z, zonelist, gfp_zone(gfp_mask))
 		zone_clear_flag(zone, ZONE_OOM_LOCKED);
-	}
 	spin_unlock(&zone_scan_lock);
 }
 
@@ -693,8 +689,8 @@ void pagefault_out_of_memory(void)
 		return;
 
 	zonelist = node_zonelist(first_memory_node, GFP_KERNEL);
-	if (try_set_zonelist_oom(zonelist, GFP_KERNEL)) {
+	if (oom_zonelist_trylock(zonelist, GFP_KERNEL)) {
 		out_of_memory(zonelist, 0, 0, NULL, false);
-		clear_zonelist_oom(zonelist, GFP_KERNEL);
+		oom_zonelist_unlock(zonelist, GFP_KERNEL);
 	}
 }
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2201,8 +2201,8 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
 {
 	struct page *page;
 
-	/* Acquire the OOM killer lock for the zones in zonelist */
-	if (!try_set_zonelist_oom(zonelist, gfp_mask)) {
+	/* Acquire the per-zone oom lock for each zone */
+	if (!oom_zonelist_trylock(zonelist, gfp_mask)) {
 		schedule_timeout_uninterruptible(1);
 		return NULL;
 	}
@@ -2240,7 +2240,7 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
 	out_of_memory(zonelist, gfp_mask, order, nodemask, false);
 
 out:
-	clear_zonelist_oom(zonelist, gfp_mask);
+	oom_zonelist_unlock(zonelist, gfp_mask);
 	return page;
 }
 

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