mm: call into direct reclaim without PF_MEMALLOC set

Peter Zijlstra

author

2006-11-15 19:25 UTC #0 lore raw

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 <[email protected]>

---
 fs/buffer.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

fs/buffer.c +11 -1

Index: linux-2.6-git/fs/buffer.c
===================================================================
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

2006-11-15 20:42 UTC #1 lore raw

Quoted 4 lines · On Wed, 15 Nov 2006 20:25:03 +0100 Peter Zijlstra <[email protected]>…

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

Quoted 1 line

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

Quoted 17 lines

Signed-off-by Peter Zijlstra <[email protected]>

---
 fs/buffer.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Index: linux-2.6-git/fs/buffer.c
===================================================================

fs/buffer.c +3 -1

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

fs/buffer.c static void free_more_memory(void) · line 375

Quoted 10 lines

fs/buffer.c +8 -0

+			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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Peter Zijlstra

author

2006-11-15 21:23 UTC #2 lore raw

PATCH 1/1 mm: call into direct reclaim without PF_MEMALLOC set

(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 <[email protected]>

---
 fs/buffer.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

fs/buffer.c +11 -1

Index: linux-2.6-git/fs/buffer.c
===================================================================
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

2006-11-15 21:32 UTC #3 lore raw

Quoted 36 lines · On Wed, 15 Nov 2006 22:23:35 +0100 Peter Zijlstra <[email protected]>…

(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 <[email protected]>
---
 fs/buffer.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
Index: linux-2.6-git/fs/buffer.c
===================================================================
fs/buffer.c +5 -1
@@ -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;

fs/buffer.c static void free_more_memory(void) · line 377

Quoted 10 lines

fs/buffer.c +6 -0

+			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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Peter Zijlstra

author

2006-11-15 21:32 UTC #6 lore raw

Quoted 4 lines · Andrew Morton

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 <[email protected]>

---
 fs/buffer.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/buffer.c +4 -1

Index: linux-2.6-git/fs/buffer.c
===================================================================
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

2006-11-15 22:00 UTC #7 lore raw

fs/buffer.c static void free_more_memory(void) · line 366

Quoted 3 lines · On Wed, 15 Nov 2006 22:32:58 +0100 Peter Zijlstra <[email protected]>…

fs/buffer.c +2 -0

+			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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Arjan van de Ven

2006-11-15 22:12 UTC #8 lore raw

Quoted 10 lines · Andrew Morton

fs/buffer.c static void free_more_memory(void) · line 366
Quoted 3 lines · On Wed, 15 Nov 2006 22:32:58 +0100 Peter Zijlstra <[email protected]>…
fs/buffer.c +2 -0
+			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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

2006-11-15 22:24 UTC #9 lore raw

Quoted 12 lines · On Wed, 15 Nov 2006 23:12:19 +0100 Arjan van de Ven <[email protected]>…

Quoted 10 lines · Andrew Morton
fs/buffer.c static void free_more_memory(void) · line 366
Quoted 3 lines · On Wed, 15 Nov 2006 22:32:58 +0100 Peter Zijlstra <[email protected]>…
fs/buffer.c +2 -0
+			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).

Quoted 4 lines

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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Peter Zijlstra

author

2006-11-16 09:52 UTC #10 lore raw

PATCH 1/1 mm: cleanup and document reclaim recursion

In reply to Andrew Morton

Quoted 10 lines · Andrew Morton

Review
Quoted 3 lines · On Wed, 15 Nov 2006 22:32:58 +0100 Peter Zijlstra <[email protected]>…
(quoted diff) +2 -0
+			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 <[email protected]>

---
 fs/buffer.c     |    9 +++++----
 mm/page_alloc.c |    2 --
 mm/vmscan.c     |   15 +++++++++++++++
 3 files changed, 20 insertions(+), 6 deletions(-)

fs/buffer.c +5 -4

Index: linux-2.6-git/fs/buffer.c
===================================================================
@@ -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;
-		}
 	}
 }

mm/page_alloc.c +0 -2

Index: linux-2.6-git/mm/page_alloc.c
===================================================================
@@ -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();

mm/vmscan.c +15 -0

Index: linux-2.6-git/mm/vmscan.c
===================================================================
@@ -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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

2006-11-17 00:16 UTC #11 lore raw

Re: [PATCH] mm: cleanup and document reclaim recursion

Quoted 12 lines · On Thu, 16 Nov 2006 10:52:25 +0100 Peter Zijlstra <[email protected]>…

Quoted 10 lines · Andrew Morton
Review
Quoted 3 lines · On Wed, 15 Nov 2006 22:32:58 +0100 Peter Zijlstra <[email protected]>…
(quoted diff) +2 -0
+			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).

Quoted 5 lines

---
Cleanup and document the reclaim recursion avoiding properties of PF_MEMALLOC.
..

hmm.

fs/buffer.c static void free_more_memory(void) · line 364

Quoted 5 lines

fs/buffer.c +4 -0

+	/* 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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Peter Zijlstra

author

2006-11-17 08:31 UTC #12 lore raw

Re: [PATCH] mm: cleanup and document reclaim recursion

Quoted 11 lines · Andrew Morton

hmm.
fs/buffer.c static void free_more_memory(void) · line 364
Quoted 5 lines
fs/buffer.c +4 -0
+	/* 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.

Quoted 6 lines

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 [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Peter Zijlstra

author

2006-11-17 12:18 UTC #13 lore raw

Re: [PATCH] mm: cleanup and document reclaim recursion

In reply to Andrew Morton

Quoted 10 lines · Andrew Morton

hmm.
fs/buffer.c static void free_more_memory(void) · line 364
Quoted 5 lines
fs/buffer.c +4 -0
+	/* 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?

Quoted 2 lines

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!

Quoted 1 line

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.

Quoted 1 line

It all needs more thought, no?

Yes, most definitely.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>