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

Groups > linux.kernel > #1409833 > unrolled thread

Re: [RFC 02/13] mm, page_alloc: set alloc_flags only once in 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: [RFC 02/13] mm, page_alloc: set alloc_flags only once in slowpath Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-31 08:30 +0200
    Re: [RFC 02/13] mm, page_alloc: set alloc_flags only once in slowpath Vlastimil Babka <vbabka@suse.cz> - 2016-05-31 10:00 +0200
      Re: [RFC 02/13] mm, page_alloc: set alloc_flags only once in slowpath Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-06-02 03:50 +0200

#1409833 — Re: [RFC 02/13] mm, page_alloc: set alloc_flags only once in slowpath

On Tue, May 10, 2016 at 02:30:11PM +0200, Vlastimil Babka wrote:
> On 05/10/2016 01:28 PM, Tetsuo Handa wrote:
> > Vlastimil Babka wrote:
> >> In __alloc_pages_slowpath(), alloc_flags doesn't change after it's initialized,
> >> so move the initialization above the retry: label. Also make the comment above
> >> the initialization more descriptive.
> > 
> > Not true. gfp_to_alloc_flags() will include ALLOC_NO_WATERMARKS if current
> > thread got TIF_MEMDIE after gfp_to_alloc_flags() was called for the first
> 
> Oh, right. Stupid global state.
> 
> > time. Do you want to make TIF_MEMDIE threads fail their allocations without
> > using memory reserves?
> 
> No, thanks for catching this. How about the following version? I think
> that's even nicer cleanup, if correct. Note it causes a conflict in
> patch 03/13 but it's simple to resolve.
> 
> Thanks
> 
> ----8<----
> >From 68f09f1d4381c7451238b4575557580380d8bf30 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Fri, 29 Apr 2016 11:51:17 +0200
> Subject: [RFC 02/13] mm, page_alloc: set alloc_flags only once in slowpath
> 
> In __alloc_pages_slowpath(), alloc_flags doesn't change after it's initialized,
> so move the initialization above the retry: label. Also make the comment above
> the initialization more descriptive.
> 
> The only exception in the alloc_flags being constant is ALLOC_NO_WATERMARKS,
> which may change due to TIF_MEMDIE being set on the allocating thread. We can
> fix this, and make the code simpler and a bit more effective at the same time,
> by moving the part that determines ALLOC_NO_WATERMARKS from
> gfp_to_alloc_flags() to gfp_pfmemalloc_allowed(). This means we don't have to
> mask out ALLOC_NO_WATERMARKS in several places in __alloc_pages_slowpath()
> anymore.  The only test for the flag can instead call gfp_pfmemalloc_allowed().

Your patch looks correct to me but it makes me wonder something.
Why do we need to mask out ALLOC_NO_WATERMARKS in several places? If
some requestors have ALLOC_NO_WATERMARKS flag, he will
eventually do ALLOC_NO_WATERMARKS allocation in retry loop. I don't
understand what's the merit of masking out it.

Thanks.

[toc] | [next] | [standalone]

#1409948

On 05/31/2016 08:20 AM, Joonsoo Kim wrote:
>> >From 68f09f1d4381c7451238b4575557580380d8bf30 Mon Sep 17 00:00:00 2001
>> From: Vlastimil Babka <vbabka@suse.cz>
>> Date: Fri, 29 Apr 2016 11:51:17 +0200
>> Subject: [RFC 02/13] mm, page_alloc: set alloc_flags only once in slowpath
>>
>> In __alloc_pages_slowpath(), alloc_flags doesn't change after it's initialized,
>> so move the initialization above the retry: label. Also make the comment above
>> the initialization more descriptive.
>>
>> The only exception in the alloc_flags being constant is ALLOC_NO_WATERMARKS,
>> which may change due to TIF_MEMDIE being set on the allocating thread. We can
>> fix this, and make the code simpler and a bit more effective at the same time,
>> by moving the part that determines ALLOC_NO_WATERMARKS from
>> gfp_to_alloc_flags() to gfp_pfmemalloc_allowed(). This means we don't have to
>> mask out ALLOC_NO_WATERMARKS in several places in __alloc_pages_slowpath()
>> anymore.  The only test for the flag can instead call gfp_pfmemalloc_allowed().
>
> Your patch looks correct to me but it makes me wonder something.
> Why do we need to mask out ALLOC_NO_WATERMARKS in several places? If
> some requestors have ALLOC_NO_WATERMARKS flag, he will
> eventually do ALLOC_NO_WATERMARKS allocation in retry loop. I don't
> understand what's the merit of masking out it.

