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

Neil Brown
2008-08-12T05:02:10Z

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 <a.p.zijlstra@chello.nl>
> ---
> 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:

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

Thanks,
NeilBrown