mm: call into direct reclaim without PF_MEMALLOC set

mm: call into direct reclaim without PF_MEMALLOC set

[Peter Zijlstra]

PF_MEMALLOC keeps direct reclaim from recursing into itself, I noticed this
call to try_to_free_pages didn't set it thus opening the floodgates.

/me wonders why this never triggered...

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 fs/buffer.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Index: linux-2.6-git/fs/buffer.c
===================================================================
--- linux-2.6-git.orig/fs/buffer.c	2006-11-15 20:14:58.000000000 +0100
+++ linux-2.6-git/fs/buffer.c	2006-11-15 20:19:22.000000000 +0100
@@ -360,8 +360,18 @@ static void free_more_memory(void)
 
 	for_each_online_pgdat(pgdat) {
 		zones = pgdat->node_zonelists[gfp_zone(GFP_NOFS)].zones;
-		if (*zones)
+		if (*zones) {
+			struct task_struct *p = current;
+			struct reclaim_state reclaim_state;
+			reclaim_state.reclaim_slab = 0;
+			p->flags |= PF_MEMALLOC;
+			p->reclaim_state = &reclaim_state;
+
 			try_to_free_pages(zones, GFP_NOFS);
+
+			p->reclaim_state = NULL;
+			p->flags &= ~PF_MEMALLOC;
+		}
 	}
 }
 


--
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: mm: call into direct reclaim without PF_MEMALLOC set

[Andrew Morton]

On Wed, 15 Nov 2006 20:25:03 +0100
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> 
> PF_MEMALLOC keeps direct reclaim from recursing into itself, I noticed this
> call to try_to_free_pages didn't set it thus opening the floodgates.
> 

Fair enough, I guess.

The changelog needs work - I had to think about it too much.

So it prevents a single level of recursion into try_to_free_pages() in the
case where free_more_memory() is not being called by try_to_free_pages()
(ie: the usual case).

> /me wonders why this never triggered...

Nobody would notice if it did - just a bit more stack space.

For the extra level of recursion to happen we'd require
try_to_free_pages(GFP_NOFS) to perform some allocation with __GFP_WAIT set.
 There will be some such cases, but not many.

> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  fs/buffer.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6-git/fs/buffer.c
> ===================================================================
> --- linux-2.6-git.orig/fs/buffer.c	2006-11-15 20:14:58.000000000 +0100
> +++ linux-2.6-git/fs/buffer.c	2006-11-15 20:19:22.000000000 +0100
> @@ -360,8 +360,18 @@ static void free_more_memory(void)
>  
>  	for_each_online_pgdat(pgdat) {
>  		zones = pgdat->node_zonelists[gfp_zone(GFP_NOFS)].zones;
> -		if (*zones)
> +		if (*zones) {
> +			struct task_struct *p = current;
> +			struct reclaim_state reclaim_state;
> +			reclaim_state.reclaim_slab = 0;
> +			p->flags |= PF_MEMALLOC;
> +			p->reclaim_state = &reclaim_state;
> +
>  			try_to_free_pages(zones, GFP_NOFS);
> +
> +			p->reclaim_state = NULL;
> +			p->flags &= ~PF_MEMALLOC;
> +		}
>  	}

Consider

	alloc_pages
	->try_to_free_pages
	  ->writepage
	    ->alloc_page_buffers
	      ->free_more_memory

The caller has already set PF_MEMALLOC, but your new code goes and
incorrectly clears it.

And I don't think we need the reclaim_state here?  If we do, you'll need to
save the caller's copy locally and restore it, rather than unconditionally
unwiring it.


--
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] mm: call into direct reclaim without PF_MEMALLOC set

[Peter Zijlstra]

(Sorry about the dup Andrew, I noticed I hit the wrong reply button)

OK, so how about this?

No use running direct reclaim if we're already in there.

---

PF_MEMALLOC is also used to prevent recursion of direct reclaim.
However this invocation does not set PF_MEMALLOC nor checks it and
hence a can make it nest a single time. Either by reaching this
spot from reclaim and then calling it again or entering here and 
encountering a __GFP_WAIT alloc from within.

