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

Groups > linux.kernel > #1624048 > unrolled thread

[PATCH 1/3] mm: consider zone which is not fully populated to have holes

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

  [PATCH 1/3] mm: consider zone which is not fully populated to have holes Michal Hocko <mhocko@kernel.org> - 2017-04-15 14:20 +0200
    Re: [PATCH 1/3] mm: consider zone which is not fully populated to  have holes Vlastimil Babka <vbabka@suse.cz> - 2017-04-18 10:50 +0200
      Re: [PATCH 1/3] mm: consider zone which is not fully populated to  have holes Michal Hocko <mhocko@kernel.org> - 2017-04-18 11:30 +0200
        Re: [PATCH 1/3] mm: consider zone which is not fully populated to  have holes Vlastimil Babka <vbabka@suse.cz> - 2017-04-19 14:00 +0200
          Re: [PATCH 1/3] mm: consider zone which is not fully populated to  have holes Michal Hocko <mhocko@kernel.org> - 2017-04-19 14:20 +0200
            Re: [PATCH 1/3] mm: consider zone which is not fully populated to  have holes Vlastimil Babka <vbabka@suse.cz> - 2017-04-19 14:40 +0200
              Re: [PATCH 1/3] mm: consider zone which is not fully populated to  have holes Michal Hocko <mhocko@kernel.org> - 2017-04-19 15:00 +0200

#1624048 — [PATCH 1/3] mm: consider zone which is not fully populated to have holes

From: Michal Hocko <mhocko@suse.com>

__pageblock_pfn_to_page has two users currently, set_zone_contiguous
which checks whether the given zone contains holes and
pageblock_pfn_to_page which then carefully returns a first valid
page from the given pfn range for the given zone. This doesn't handle
zones which are not fully populated though. Memory pageblocks can be
offlined or might not have been onlined yet. In such a case the zone
should be considered to have holes otherwise pfn walkers can touch
and play with offline pages.

Current callers of pageblock_pfn_to_page in compaction seem to work
properly right now because they only isolate PageBuddy
(isolate_freepages_block) or PageLRU resp. __PageMovable
(isolate_migratepages_block) which will be always false for these pages.
It would be safer to skip these pages altogether, though. In order
to do that let's check PageReserved in __pageblock_pfn_to_page because
offline pages are reserved.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/page_alloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0cacba69ab04..dcbbcfdda60e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1351,6 +1351,8 @@ struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
 		return NULL;
 
 	start_page = pfn_to_page(start_pfn);
+	if (PageReserved(start_page))
+		return NULL;
 
 	if (page_zone(start_page) != zone)
 		return NULL;
-- 
2.11.0

[toc] | [next] | [standalone]

#1625147 — Re: [PATCH 1/3] mm: consider zone which is not fully populated to have holes

On 04/15/2017 02:17 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> __pageblock_pfn_to_page has two users currently, set_zone_contiguous
> which checks whether the given zone contains holes and
> pageblock_pfn_to_page which then carefully returns a first valid
> page from the given pfn range for the given zone. This doesn't handle
> zones which are not fully populated though. Memory pageblocks can be
> offlined or might not have been onlined yet. In such a case the zone
> should be considered to have holes otherwise pfn walkers can touch
> and play with offline pages.
> 
> Current callers of pageblock_pfn_to_page in compaction seem to work
> properly right now because they only isolate PageBuddy
> (isolate_freepages_block) or PageLRU resp. __PageMovable
> (isolate_migratepages_block) which will be always false for these pages.
> It would be safer to skip these pages altogether, though. In order
> to do that let's check PageReserved in __pageblock_pfn_to_page because
> offline pages are reserved.

My issue with this is that PageReserved can be also set for other
reasons than offlined block, e.g. by a random driver. So there are two
suboptimal scenarios:

- PageReserved is set on some page in the middle of pageblock. It won't
be detected by this patch. This violates the "it would be safer" argument.
- PageReserved is set on just the first (few) page(s) and because of
this patch, we skip it completely and won't compact the rest of it.

