Re: [PATCH 02/30] mm: gfp_to_alloc_flags()

On Thursday July 24, a.p.zijlstra@chello.nl wrote:

Factor out the gfp to alloc_flags mapping so it can be used in other places.

Signed-off-by: Peter Zijlstra <redacted>
---
 mm/internal.h   |   10 +++++
 mm/page_alloc.c |   95 +++++++++++++++++++++++++++++++-------------------------
 2 files changed, 64 insertions(+), 41 deletions(-)

This patch all looks "obviously correct" and a nice factorisation of
code, except the last little bit:

I don't remember seeing it before (though my memory is imperfect) and
it doesn't seem to fit with the rest of the patch (except spatially).

There is a test above for PF_MEMALLOC which will result in a "goto"
somewhere else unless "in_interrupt()".
There is immediately above a test for "!wait".
So the only way this test can fire is when in_interrupt and wait.
But if that happens, then the
	might_sleep_if(wait)
at the top should have thrown a warning...  It really shouldn't happen.

So it looks like it is useless code:  there is already protection
against recursion in this case.

Did I miss something?
If I did, maybe more text in the changelog entry (or the comment)
would help.

Thanks,
NeilBrown

On Tue, 2008-08-12 at 15:01 +1000, Neil Brown wrote:

On Thursday July 24, a.p.zijlstra@chello.nl wrote:

quoted

Factor out the gfp to alloc_flags mapping so it can be used in other places.

Signed-off-by: Peter Zijlstra <redacted>
---
 mm/internal.h   |   10 +++++
 mm/page_alloc.c |   95 +++++++++++++++++++++++++++++++-------------------------
 2 files changed, 64 insertions(+), 41 deletions(-)

This patch all looks "obviously correct" and a nice factorisation of
code, except the last little bit:

quoted

@@ -1618,6 +1627,10 @@ nofail_alloc:
 	if (!wait)
 		goto nopage;
 
+	/* Avoid recursion of direct reclaim */
+	if (p->flags & PF_MEMALLOC)
+		goto nopage;
+
 	cond_resched();
 
 	/* We now go into synchronous reclaim */

-- 

I don't remember seeing it before (though my memory is imperfect) and
it doesn't seem to fit with the rest of the patch (except spatially).

There is a test above for PF_MEMALLOC which will result in a "goto"
somewhere else unless "in_interrupt()".
There is immediately above a test for "!wait".
So the only way this test can fire is when in_interrupt and wait.
But if that happens, then the
	might_sleep_if(wait)
at the top should have thrown a warning...  It really shouldn't happen.

So it looks like it is useless code:  there is already protection
against recursion in this case.

Did I miss something?
If I did, maybe more text in the changelog entry (or the comment)
would help.

Ok, so the old code did:

  if (((p->flags & PF_MEMALLOC) || ...) && !in_interrupt) {
    ....
    goto nopage;
  }

which avoid anything that has PF_MEMALLOC set from entering into direct
reclaim, right?

Now, the new code reads:

  if (alloc_flags & ALLOC_NO_WATERMARK) {
  }

Which might be false, even though we have PF_MEMALLOC set -
__GFP_NOMEMALLOC comes to mind.

So we have to stop that recursion from happening.

so we add:

  if (p->flags & PF_MEMALLOC)
    goto nopage;

Now, if it were done before the !wait check, we'd have to consider
atomic contexts, but as those are - as you rightly pointed out - handled
by the !wait case, we can plainly do this check.