I can think of a reason. If e.g. reclaim makes free pages above 
watermark in the 4th zone in the zonelist, we would like the subsequent 
get_page_from_freelist() to succeed in that 4th zone. Passing 
ALLOC_NO_WATERMARKS there would likely succeed in the first zone, 
needlessly below the watermark.

But this actually makes no difference, since the ALLOC_NO_WATERMARKS 
attempt precedes reclaim/compaction attempts. It probably shouldn't...

> Thanks.
>

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

#1411751

On Tue, May 31, 2016 at 09:59:36AM +0200, Vlastimil Babka wrote:
> On 05/31/2016 08:20 AM, Joonsoo Kim wrote:
> >>>From 68f09f1d4381c7451238b4575557580380d8bf30 Mon Sep 17 00:00:00 2001
> >>From: Vlastimil Babka <vbabka@suse.cz>
> >>Date: Fri, 29 Apr 2016 11:51:17 +0200
> >>Subject: [RFC 02/13] mm, page_alloc: set alloc_flags only once in slowpath
> >>
> >>In __alloc_pages_slowpath(), alloc_flags doesn't change after it's initialized,
> >>so move the initialization above the retry: label. Also make the comment above
> >>the initialization more descriptive.
> >>
> >>The only exception in the alloc_flags being constant is ALLOC_NO_WATERMARKS,
> >>which may change due to TIF_MEMDIE being set on the allocating thread. We can
> >>fix this, and make the code simpler and a bit more effective at the same time,
> >>by moving the part that determines ALLOC_NO_WATERMARKS from
> >>gfp_to_alloc_flags() to gfp_pfmemalloc_allowed(). This means we don't have to
> >>mask out ALLOC_NO_WATERMARKS in several places in __alloc_pages_slowpath()
> >>anymore.  The only test for the flag can instead call gfp_pfmemalloc_allowed().
> >
> >Your patch looks correct to me but it makes me wonder something.
> >Why do we need to mask out ALLOC_NO_WATERMARKS in several places? If
> >some requestors have ALLOC_NO_WATERMARKS flag, he will
> >eventually do ALLOC_NO_WATERMARKS allocation in retry loop. I don't
> >understand what's the merit of masking out it.
> 
> I can think of a reason. If e.g. reclaim makes free pages above
> watermark in the 4th zone in the zonelist, we would like the
> subsequent get_page_from_freelist() to succeed in that 4th zone.
> Passing ALLOC_NO_WATERMARKS there would likely succeed in the first
> zone, needlessly below the watermark.

Hmm... it's intent would be like as your guess but I think that it's
not correct. There would be not much disadvantage even if we allocate the
freepage from 1st zone where watermark is unmet, since 1st zone would
be higher zone than 4th zone and who can utilize higher zone can
utilize lower zone. There are many corner case handlings and we can't
be sure that we would eventually do the ALLOC_NO_WATERMARKS allocation
attempt again. And, ALLOC_NO_WATERMARKS means that it is a very
importatn task so we need to make it successful as fast as possible. I
think that allowing ALLOC_NO_WATERMARKS as much as possible is better.

So, IMO, keeping ALLOC_NO_WATERMARKS in alloc_flags and don't mask out it
as much as possible would be a way to go.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web