So check for PF_MEMALLOC and avoid a second invocation and otherwise
set PF_MEMALLOC.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 fs/buffer.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Index: linux-2.6-git/fs/buffer.c
===================================================================
--- linux-2.6-git.orig/fs/buffer.c	2006-11-15 20:32:14.000000000 +0100
+++ linux-2.6-git/fs/buffer.c	2006-11-15 21:52:05.000000000 +0100
@@ -360,8 +360,18 @@ static void free_more_memory(void)
 
 	for_each_online_pgdat(pgdat) {
 		zones = pgdat->node_zonelists[gfp_zone(GFP_NOFS)].zones;
-		if (*zones)
+		if (*zones && !(current->flags & PF_MEMALLOC)) {
+			struct task_struct *p = current;
+			struct reclaim_state reclaim_state = { 0 };
+
+			p->flags |= PF_MEMALLOC;
+			p->reclaim_state = &reclaim_state;
+
 			try_to_free_pages(zones, GFP_NOFS);
+
+			p->reclaim_state = NULL;
+			p->flags &= ~PF_MEMALLOC;
+		}
 	}
 }
 


--
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] mm: call into direct reclaim without PF_MEMALLOC set

[Andrew Morton]

On Wed, 15 Nov 2006 22:23:35 +0100
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> (Sorry about the dup Andrew, I noticed I hit the wrong reply button)
> 
> OK, so how about this?
> 
> No use running direct reclaim if we're already in there.
> 
> ---
> 
> PF_MEMALLOC is also used to prevent recursion of direct reclaim.
> However this invocation does not set PF_MEMALLOC nor checks it and
> hence a can make it nest a single time. Either by reaching this
> spot from reclaim and then calling it again or entering here and 
> encountering a __GFP_WAIT alloc from within.
> 
> So check for PF_MEMALLOC and avoid a second invocation and otherwise
> set PF_MEMALLOC.
> 
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
>  fs/buffer.c |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6-git/fs/buffer.c
> ===================================================================
> --- linux-2.6-git.orig/fs/buffer.c	2006-11-15 20:32:14.000000000 +0100
> +++ linux-2.6-git/fs/buffer.c	2006-11-15 21:52:05.000000000 +0100
> @@ -360,8 +360,18 @@ static void free_more_memory(void)
>  
>  	for_each_online_pgdat(pgdat) {
>  		zones = pgdat->node_zonelists[gfp_zone(GFP_NOFS)].zones;
> -		if (*zones)
> +		if (*zones && !(current->flags & PF_MEMALLOC)) {
> +			struct task_struct *p = current;
> +			struct reclaim_state reclaim_state = { 0 };
> +
> +			p->flags |= PF_MEMALLOC;
> +			p->reclaim_state = &reclaim_state;
> +
>  			try_to_free_pages(zones, GFP_NOFS);
> +
> +			p->reclaim_state = NULL;
> +			p->flags &= ~PF_MEMALLOC;
> +		}
>  	}
>  }
>  
spose so.  It assume that current->reclaim_state is NULL if !PF_MEMALLOC
which I guess is true.

But do we need to set current->reclaim_state at all in here?

--
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] mm: call into direct reclaim without PF_MEMALLOC set

[Peter Zijlstra]

On Wed, 2006-11-15 at 13:23 -0800, Andrew Morton wrote:

> spose so.  It assume that current->reclaim_state is NULL if !PF_MEMALLOC
> which I guess is true.
> 
> But do we need to set current->reclaim_state at all in here?

I did a quick grep before sending this out, and thought code assumed
current->reclaim_state was !NULL, however on closer inspection this
seems not so.

*sigh* another version - almost hitting the DaveJ barrier:
  revisions > LOC

---

PF_MEMALLOC is also used to prevent recursion of direct reclaim.
However this invocation does not set PF_MEMALLOC nor checks it and
hence a can make it nest a single time. Either by reaching this
spot from reclaim and then calling it again or entering here and 
encountering a __GFP_WAIT alloc from within.

So check for PF_MEMALLOC and avoid a second invocation and otherwise
set PF_MEMALLOC.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 fs/buffer.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-2.6-git/fs/buffer.c
===================================================================
--- linux-2.6-git.orig/fs/buffer.c	2006-11-15 20:32:14.000000000 +0100
+++ linux-2.6-git/fs/buffer.c	2006-11-15 22:28:43.000000000 +0100
@@ -360,8 +360,11 @@ static void free_more_memory(void)
 
 	for_each_online_pgdat(pgdat) {
 		zones = pgdat->node_zonelists[gfp_zone(GFP_NOFS)].zones;
-		if (*zones)
+		if (*zones && !(current->flags & PF_MEMALLOC)) {
+			current->flags |= PF_MEMALLOC;
 			try_to_free_pages(zones, GFP_NOFS);
+			current->flags &= ~PF_MEMALLOC;
+		}
 	}
 }
 


--
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] mm: call into direct reclaim without PF_MEMALLOC set

[Andrew Morton]

On Wed, 15 Nov 2006 22:32:58 +0100
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> +			current->flags |= PF_MEMALLOC;
>  			try_to_free_pages(zones, GFP_NOFS);
> +			current->flags &= ~PF_MEMALLOC;