So if we decide we really need to check PageReserved to ensure safety,
then we have to check it on each page. But I hope the existing criteria
in compaction scanners are sufficient. Unless the semantic is that if
somebody sets PageReserved, he's free to repurpose the rest of flags at
his will (IMHO that's not the case).

The pageblock-level check them becomes a performance optimization so
when there's an "offline hole", compaction won't iterate it page by
page. But the downside is the false positive resulting in skipping whole
pageblock due to single page.
I guess it's uncommon for a longlived offline holes to exist, so we
could simply just drop this?

> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  mm/page_alloc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0cacba69ab04..dcbbcfdda60e 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1351,6 +1351,8 @@ struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>  		return NULL;
>  
>  	start_page = pfn_to_page(start_pfn);
> +	if (PageReserved(start_page))
> +		return NULL;
>  
>  	if (page_zone(start_page) != zone)
>  		return NULL;
> 

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

#1625176 — Re: [PATCH 1/3] mm: consider zone which is not fully populated to have holes

On Tue 18-04-17 10:45:23, Vlastimil Babka wrote:
> On 04/15/2017 02:17 PM, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > __pageblock_pfn_to_page has two users currently, set_zone_contiguous
> > which checks whether the given zone contains holes and
> > pageblock_pfn_to_page which then carefully returns a first valid
> > page from the given pfn range for the given zone. This doesn't handle
> > zones which are not fully populated though. Memory pageblocks can be
> > offlined or might not have been onlined yet. In such a case the zone
> > should be considered to have holes otherwise pfn walkers can touch
> > and play with offline pages.
> > 
> > Current callers of pageblock_pfn_to_page in compaction seem to work
> > properly right now because they only isolate PageBuddy
> > (isolate_freepages_block) or PageLRU resp. __PageMovable
> > (isolate_migratepages_block) which will be always false for these pages.
> > It would be safer to skip these pages altogether, though. In order
> > to do that let's check PageReserved in __pageblock_pfn_to_page because
> > offline pages are reserved.
> 
> My issue with this is that PageReserved can be also set for other
> reasons than offlined block, e.g. by a random driver. So there are two
> suboptimal scenarios:
> 
> - PageReserved is set on some page in the middle of pageblock. It won't
> be detected by this patch. This violates the "it would be safer" argument.
> - PageReserved is set on just the first (few) page(s) and because of
> this patch, we skip it completely and won't compact the rest of it.

Why would that be a big problem? PageReserved is used only very seldom
and few page blocks skipped would seem like a minor issue to me.

> So if we decide we really need to check PageReserved to ensure safety,
> then we have to check it on each page. But I hope the existing criteria
> in compaction scanners are sufficient. Unless the semantic is that if
> somebody sets PageReserved, he's free to repurpose the rest of flags at
> his will (IMHO that's not the case).

I am not aware of any such user. PageReserved has always been about "the
core mm should touch these pages and modify their state" AFAIR.
But I believe that touching those holes just asks for problems so I
would rather have them covered.

> The pageblock-level check them becomes a performance optimization so
> when there's an "offline hole", compaction won't iterate it page by
> page. But the downside is the false positive resulting in skipping whole
> pageblock due to single page.
> I guess it's uncommon for a longlived offline holes to exist, so we
> could simply just drop this?

This is hard to tell but I can imagine that some memory hotplug
balloning drivers might want to offline hole into existing zones. 
 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> >  mm/page_alloc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 0cacba69ab04..dcbbcfdda60e 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1351,6 +1351,8 @@ struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
> >  		return NULL;
> >  
> >  	start_page = pfn_to_page(start_pfn);
> > +	if (PageReserved(start_page))
> > +		return NULL;
> >  
> >  	if (page_zone(start_page) != zone)
> >  		return NULL;
> > 

-- 
Michal Hocko
SUSE Labs

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

#1626048 — Re: [PATCH 1/3] mm: consider zone which is not fully populated to have holes

On 04/18/2017 11:27 AM, Michal Hocko wrote:
> On Tue 18-04-17 10:45:23, Vlastimil Babka wrote:
>> On 04/15/2017 02:17 PM, Michal Hocko wrote:
>>> From: Michal Hocko <mhocko@suse.com>
>>>
>>
>> My issue with this is that PageReserved can be also set for other
>> reasons than offlined block, e.g. by a random driver. So there are two
>> suboptimal scenarios:
>>
>> - PageReserved is set on some page in the middle of pageblock. It won't
>> be detected by this patch. This violates the "it would be safer" argument.
>> - PageReserved is set on just the first (few) page(s) and because of
>> this patch, we skip it completely and won't compact the rest of it.
> 
> Why would that be a big problem? PageReserved is used only very seldom
> and few page blocks skipped would seem like a minor issue to me.

Yes it's not critical, just suboptimal. Can be improved later.

>> So if we decide we really need to check PageReserved to ensure safety,
>> then we have to check it on each page. But I hope the existing criteria
>> in compaction scanners are sufficient. Unless the semantic is that if
>> somebody sets PageReserved, he's free to repurpose the rest of flags at
>> his will (IMHO that's not the case).
> 
> I am not aware of any such user. PageReserved has always been about "the
> core mm should touch these pages and modify their state" AFAIR.
> But I believe that touching those holes just asks for problems so I
> would rather have them covered.

OK. I guess it's OK to use PageReserved of first pageblock page to
determine if we can trust page_zone(), because the memory offline
scenario should have sufficient granularity and not make holes inside
pageblock?

>> The pageblock-level check them becomes a performance optimization so
>> when there's an "offline hole", compaction won't iterate it page by
>> page. But the downside is the false positive resulting in skipping whole
>> pageblock due to single page.
>> I guess it's uncommon for a longlived offline holes to exist, so we
>> could simply just drop this?
> 
> This is hard to tell but I can imagine that some memory hotplug
> balloning drivers might want to offline hole into existing zones.

OK.

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

#1626054 — Re: [PATCH 1/3] mm: consider zone which is not fully populated to have holes

On Wed 19-04-17 13:59:40, Vlastimil Babka wrote:
> On 04/18/2017 11:27 AM, Michal Hocko wrote:
[...]
> > I am not aware of any such user. PageReserved has always been about "the
> > core mm should touch these pages and modify their state" AFAIR.
> > But I believe that touching those holes just asks for problems so I
> > would rather have them covered.
> 
> OK. I guess it's OK to use PageReserved of first pageblock page to
> determine if we can trust page_zone(), because the memory offline
> scenario should have sufficient granularity and not make holes inside
> pageblock?

Yes memblocks should be section size aligned and that is 128M resp. 2GB
on large machines. So we are talking about much larger than page block
granularity here.

Anyway, Joonsoo didn't like the the explicit PageReserved checks so I
have come with pfn_to_online_page which hides this implementation
detail. How do you like the following instead?
---
From 0f5544b5d01f4bc1572e43cc2a0156ae33a2922c Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Thu, 13 Apr 2017 10:28:45 +0200
Subject: [PATCH] mm: consider zone which is not fully populated to have holes

__pageblock_pfn_to_page has two users currently, set_zone_contiguous
which checks whether the given zone contains holes and
pageblock_pfn_to_page which then carefully returns a first valid
page from the given pfn range for the given zone. This doesn't handle
zones which are not fully populated though. Memory pageblocks can be
offlined or might not have been onlined yet. In such a case the zone
should be considered to have holes otherwise pfn walkers can touch
and play with offline pages.

Current callers of pageblock_pfn_to_page in compaction seem to work
properly right now because they only isolate PageBuddy
(isolate_freepages_block) or PageLRU resp. __PageMovable
(isolate_migratepages_block) which will be always false for these pages.
It would be safer to skip these pages altogether, though. In order
to do that let's add pfn_to_online_page helper which checks PageReserved
because offline pages are reserved until they are onlined. There might
be other users of the PageReserved flag but they are rare and even if we
hit into those pages we should skip them in pfn walkers anyway. So this
is not harmful.

Use the new helper in __pageblock_pfn_to_page and skip the whole page
block in such a case. Vlastimil has noted that we might skip over
the page block even when there is a single reserved page but that
shouldn't lead to major issues because reserved pages are used very
seldom.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 include/linux/memory_hotplug.h | 28 ++++++++++++++++++++++++++++
 mm/page_alloc.c                |  4 +++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 3c8cf86201c3..736fe73e65af 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -14,6 +14,26 @@ struct memory_block;
 struct resource;
 
 #ifdef CONFIG_MEMORY_HOTPLUG
+/*
+ * Return page for the valid pfn only if the page is online.
+ * Offline pages are marked reserved. There are other users of PageReserved
+ * but pfn walkers should avoid them in general so such a false positive
+ * is not harmful.
+ *
+ * It would be great if this was a static inline but dependency hell doesn't
+ * allow that for now.
+ */
+#define pfn_to_online_page(pfn)				\
+({							\
+	struct page *___page = NULL;			\
+							\
+	if (pfn_valid(pfn)) {				\
+		___page = pfn_to_page(pfn);		\
+		if (unlikely(PageReserved(___page))) 	\
+			___page = NULL;			\
+	}						\
+	___page;					\
+})
 
 /*
  * Types for free bootmem stored in page->lru.next. These have to be in
@@ -203,6 +223,14 @@ extern void set_zone_contiguous(struct zone *zone);
 extern void clear_zone_contiguous(struct zone *zone);
 
 #else /* ! CONFIG_MEMORY_HOTPLUG */
+#define pfn_to_online_page(pfn)			\
+({						\
+	struct page *___page = NULL;		\
+	if (pfn_valid(pfn))			\
+		___page = pfn_to_page(pfn);	\
+	___page;				\
+ })
+
 /*
  * Stub functions for when hotplug is off
  */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5d72d29a6ece..9dd814f4e7f5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1353,7 +1353,9 @@ struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
 	if (!pfn_valid(start_pfn) || !pfn_valid(end_pfn))
 		return NULL;
 
-	start_page = pfn_to_page(start_pfn);
+	start_page = pfn_to_online_page(start_pfn);
+	if (!start_page)
+		return NULL;
 
 	if (page_zone(start_page) != zone)
 		return NULL;
-- 
2.11.0

-- 
Michal Hocko
SUSE Labs

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

#1626063 — Re: [PATCH 1/3] mm: consider zone which is not fully populated to have holes

On 04/19/2017 02:16 PM, Michal Hocko wrote:
> On Wed 19-04-17 13:59:40, Vlastimil Babka wrote:
>> On 04/18/2017 11:27 AM, Michal Hocko wrote:
> [...]
>>> I am not aware of any such user. PageReserved has always been about "the
>>> core mm should touch these pages and modify their state" AFAIR.
>>> But I believe that touching those holes just asks for problems so I
>>> would rather have them covered.
>>
>> OK. I guess it's OK to use PageReserved of first pageblock page to
>> determine if we can trust page_zone(), because the memory offline
>> scenario should have sufficient granularity and not make holes inside
>> pageblock?
> 
> Yes memblocks should be section size aligned and that is 128M resp. 2GB
> on large machines. So we are talking about much larger than page block
> granularity here.
> 
> Anyway, Joonsoo didn't like the the explicit PageReserved checks so I
> have come with pfn_to_online_page which hides this implementation
> detail. How do you like the following instead?

Yeah that's OK. The other two patches will be updated as well?
Ideally we would later convert this helper to use some special values
for zone/node id (such as -1) instead of PageReserved to indicate an
offline node, as we discussed.

> ---
> From 0f5544b5d01f4bc1572e43cc2a0156ae33a2922c Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Thu, 13 Apr 2017 10:28:45 +0200
> Subject: [PATCH] mm: consider zone which is not fully populated to have holes
> 
> __pageblock_pfn_to_page has two users currently, set_zone_contiguous
> which checks whether the given zone contains holes and
> pageblock_pfn_to_page which then carefully returns a first valid
> page from the given pfn range for the given zone. This doesn't handle
> zones which are not fully populated though. Memory pageblocks can be
> offlined or might not have been onlined yet. In such a case the zone
> should be considered to have holes otherwise pfn walkers can touch
> and play with offline pages.
> 
> Current callers of pageblock_pfn_to_page in compaction seem to work
> properly right now because they only isolate PageBuddy
> (isolate_freepages_block) or PageLRU resp. __PageMovable
> (isolate_migratepages_block) which will be always false for these pages.
> It would be safer to skip these pages altogether, though. In order
> to do that let's add pfn_to_online_page helper which checks PageReserved
> because offline pages are reserved until they are onlined. There might
> be other users of the PageReserved flag but they are rare and even if we
> hit into those pages we should skip them in pfn walkers anyway. So this
> is not harmful.
> 
> Use the new helper in __pageblock_pfn_to_page and skip the whole page
> block in such a case. Vlastimil has noted that we might skip over
> the page block even when there is a single reserved page but that
> shouldn't lead to major issues because reserved pages are used very
> seldom.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  include/linux/memory_hotplug.h | 28 ++++++++++++++++++++++++++++
>  mm/page_alloc.c                |  4 +++-
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 3c8cf86201c3..736fe73e65af 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -14,6 +14,26 @@ struct memory_block;
>  struct resource;
>  
>  #ifdef CONFIG_MEMORY_HOTPLUG
> +/*
> + * Return page for the valid pfn only if the page is online.
> + * Offline pages are marked reserved. There are other users of PageReserved
> + * but pfn walkers should avoid them in general so such a false positive
> + * is not harmful.
> + *
> + * It would be great if this was a static inline but dependency hell doesn't
> + * allow that for now.
> + */
> +#define pfn_to_online_page(pfn)				\
> +({							\
> +	struct page *___page = NULL;			\
> +							\
> +	if (pfn_valid(pfn)) {				\
> +		___page = pfn_to_page(pfn);		\
> +		if (unlikely(PageReserved(___page))) 	\
> +			___page = NULL;			\
> +	}						\
> +	___page;					\
> +})
>  
>  /*
>   * Types for free bootmem stored in page->lru.next. These have to be in
> @@ -203,6 +223,14 @@ extern void set_zone_contiguous(struct zone *zone);
>  extern void clear_zone_contiguous(struct zone *zone);
>  
>  #else /* ! CONFIG_MEMORY_HOTPLUG */
> +#define pfn_to_online_page(pfn)			\
> +({						\
> +	struct page *___page = NULL;		\
> +	if (pfn_valid(pfn))			\
> +		___page = pfn_to_page(pfn);	\
> +	___page;				\
> + })
> +
>  /*
>   * Stub functions for when hotplug is off
>   */
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5d72d29a6ece..9dd814f4e7f5 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1353,7 +1353,9 @@ struct page *__pageblock_pfn_to_page(unsigned long start_pfn,
>  	if (!pfn_valid(start_pfn) || !pfn_valid(end_pfn))
>  		return NULL;
>  
> -	start_page = pfn_to_page(start_pfn);
> +	start_page = pfn_to_online_page(start_pfn);
> +	if (!start_page)
> +		return NULL;
>  
>  	if (page_zone(start_page) != zone)
>  		return NULL;
> 

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

#1626074 — Re: [PATCH 1/3] mm: consider zone which is not fully populated to have holes

On Wed 19-04-17 14:34:54, Vlastimil Babka wrote:
> On 04/19/2017 02:16 PM, Michal Hocko wrote:
> > On Wed 19-04-17 13:59:40, Vlastimil Babka wrote:
> >> On 04/18/2017 11:27 AM, Michal Hocko wrote:
> > [...]
> >>> I am not aware of any such user. PageReserved has always been about "the
> >>> core mm should touch these pages and modify their state" AFAIR.
> >>> But I believe that touching those holes just asks for problems so I
> >>> would rather have them covered.
> >>
> >> OK. I guess it's OK to use PageReserved of first pageblock page to
> >> determine if we can trust page_zone(), because the memory offline
> >> scenario should have sufficient granularity and not make holes inside
> >> pageblock?
> > 
> > Yes memblocks should be section size aligned and that is 128M resp. 2GB
> > on large machines. So we are talking about much larger than page block
> > granularity here.
> > 
> > Anyway, Joonsoo didn't like the the explicit PageReserved checks so I
> > have come with pfn_to_online_page which hides this implementation
> > detail. How do you like the following instead?
> 
> Yeah that's OK. The other two patches will be updated as well?

yes

> Ideally we would later convert this helper to use some special values
> for zone/node id (such as -1) instead of PageReserved to indicate an
> offline node, as we discussed.

I have considered zone_id to be -1 but there is just too much code which
uses the id to translate it to the struct zone * directly and that would
lead to subtle bugs. On the other hand zone_id == 0 is not optimal but
much safer from that POV. I will think about the safest way forward long
term but my intention was to have something reasonably good for starter.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web