Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1387490 > unrolled thread

Re: [PATCH 15/28] mm, page_alloc: Move might_sleep_if check to the allocator slowpath

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 15/28] mm, page_alloc: Move might_sleep_if check to the  allocator slowpath Vlastimil Babka <vbabka@suse.cz> - 2016-04-26 15:50 +0200
    Re: [PATCH 15/28] mm, page_alloc: Move might_sleep_if check to the  allocator slowpath Mel Gorman <mgorman@techsingularity.net> - 2016-04-26 17:00 +0200
      Re: [PATCH 15/28] mm, page_alloc: Move might_sleep_if check to the  allocator slowpath Vlastimil Babka <vbabka@suse.cz> - 2016-04-26 17:20 +0200
        Re: [PATCH 15/28] mm, page_alloc: Move might_sleep_if check to the  allocator slowpath Mel Gorman <mgorman@techsingularity.net> - 2016-04-26 18:30 +0200

#1387490 — Re: [PATCH 15/28] mm, page_alloc: Move might_sleep_if check to the allocator slowpath

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.

> 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()?

> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> ---
>   mm/page_alloc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 21aaef6ddd7a..9ef2f4ab9ca5 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3176,6 +3176,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>   		return NULL;
>   	}
>
> +	might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
> +
>   	/*
>   	 * We also sanity check to catch abuse of atomic reserves being used by
>   	 * callers that are not in atomic context.
> @@ -3369,8 +3371,6 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
>
>   	lockdep_trace_alloc(gfp_mask);
>
> -	might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
> -
>   	if (should_fail_alloc_page(gfp_mask, order))
>   		return NULL;
>
>

[toc] | [next] | [standalone]

#1387577

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.

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

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1387612

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), but do so 
from the wrong context. Am I getting that wrong?

[toc] | [prev] | [next] | [standalone]

#1387674

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web