Sometime, later, in a different patch, we might as well suck that into
try_to_free_pages() itself.   Along with nice comment explaining
what it means and WARN_ON(current->flags & PF_MEMALLOC).

--
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] mm: call into direct reclaim without PF_MEMALLOC set

[Arjan van de Ven]

On Wed, 2006-11-15 at 14:00 -0800, Andrew Morton wrote:
> On Wed, 15 Nov 2006 22:32:58 +0100
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> > +			current->flags |= PF_MEMALLOC;
> >  			try_to_free_pages(zones, GFP_NOFS);
> > +			current->flags &= ~PF_MEMALLOC;
> 
> Sometime, later, in a different patch, we might as well suck that into
> try_to_free_pages() itself.   Along with nice comment explaining
> what it means and WARN_ON(current->flags & PF_MEMALLOC).

also I've seen a few cases where this will break.
If you already *have* PF_MEMALLOC you'd lose it here; it's generally a
mistake to do so. It's a lot safer to save the old value and restore
it...

--
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] mm: call into direct reclaim without PF_MEMALLOC set

[Andrew Morton]

On Wed, 15 Nov 2006 23:12:19 +0100
Arjan van de Ven <arjan@fenrus.demon.nl> wrote:

> On Wed, 2006-11-15 at 14:00 -0800, Andrew Morton wrote:
> > On Wed, 15 Nov 2006 22:32:58 +0100
> > Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > 
> > > +			current->flags |= PF_MEMALLOC;
> > >  			try_to_free_pages(zones, GFP_NOFS);
> > > +			current->flags &= ~PF_MEMALLOC;
> > 
> > Sometime, later, in a different patch, we might as well suck that into
> > try_to_free_pages() itself.   Along with nice comment explaining
> > what it means and WARN_ON(current->flags & PF_MEMALLOC).
> 
> also I've seen a few cases where this will break.
> If you already *have* PF_MEMALLOC you'd lose it here; it's generally a
> mistake to do so. It's a lot safer to save the old value and restore
> it...

it does:

+		if (*zones && !(current->flags & PF_MEMALLOC)) {
+			current->flags |= PF_MEMALLOC;
 			try_to_free_pages(zones, GFP_NOFS);
+			current->flags &= ~PF_MEMALLOC;
+		}

--
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] mm: cleanup and document reclaim recursion

[Peter Zijlstra]

On Wed, 2006-11-15 at 14:00 -0800, Andrew Morton wrote:
> On Wed, 15 Nov 2006 22:32:58 +0100
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> > +			current->flags |= PF_MEMALLOC;
> >  			try_to_free_pages(zones, GFP_NOFS);
> > +			current->flags &= ~PF_MEMALLOC;
> 
> Sometime, later, in a different patch, we might as well suck that into
> try_to_free_pages() itself.   Along with nice comment explaining
> what it means and WARN_ON(current->flags & PF_MEMALLOC).

---

Cleanup and document the reclaim recursion avoiding properties of PF_MEMALLOC.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 fs/buffer.c     |    9 +++++----
 mm/page_alloc.c |    2 --
 mm/vmscan.c     |   15 +++++++++++++++
 3 files changed, 20 insertions(+), 6 deletions(-)

Index: linux-2.6-git/fs/buffer.c
===================================================================
--- linux-2.6-git.orig/fs/buffer.c	2006-11-16 10:28:13.000000000 +0100
+++ linux-2.6-git/fs/buffer.c	2006-11-16 10:37:32.000000000 +0100
@@ -358,13 +358,14 @@ static void free_more_memory(void)
 	wakeup_pdflush(1024);
 	yield();
 
+	/* We're already in reclaim */
+	if (current->flags & PF_MEMALLOC)
+		return;
+
 	for_each_online_pgdat(pgdat) {
 		zones = pgdat->node_zonelists[gfp_zone(GFP_NOFS)].zones;
-		if (*zones && !(current->flags & PF_MEMALLOC)) {
-			current->flags |= PF_MEMALLOC;
+		if (*zones)
 			try_to_free_pages(zones, GFP_NOFS);
-			current->flags &= ~PF_MEMALLOC;
-		}
 	}
 }
 
Index: linux-2.6-git/mm/page_alloc.c
===================================================================
--- linux-2.6-git.orig/mm/page_alloc.c	2006-11-16 10:28:13.000000000 +0100
+++ linux-2.6-git/mm/page_alloc.c	2006-11-16 10:37:32.000000000 +0100
@@ -1067,14 +1067,12 @@ rebalance:
 
 	/* We now go into synchronous reclaim */
 	cpuset_memory_pressure_bump();
