linux-kernel - Re: [PATCH 15/28] mm, page_alloc: Move might_sleep

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]

Message-ID: <20160426162928.GE2858@techsingularity.net>
Date:	Tue, 26 Apr 2016 17:29:28 +0100
From:	Mel Gorman <mgorman@...hsingularity.net>
To:	Vlastimil Babka <vbabka@...e.cz>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Jesper Dangaard Brouer <brouer@...hat.com>,
	Linux-MM <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 15/28] mm, page_alloc: Move might_sleep_if check to the
 allocator slowpath

On Tue, Apr 26, 2016 at 05:16:21PM +0200, Vlastimil Babka wrote:
> On 04/26/2016 04:50 PM, Mel Gorman wrote:
> >On Tue, Apr 26, 2016 at 03:41:22PM +0200, Vlastimil Babka wrote:
> >>On 04/15/2016 11:07 AM, Mel Gorman wrote:
> >>>There is a debugging check for callers that specify __GFP_DIRECT_RECLAIM
> >>>from a context that cannot sleep. Triggering this is almost certainly
> >>>a bug but it's also overhead in the fast path.
> >>
> >>For CONFIG_DEBUG_ATOMIC_SLEEP, enabling is asking for the overhead. But for
> >>CONFIG_PREEMPT_VOLUNTARY which turns it into _cond_resched(), I guess it's
> >>not.
> >>
> >
> >Either way, it struck me as odd. It does depend on the config and it's
> >marginal so if there is a problem then I can drop it.
> 
> What I tried to say is that it makes sense, but it's perhaps non-obvious :)
> 
> >>>Move the check to the slow
> >>>path. It'll be harder to trigger as it'll only be checked when watermarks
> >>>are depleted but it'll also only be checked in a path that can sleep.
> >>
> >>Hmm what about zone_reclaim_mode=1, should the check be also duplicated to
> >>that part of get_page_from_freelist()?
> >>
> >
> >zone_reclaim has a !gfpflags_allow_blocking() check, does not call
> >cond_resched() before that check so it does not fall into an accidental
> >sleep path. I'm not seeing why the check is necessary there.
> 
> Hmm I thought the primary purpose of this might_sleep_if() is to catch those
> (via the DEBUG_ATOMIC_SLEEP) that do pass __GFP_DIRECT_RECLAIM (which means
> gfpflags_allow_blocking() will be true and zone_reclaim will proceed),

It proceeds but fails immediately so what I'm failing to see is why
moving the check increases risk. I wanted to remove the check from the
path where the problem it's catching cannot happen. It does mean the
debugging check is made less frequently but it's still useful. If you
feel the safety is preferred then I'll drop the patch.

-- 
Mel Gorman
SUSE Labs