Bug in __alloc_pages()?

Bug in __alloc_pages()?

[Matthew Dobson]

[-- Attachment #1: Type: text/plain, Size: 3590 bytes --]

While looking at some bugs related to OOM handling in 2.6, Martin Bligh and 
I noticed some order 0 page allocation failures from kswapd:

kswapd0: page allocation failure. order:0, mode:0x50
  [<c0147b92>] __alloc_pages+0x288/0x295
  [<c0147bb7>] __get_free_pages+0x18/0x24
  [<c014b2c4>] kmem_getpages+0x15/0x94
  [<c014c047>] cache_grow+0x154/0x299
  [<c014c399>] cache_alloc_refill+0x20d/0x23d
  [<c014c622>] kmem_cache_alloc+0x46/0x4c
  [<f885a4b0>] journal_alloc_journal_head+0x10/0x5d [jbd]
  [<f885a523>] journal_add_journal_head+0x1a/0xe1 [jbd]
  [<f8850be3>] journal_dirty_data+0x2e/0x3a5 [jbd]
  [<f8883400>] ext3_journal_dirty_data+0xc/0x2a [ext3]
  [<f888329a>] walk_page_buffers+0x62/0x87 [ext3]
  [<f888382d>] ext3_ordered_writepage+0xea/0x136 [ext3]
  [<f8883731>] journal_dirty_data_fn+0x0/0x12 [ext3]
  [<c014ec31>] pageout+0x83/0xc0
  [<c014ee80>] shrink_list+0x212/0x55f
  [<c014de86>] __pagevec_release+0x15/0x1d
  [<c014f400>] shrink_cache+0x233/0x4d5
  [<c014fee2>] shrink_zone+0x91/0x9c
  [<c01501e9>] balance_pgdat+0x15f/0x208
  [<c0150350>] kswapd+0xbe/0xc0
  [<c011e1ef>] autoremove_wake_function+0x0/0x2d
  [<c0308886>] ret_from_fork+0x6/0x20
  [<c011e1ef>] autoremove_wake_function+0x0/0x2d
  [<c0150292>] kswapd+0x0/0xc0
  [<c01041d5>] kernel_thread_helper+0x5/0xb

We decided that seemed odd, as kswapd should be able to get a page as long 
as there is even one page left in the system, since being a memory 
allocator task (PF_MEMALLOC) should exempt kswapd from any page watermark 
restrictions.  Digging into the code I found what looked like a bug that 
could potentially cause this situation to be far more common.

This chunk of code from __alloc_pages() demonstrates the problem:

	/* This allocation should allow future memory freeing. */
	if (((p->flags & PF_MEMALLOC) || unlikely(test_thread_flag(TIF_MEMDIE))) 
&& !in_interrupt()) {
		/* go through the zonelist yet again, ignoring mins */
		for (i = 0; (z = zones[i]) != NULL; i++) {
			if (!cpuset_zone_allowed(z))
				continue;
			page = buffered_rmqueue(z, order, gfp_mask);
			if (page)
				goto got_pg;
		}
		goto nopage;
	}

	/* Atomic allocations - we can't balance anything */
	if (!wait)
		goto nopage;

rebalance:
	cond_resched();

	/* We now go into synchronous reclaim */
	p->flags |= PF_MEMALLOC;
	reclaim_state.reclaimed_slab = 0;
	p->reclaim_state = &reclaim_state;

	did_some_progress = try_to_free_pages(zones, gfp_mask, order);

	p->reclaim_state = NULL;
	p->flags &= ~PF_MEMALLOC;

If, while the system is under memory pressure, something attempts to 
allocate a page from interrupt context while current == kswapd we will 
obviously fail the !in_interrupt() check and fall through.  If this 
allocation request was made with __GFP_WAIT set then we'll fall through the 
next !wait check.  We will then set the PF_MEMALLOC flag and set 
p->reclaim_state to point to __alloc_pages() local reclaim_state structure. 
  kswapd alread has it's own reclaim_state and already has PF_MEMALLOC set, 
which would then be lost when, after try_to_free_pages(), we 
unconditionally set the reclaim_state to NULL and turn off the PF_MEMALLOC 
flag.

I'm not 100% sure that this potential bug is even possible (ie: can we have 
an in_interrupt() page request that has __GFP_WAIT set?), or is the cause 
of the 0-order page allocation failures we see, but it does seem like 
potentially dangerous code.  I have attatched a patch (against 2.6.11-mm4) 
to check whether the current task has it's own reclaim_state or already has 
PF_MEMALLOC set and if so, no longer throws away this data.

-Matt

