mm: couple rcu and memory reclaim

Just an idea I had, it seems like a good idea to wait for RCU callbacks in reclaim so that we won't get all of memory stuck there.

If this location is too aggressive we might stick it next to disable_swap_token().

---
Couple RCU and reclaim.

There could be a lot of memory stuck in RCU callbacks. Wait for RCU to finish before giving it another go.

Placed in kswapd and not direct reclaim path because kswapd never holds rcu_read_lock() at this point and can thus not deadlock. Direct reclaim callers might hold rcu_read_lock() and would suffer from deadlocks if sync_rcu() were to be called.

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

---
 mm/vmscan.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6/mm/vmscan.c
===================================================================

mm/vmscan.c +1 -1

 			disable_swap_token();
+		}
 
 		all_zones_ok = 1;
 
-

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 30 lines · Peter Zijlstra

Just an idea I had, it seems like a good idea to wait for RCU callbacks in reclaim so that we won't get all of memory stuck there.

If this location is too aggressive we might stick it next to disable_swap_token().

---
Couple RCU and reclaim.

There could be a lot of memory stuck in RCU callbacks. Wait for RCU to finish before giving it another go.

Placed in kswapd and not direct reclaim path because kswapd never holds rcu_read_lock() at this point and can thus not deadlock. Direct reclaim callers might hold rcu_read_lock() and would suffer from deadlocks if sync_rcu() were to be called.

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

---
 mm/vmscan.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6/mm/vmscan.c
===================================================================

mm/vmscan.c +0 -0

@@ -1435,8 +1435,10 @@ loop_again:
 		unsigned long lru_pages = 0;
 

mm/vmscan.c loop_again: · line 1439

Quoted 4 lines

mm/vmscan.c +2 -1

 		/* The swap token gets in the way of swapout... */
-		if (!priority)
+		if (!priority) {
+			synchronize_rcu();

Interesting change

1. I suspect that synchronize_rcu() is most likely to free up slab pages, so shrink_slab() will clean up all the freed pages. Could we add a comment to indicate the same?

2. Shouldn't we do this in balance_pgdat() as well?

Quoted 30 lines · On Mon, 24 Sep 2007 16:12:19 +0530 Balbir Singh <[email protected]>…

Quoted 29 lines · Peter Zijlstra

Just an idea I had, it seems like a good idea to wait for RCU callbacks in reclaim so that we won't get all of memory stuck there.

If this location is too aggressive we might stick it next to disable_swap_token().

---
Couple RCU and reclaim.

There could be a lot of memory stuck in RCU callbacks. Wait for RCU to finish before giving it another go.

Placed in kswapd and not direct reclaim path because kswapd never holds rcu_read_lock() at this point and can thus not deadlock. Direct reclaim callers might hold rcu_read_lock() and would suffer from deadlocks if sync_rcu() were to be called.

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

---
 mm/vmscan.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6/mm/vmscan.c
===================================================================

mm/vmscan.c +0 -0

@@ -1435,8 +1435,10 @@ loop_again:
 		unsigned long lru_pages = 0;
 

Quoted 4 lines

mm/vmscan.c loop_again: · line 1439

Quoted 4 lines

mm/vmscan.c +2 -1

 		/* The swap token gets in the way of swapout... */
-		if (!priority)
+		if (!priority) {
+			synchronize_rcu();

Bah, it seems I send the wrong patch out :-/

this is the one against disable_swap_token(). I meant to send out this one:

(quoted diff) +3 -1

@@ -1527,8 +1527,10 @@ loop_again:
                 * OK, kswapd is getting into trouble.  Take a nap, then take
                 * another pass across the zones.
                 */
-               if (total_scanned && priority < DEF_PRIORITY - 2)
+               if (total_scanned && priority < DEF_PRIORITY - 2) {
+                       synchronize_rcu();
                        congestion_wait(WRITE, HZ/10);
+               }
 
 

Quoted 5 lines

Interesting change

1. I suspect that synchronize_rcu() is most likely to free up slab pages, so shrink_slab() will clean up all the freed pages. Could we add a comment to indicate the same?

Yes indeed, will add such a comment.

Quoted 1 line

2. Shouldn't we do this in balance_pgdat() as well?

Uhm, this is balance_pgdat() (both these changes) :-)

Only kswapd can do this, direct reclaim has deadlock potential.

Quoted 16 lines · Peter Zijlstra

Quoted 13 lines · On Mon, 24 Sep 2007 16:12:19 +0530 Balbir Singh <[email protected]>…

Quoted 12 lines · Peter Zijlstra

Just an idea I had, it seems like a good idea to wait for RCU callbacks in reclaim so that we won't get all of memory stuck there.

If this location is too aggressive we might stick it next to disable_swap_token().

---
Couple RCU and reclaim.

There could be a lot of memory stuck in RCU callbacks. Wait for RCU to finish before giving it another go.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Placed in kswapd and not direct reclaim path because kswapd never holds rcu_read_lock() at this point and can thus not deadlock. Direct reclaim callers might hold rcu_read_lock() and would suffer from deadlocks if sync_rcu() were to be called.

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

---

Quoted 1 line

Only kswapd can do this, direct reclaim has deadlock potential.

Yes, but not in all cases, do you want to add any gfp_mask based smartness for direct reclaim?

Quoted 3 lines

Quoted 1 line

Only kswapd can do this, direct reclaim has deadlock potential.

Yes, but not in all cases, do you want to add any gfp_mask based smartness for direct reclaim?

gfp_mask doesn't carry the needed information. It depends on whether the current context holds a rcu_read_lock().

so something like:

rcu_read_lock()
foo = kmalloc(sizeof(foo))

      new_slab()
        __alloc_pages()
          try_to_free_pages()
            synchronise_rcu() <-- deadlock

rcu_read_unlock()

Quoted 7 lines · Peter Zijlstra

Quoted 3 lines

Quoted 1 line

Only kswapd can do this, direct reclaim has deadlock potential.

Yes, but not in all cases, do you want to add any gfp_mask based smartness for direct reclaim?

gfp_mask doesn't carry the needed information. It depends on whether the current context holds a rcu_read_lock().

What I meant was that nobody would hold rcu_read_lock() and pass gfp_mask of GFP_KERNEL in scan_control or to do_try_to_free_pages()

Quoted 4 lines

so something like:

rcu_read_lock()
foo = kmalloc(sizeof(foo))

At this point, you really can't use GFP_KERNEL, since rcu_read_lock() disables pre-emption in the current kernel, ideally you should see a might_sleep() BUG.

Quoted 5 lines

      new_slab()
        __alloc_pages()
          try_to_free_pages()
            synchronise_rcu() <-- deadlock

rcu_read_unlock()

Just an idea I had, it seems like a good idea to wait for RCU callbacks in reclaim so that we won't get all of memory stuck there.

If this location is too aggressive we might stick it next to disable_swap_token().