-	p->flags |= PF_MEMALLOC;
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
 	did_some_progress = try_to_free_pages(zonelist->zones, gfp_mask);
 
 	p->reclaim_state = NULL;
-	p->flags &= ~PF_MEMALLOC;
 
 	cond_resched();
 
Index: linux-2.6-git/mm/vmscan.c
===================================================================
--- linux-2.6-git.orig/mm/vmscan.c	2006-11-16 10:28:13.000000000 +0100
+++ linux-2.6-git/mm/vmscan.c	2006-11-16 10:37:32.000000000 +0100
@@ -1030,6 +1030,20 @@ unsigned long try_to_free_pages(struct z
 
 	count_vm_event(ALLOCSTALL);
 
+	/*
+	 * PF_MEMALLOC also keeps direct reclaim from recursing into itself.
+	 * Any invocation of direct reclaim with PF_MEMALLOC set is therefore
+	 * invalid.
+	 *
+	 * This makes sense, in that PF_MEMALLOC results in ALLOC_NO_WATERMARKS
+	 * for allocations (except __GFP_NOMEMALLOC), which only makes sense
+	 * for reclaim (or reclaim aiding) contexts. So starting reclaim
+	 * from a context that either helps out reclaim or is reclaim doesn't
+	 * make sense.
+	 */
+	WARN_ON(current->flags & PF_MEMALLOC);
+	current->flags |= PF_MEMALLOC;
+
 	for (i = 0; zones[i] != NULL; i++) {
 		struct zone *zone = zones[i];
 
@@ -1093,6 +1107,7 @@ out:
 
 		zone->prev_priority = priority;
 	}
+	current->flags &= ~PF_MEMALLOC;
 	return ret;
 }
 


--
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] mm: cleanup and document reclaim recursion

[Andrew Morton]

On Thu, 16 Nov 2006 10:52:25 +0100
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Wed, 2006-11-15 at 14:00 -0800, Andrew Morton wrote:
> > On Wed, 15 Nov 2006 22:32:58 +0100
> > Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > 
> > > +			current->flags |= PF_MEMALLOC;
> > >  			try_to_free_pages(zones, GFP_NOFS);
> > > +			current->flags &= ~PF_MEMALLOC;
> > 
> > Sometime, later, in a different patch, we might as well suck that into
> > try_to_free_pages() itself.   Along with nice comment explaining
> > what it means and WARN_ON(current->flags & PF_MEMALLOC).
> 
> ---
> 
> Cleanup and document the reclaim recursion avoiding properties of PF_MEMALLOC.
> 
> ..

hmm.

>  
> +	/* We're already in reclaim */
> +	if (current->flags & PF_MEMALLOC)
> +		return;
> +

We're kinda dead if free_more_memory() does this.  It'll go into an
infinite loop.

Recurring back into try_to_free_pages() would actually be a better thing to
do..

Taking a nap might make some sense, not sure.

It all needs more thought, no?

--
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] mm: cleanup and document reclaim recursion

[Peter Zijlstra]

On Thu, 2006-11-16 at 16:16 -0800, Andrew Morton wrote:

> hmm.
> 
> >  
> > +	/* We're already in reclaim */
> > +	if (current->flags & PF_MEMALLOC)
> > +		return;
> > +
> 
> We're kinda dead if free_more_memory() does this.  It'll go into an
> infinite loop.
> 
> Recurring back into try_to_free_pages() would actually be a better thing to
> do..
> 
> Taking a nap might make some sense, not sure.
> 
> It all needs more thought, no?

I'm not much familiar with this filesystem thing, but I'll have me a
look, worst thing that can happen is that I learn something new
today :-)



--
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] mm: cleanup and document reclaim recursion

[Peter Zijlstra]

On Thu, 2006-11-16 at 16:16 -0800, Andrew Morton wrote:

> hmm.
> 
> >  
> > +	/* We're already in reclaim */
> > +	if (current->flags & PF_MEMALLOC)
> > +		return;
> > +
> 
> We're kinda dead if free_more_memory() does this.  It'll go into an
> infinite loop.

Yeah, this yield() might slow it down or not, but this direct claim
instance will indeed stall and busy wait for some other reclaimer to
free up memory. Which might only be kswapd() that also runs with
__GFP_FS and hence might deadlock?

> Recurring back into try_to_free_pages() would actually be a better thing to
> do..

*sigh*, it would be able to make progress due to the GFP_NOFS thing, but
gah ugly!

> Taking a nap might make some sense, not sure.

If we can deadlock because kswapd runs __GFP_FS then no, just a nap
won't do.

> It all needs more thought, no?

Yes, most definitely.

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