[-- Attachment #2: fix-__alloc_pages.patch --]
[-- Type: text/plain, Size: 1378 bytes --]

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.11-mm4/mm/page_alloc.c linux-2.6.11-mm4+fix-__alloc_pages/mm/page_alloc.c
--- linux-2.6.11-mm4/mm/page_alloc.c	2005-03-16 16:07:49.179230440 -0800
+++ linux-2.6.11-mm4+fix-__alloc_pages/mm/page_alloc.c	2005-03-16 16:09:54.019251872 -0800
@@ -867,13 +867,14 @@ __alloc_pages(unsigned int gfp_mask, uns
 	const int wait = gfp_mask & __GFP_WAIT;
 	struct zone **zones, *z;
 	struct page *page;
-	struct reclaim_state reclaim_state;
+	struct reclaim_state reclaim_state = { .reclaimed_slab = 0 };
 	struct task_struct *p = current;
 	int i;
 	int classzone_idx;
 	int do_retry;
 	int can_try_harder;
 	int did_some_progress;
+	int is_memalloc = 0, has_reclaim_state = 0;
 
 	might_sleep_if(wait);
 
@@ -957,14 +958,22 @@ rebalance:
 	cond_resched();
 
 	/* We now go into synchronous reclaim */
-	p->flags |= PF_MEMALLOC;
-	reclaim_state.reclaimed_slab = 0;
-	p->reclaim_state = &reclaim_state;
+	if (p->flags & PF_MEMALLOC)
+		is_memalloc = 1;
+	else
+		p->flags |= PF_MEMALLOC;
+
+	if (p->reclaim_state)
+		has_reclaim_state = 1;
+	else
+		p->reclaim_state = &reclaim_state;
 
 	did_some_progress = try_to_free_pages(zones, gfp_mask, order);
 
-	p->reclaim_state = NULL;
-	p->flags &= ~PF_MEMALLOC;
+	if (!has_reclaim_state)
+		p->reclaim_state = NULL;
+	if (!is_memalloc)
+		p->flags &= ~PF_MEMALLOC;
 
 	cond_resched();
 

Re: Bug in __alloc_pages()?

[Nick Piggin]

Matthew Dobson wrote:
> While looking at some bugs related to OOM handling in 2.6, Martin Bligh 
> and I noticed some order 0 page allocation failures from kswapd:
> 
> kswapd0: page allocation failure. order:0, mode:0x50
>  [<c0147b92>] __alloc_pages+0x288/0x295
>  [<c0147bb7>] __get_free_pages+0x18/0x24
>  [<c014b2c4>] kmem_getpages+0x15/0x94
>  [<c014c047>] cache_grow+0x154/0x299
>  [<c014c399>] cache_alloc_refill+0x20d/0x23d
>  [<c014c622>] kmem_cache_alloc+0x46/0x4c
>  [<f885a4b0>] journal_alloc_journal_head+0x10/0x5d [jbd]
>  [<f885a523>] journal_add_journal_head+0x1a/0xe1 [jbd]
>  [<f8850be3>] journal_dirty_data+0x2e/0x3a5 [jbd]
>  [<f8883400>] ext3_journal_dirty_data+0xc/0x2a [ext3]
>  [<f888329a>] walk_page_buffers+0x62/0x87 [ext3]
>  [<f888382d>] ext3_ordered_writepage+0xea/0x136 [ext3]
>  [<f8883731>] journal_dirty_data_fn+0x0/0x12 [ext3]
>  [<c014ec31>] pageout+0x83/0xc0
>  [<c014ee80>] shrink_list+0x212/0x55f
>  [<c014de86>] __pagevec_release+0x15/0x1d
>  [<c014f400>] shrink_cache+0x233/0x4d5
>  [<c014fee2>] shrink_zone+0x91/0x9c
>  [<c01501e9>] balance_pgdat+0x15f/0x208
>  [<c0150350>] kswapd+0xbe/0xc0
>  [<c011e1ef>] autoremove_wake_function+0x0/0x2d
>  [<c0308886>] ret_from_fork+0x6/0x20
>  [<c011e1ef>] autoremove_wake_function+0x0/0x2d
>  [<c0150292>] kswapd+0x0/0xc0
>  [<c01041d5>] kernel_thread_helper+0x5/0xb
> 
> We decided that seemed odd, as kswapd should be able to get a page as 
> long as there is even one page left in the system, since being a memory 
> allocator task (PF_MEMALLOC) should exempt kswapd from any page 
> watermark restrictions.  Digging into the code I found what looked like 
> a bug that could potentially cause this situation to be far more common.
> 
> This chunk of code from __alloc_pages() demonstrates the problem:
> 
>     /* This allocation should allow future memory freeing. */
>     if (((p->flags & PF_MEMALLOC) || 
> unlikely(test_thread_flag(TIF_MEMDIE))) && !in_interrupt()) {
>         /* go through the zonelist yet again, ignoring mins */
>         for (i = 0; (z = zones[i]) != NULL; i++) {
>             if (!cpuset_zone_allowed(z))
>                 continue;
>             page = buffered_rmqueue(z, order, gfp_mask);
>             if (page)
>                 goto got_pg;
>         }
>         goto nopage;
>     }
> 
>     /* Atomic allocations - we can't balance anything */
>     if (!wait)
>         goto nopage;
> 
> rebalance:
>     cond_resched();
> 
>     /* We now go into synchronous reclaim */
>     p->flags |= PF_MEMALLOC;
>     reclaim_state.reclaimed_slab = 0;
>     p->reclaim_state = &reclaim_state;
> 
>     did_some_progress = try_to_free_pages(zones, gfp_mask, order);
> 
>     p->reclaim_state = NULL;
>     p->flags &= ~PF_MEMALLOC;
> 
> If, while the system is under memory pressure, something attempts to 
> allocate a page from interrupt context while current == kswapd we will 
> obviously fail the !in_interrupt() check and fall through.  If this 
> allocation request was made with __GFP_WAIT set then we'll fall through 
> the next !wait check.  We will then set the PF_MEMALLOC flag and set 
> p->reclaim_state to point to __alloc_pages() local reclaim_state 
> structure.  kswapd alread has it's own reclaim_state and already has 
> PF_MEMALLOC set, which would then be lost when, after 
> try_to_free_pages(), we unconditionally set the reclaim_state to NULL 
> and turn off the PF_MEMALLOC flag.
> 
> I'm not 100% sure that this potential bug is even possible (ie: can we 
> have an in_interrupt() page request that has __GFP_WAIT set?), or is the 
> cause of the 0-order page allocation failures we see, but it does seem 
> like potentially dangerous code.  I have attatched a patch (against 
> 2.6.11-mm4) to check whether the current task has it's own reclaim_state 
> or already has PF_MEMALLOC set and if so, no longer throws away this data.
> 

I don't think in_interrupt allocations can have __GFP_WAIT set, so
this should probably be OK.

Nick

--
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:"aart@kvack.org"> aart@kvack.org </a>

Re: Bug in __alloc_pages()?

[Matthew Dobson]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

Nick Piggin wrote:
> Matthew Dobson wrote:
> 
>> While looking at some bugs related to OOM handling in 2.6, Martin 
>> Bligh and I noticed some order 0 page allocation failures from kswapd:
>>
 >> <snip>
>>
>> If, while the system is under memory pressure, something attempts to 
>> allocate a page from interrupt context while current == kswapd we will 
>> obviously fail the !in_interrupt() check and fall through.  If this 
>> allocation request was made with __GFP_WAIT set then we'll fall 
>> through the next !wait check.  We will then set the PF_MEMALLOC flag 
>> and set p->reclaim_state to point to __alloc_pages() local 
>> reclaim_state structure.  kswapd alread has it's own reclaim_state and 
>> already has PF_MEMALLOC set, which would then be lost when, after 
>> try_to_free_pages(), we unconditionally set the reclaim_state to NULL 
>> and turn off the PF_MEMALLOC flag.
>>
>> I'm not 100% sure that this potential bug is even possible (ie: can we 
>> have an in_interrupt() page request that has __GFP_WAIT set?), or is 
>> the cause of the 0-order page allocation failures we see, but it does 
>> seem like potentially dangerous code.  I have attatched a patch 
>> (against 2.6.11-mm4) to check whether the current task has it's own 
>> reclaim_state or already has PF_MEMALLOC set and if so, no longer 
>> throws away this data.
>>
> 
> I don't think in_interrupt allocations can have __GFP_WAIT set, so
> this should probably be OK.
> 
> Nick

