netdev - Re: [PATCH 02/30] mm: gfp_to_alloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 Aug 2008 15:01:36 +1000
From:	Neil Brown <neilb@...e.de>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	netdev@...r.kernel.org, trond.myklebust@....uio.no,
	Daniel Lezcano <dlezcano@...ibm.com>,
	Pekka Enberg <penberg@...helsinki.fi>
Subject: Re: [PATCH 02/30] mm: gfp_to_alloc_flags()

On Thursday July 24, a.p.zijlstra@...llo.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@...llo.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html