Agreed.  It seems unlikely, but not entirely impossible.  All it would take 
is one sloppily coded driver, right?  How about this patch instead?

-Matt

[-- Attachment #2: fix-__alloc_pages.patch --]
[-- Type: text/plain, Size: 618 bytes --]

diff -Nurp --exclude-from=/home/mcd/.dontdiff linux-2.6.11-mm4/mm/page_alloc.c linux-2.6.11-mm4+fix-__alloc_pages/mm/page_alloc.c
--- linux-2.6.11-mm4/mm/page_alloc.c	2005-03-16 16:07:49.000000000 -0800
+++ linux-2.6.11-mm4+fix-__alloc_pages/mm/page_alloc.c	2005-03-18 14:10:27.433667720 -0800
@@ -957,8 +957,10 @@ rebalance:
 	cond_resched();
 
 	/* We now go into synchronous reclaim */
+	BUG_ON(p->flags & PF_MEMALLOC);
 	p->flags |= PF_MEMALLOC;
 	reclaim_state.reclaimed_slab = 0;
+	BUG_ON(p->reclaim_state);
 	p->reclaim_state = &reclaim_state;
 
 	did_some_progress = try_to_free_pages(zones, gfp_mask, order);

Re: Bug in __alloc_pages()?

[Nick Piggin]

Matthew Dobson wrote:

> 
> Agreed.  It seems unlikely, but not entirely impossible.  All it would 
> take is one sloppily coded driver, right?  How about this patch instead?
> 

Sure that would be fine with me. It kind of makes the logic
explicit, as Martin said.

Nick

--
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:"aart@kvack.org"> aart@kvack.org </a>