linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/memory_hotplug: build zonelist for managed_zone
@ 2022-01-27  1:20 Wei Yang
  2022-01-27  8:28 ` Michal Hocko
  2022-01-27  8:39 ` David Hildenbrand
  0 siblings, 2 replies; 9+ messages in thread
From: Wei Yang @ 2022-01-27  1:20 UTC (permalink / raw)
  To: akpm, mgorman, mhocko; +Cc: linux-mm, linux-kernel, Wei Yang

During memory hotplug, when online/offline a zone, we need to rebuild
the zonelist for all node. There are two checks to decide whether a zone
would be added to zonelist:

  * one in online_pages/offline_pages to decide necessity
  * one in build_zonerefs_node to do real add

Currently we use different criteria at these two places, which is
different from the original behavior.

Originally during memory hotplug, zonelist is re-built when zone hasn't
been populated. This in introduced in 'commit 6811378e7d8b ("[PATCH]
wait_table and zonelist initializing for memory hotadd: update zonelists")'.
And at that moment, build_zonelists_node() also use populated_zone() to
decide whether the zone should be added to zonelist.

While in 'commit 6aa303defb74 ("mm, vmscan: only allocate and reclaim
from zones with pages managed by the buddy allocator")',
build_zonelists_node() changed to use managed_zone() to add zonelist.
But we still use populated_zone() to decide the necessity.

This patch restore the original behavior by using the same criteria to
add a zone in zonelist during memory hotplug.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Fixes: 6aa303defb74 ("mm, vmscan: only allocate and reclaim from zones with pages managed by the buddy allocator")
CC: Mel Gorman <mgorman@techsingularity.net>
---
 mm/memory_hotplug.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 2a9627dc784c..8f1906b33937 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1102,11 +1102,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
 	spin_unlock_irqrestore(&zone->lock, flags);
 
 	/*
-	 * If this zone is not populated, then it is not in zonelist.
+	 * If this zone is not managed, then it is not in zonelist.
 	 * This means the page allocator ignores this zone.
 	 * So, zonelist must be updated after online.
 	 */
-	if (!populated_zone(zone)) {
+	if (!managed_zone(zone)) {
 		need_zonelists_rebuild = 1;
 		setup_zone_pageset(zone);
 	}
@@ -1985,7 +1985,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
 	/* reinitialise watermarks and update pcp limits */
 	init_per_zone_wmark_min();
 
-	if (!populated_zone(zone)) {
+	if (!managed_zone(zone)) {
 		zone_pcp_reset(zone);
 		build_all_zonelists(NULL);
 	}
-- 
2.33.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/memory_hotplug: build zonelist for managed_zone
  2022-01-27  1:20 [PATCH] mm/memory_hotplug: build zonelist for managed_zone Wei Yang
@ 2022-01-27  8:28 ` Michal Hocko
  2022-01-29  0:26   ` Wei Yang
  2022-01-27  8:39 ` David Hildenbrand
  1 sibling, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2022-01-27  8:28 UTC (permalink / raw)
  To: Wei Yang; +Cc: akpm, mgorman, linux-mm, linux-kernel

On Thu 27-01-22 01:20:23, Wei Yang wrote:
> During memory hotplug, when online/offline a zone, we need to rebuild
> the zonelist for all node. There are two checks to decide whether a zone
> would be added to zonelist:
> 
>   * one in online_pages/offline_pages to decide necessity
>   * one in build_zonerefs_node to do real add
> 
> Currently we use different criteria at these two places, which is
> different from the original behavior.
> 
> Originally during memory hotplug, zonelist is re-built when zone hasn't
> been populated. This in introduced in 'commit 6811378e7d8b ("[PATCH]
> wait_table and zonelist initializing for memory hotadd: update zonelists")'.
> And at that moment, build_zonelists_node() also use populated_zone() to
> decide whether the zone should be added to zonelist.
> 
> While in 'commit 6aa303defb74 ("mm, vmscan: only allocate and reclaim
> from zones with pages managed by the buddy allocator")',
> build_zonelists_node() changed to use managed_zone() to add zonelist.
> But we still use populated_zone() to decide the necessity.
> 
> This patch restore the original behavior by using the same criteria to
> add a zone in zonelist during memory hotplug.

Why?

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Fixes: 6aa303defb74 ("mm, vmscan: only allocate and reclaim from zones with pages managed by the buddy allocator")
> CC: Mel Gorman <mgorman@techsingularity.net>
> ---
>  mm/memory_hotplug.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 2a9627dc784c..8f1906b33937 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1102,11 +1102,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
>  	spin_unlock_irqrestore(&zone->lock, flags);
>  
>  	/*
> -	 * If this zone is not populated, then it is not in zonelist.
> +	 * If this zone is not managed, then it is not in zonelist.
>  	 * This means the page allocator ignores this zone.
>  	 * So, zonelist must be updated after online.
>  	 */
> -	if (!populated_zone(zone)) {
> +	if (!managed_zone(zone)) {
>  		need_zonelists_rebuild = 1;
>  		setup_zone_pageset(zone);
>  	}
> @@ -1985,7 +1985,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>  	/* reinitialise watermarks and update pcp limits */
>  	init_per_zone_wmark_min();
>  
> -	if (!populated_zone(zone)) {
> +	if (!managed_zone(zone)) {
>  		zone_pcp_reset(zone);
>  		build_all_zonelists(NULL);
>  	}
> -- 
> 2.33.1

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/memory_hotplug: build zonelist for managed_zone
  2022-01-27  1:20 [PATCH] mm/memory_hotplug: build zonelist for managed_zone Wei Yang
  2022-01-27  8:28 ` Michal Hocko
@ 2022-01-27  8:39 ` David Hildenbrand
  2022-01-29  0:27   ` Wei Yang
  1 sibling, 1 reply; 9+ messages in thread
From: David Hildenbrand @ 2022-01-27  8:39 UTC (permalink / raw)
  To: Wei Yang, akpm, mgorman, mhocko; +Cc: linux-mm, linux-kernel

On 27.01.22 02:20, Wei Yang wrote:
> During memory hotplug, when online/offline a zone, we need to rebuild
> the zonelist for all node. There are two checks to decide whether a zone
> would be added to zonelist:
> 
>   * one in online_pages/offline_pages to decide necessity
>   * one in build_zonerefs_node to do real add
> 
> Currently we use different criteria at these two places, which is
> different from the original behavior.
> 
> Originally during memory hotplug, zonelist is re-built when zone hasn't
> been populated. This in introduced in 'commit 6811378e7d8b ("[PATCH]
> wait_table and zonelist initializing for memory hotadd: update zonelists")'.
> And at that moment, build_zonelists_node() also use populated_zone() to
> decide whether the zone should be added to zonelist.
> 
> While in 'commit 6aa303defb74 ("mm, vmscan: only allocate and reclaim
> from zones with pages managed by the buddy allocator")',
> build_zonelists_node() changed to use managed_zone() to add zonelist.
> But we still use populated_zone() to decide the necessity.
> 
> This patch restore the original behavior by using the same criteria to
> add a zone in zonelist during memory hotplug.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Fixes: 6aa303defb74 ("mm, vmscan: only allocate and reclaim from zones with pages managed by the buddy allocator")
> CC: Mel Gorman <mgorman@techsingularity.net>
> ---
>  mm/memory_hotplug.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 2a9627dc784c..8f1906b33937 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1102,11 +1102,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
>  	spin_unlock_irqrestore(&zone->lock, flags);
>  
>  	/*
> -	 * If this zone is not populated, then it is not in zonelist.
> +	 * If this zone is not managed, then it is not in zonelist.
>  	 * This means the page allocator ignores this zone.
>  	 * So, zonelist must be updated after online.
>  	 */
> -	if (!populated_zone(zone)) {
> +	if (!managed_zone(zone)) {
>  		need_zonelists_rebuild = 1;
>  		setup_zone_pageset(zone);
>  	}
> @@ -1985,7 +1985,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>  	/* reinitialise watermarks and update pcp limits */
>  	init_per_zone_wmark_min();
>  
> -	if (!populated_zone(zone)) {
> +	if (!managed_zone(zone)) {
>  		zone_pcp_reset(zone);
>  		build_all_zonelists(NULL);
>  	}

A note that managed_zone() is a moving target w.r.t. memory ballooning.
In extreme cases, we can have whole zones (temporarily) be completely
!managed for that reason.

IMHO memory hot(un)plug is usually the wrong place to check for
managed_zone(), it cares about populated_zone().

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/memory_hotplug: build zonelist for managed_zone
  2022-01-27  8:28 ` Michal Hocko
@ 2022-01-29  0:26   ` Wei Yang
  2022-01-31 10:24     ` Michal Hocko
  0 siblings, 1 reply; 9+ messages in thread
From: Wei Yang @ 2022-01-29  0:26 UTC (permalink / raw)
  To: Michal Hocko; +Cc: Wei Yang, akpm, mgorman, linux-mm, linux-kernel

On Thu, Jan 27, 2022 at 09:28:17AM +0100, Michal Hocko wrote:
>On Thu 27-01-22 01:20:23, Wei Yang wrote:
>> During memory hotplug, when online/offline a zone, we need to rebuild
>> the zonelist for all node. There are two checks to decide whether a zone
>> would be added to zonelist:
>> 
>>   * one in online_pages/offline_pages to decide necessity
>>   * one in build_zonerefs_node to do real add
>> 
>> Currently we use different criteria at these two places, which is
>> different from the original behavior.
>> 
>> Originally during memory hotplug, zonelist is re-built when zone hasn't
>> been populated. This in introduced in 'commit 6811378e7d8b ("[PATCH]
>> wait_table and zonelist initializing for memory hotadd: update zonelists")'.
>> And at that moment, build_zonelists_node() also use populated_zone() to
>> decide whether the zone should be added to zonelist.
>> 
>> While in 'commit 6aa303defb74 ("mm, vmscan: only allocate and reclaim
>> from zones with pages managed by the buddy allocator")',
>> build_zonelists_node() changed to use managed_zone() to add zonelist.
>> But we still use populated_zone() to decide the necessity.
>> 
>> This patch restore the original behavior by using the same criteria to
>> add a zone in zonelist during memory hotplug.
>
>Why?
>

In case we online a populated zone, but not managed. Then this zone will not
be in zonelist. Right?

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/memory_hotplug: build zonelist for managed_zone
  2022-01-27  8:39 ` David Hildenbrand
@ 2022-01-29  0:27   ` Wei Yang
  2022-01-31 10:53     ` David Hildenbrand
  0 siblings, 1 reply; 9+ messages in thread
From: Wei Yang @ 2022-01-29  0:27 UTC (permalink / raw)
  To: David Hildenbrand; +Cc: Wei Yang, akpm, mgorman, mhocko, linux-mm, linux-kernel

On Thu, Jan 27, 2022 at 09:39:56AM +0100, David Hildenbrand wrote:
>On 27.01.22 02:20, Wei Yang wrote:
>> During memory hotplug, when online/offline a zone, we need to rebuild
>> the zonelist for all node. There are two checks to decide whether a zone
>> would be added to zonelist:
>> 
>>   * one in online_pages/offline_pages to decide necessity
>>   * one in build_zonerefs_node to do real add
>> 
>> Currently we use different criteria at these two places, which is
>> different from the original behavior.
>> 
>> Originally during memory hotplug, zonelist is re-built when zone hasn't
>> been populated. This in introduced in 'commit 6811378e7d8b ("[PATCH]
>> wait_table and zonelist initializing for memory hotadd: update zonelists")'.
>> And at that moment, build_zonelists_node() also use populated_zone() to
>> decide whether the zone should be added to zonelist.
>> 
>> While in 'commit 6aa303defb74 ("mm, vmscan: only allocate and reclaim
>> from zones with pages managed by the buddy allocator")',
>> build_zonelists_node() changed to use managed_zone() to add zonelist.
>> But we still use populated_zone() to decide the necessity.
>> 
>> This patch restore the original behavior by using the same criteria to
>> add a zone in zonelist during memory hotplug.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> Fixes: 6aa303defb74 ("mm, vmscan: only allocate and reclaim from zones with pages managed by the buddy allocator")
>> CC: Mel Gorman <mgorman@techsingularity.net>
>> ---
>>  mm/memory_hotplug.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 2a9627dc784c..8f1906b33937 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1102,11 +1102,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
>>  	spin_unlock_irqrestore(&zone->lock, flags);
>>  
>>  	/*
>> -	 * If this zone is not populated, then it is not in zonelist.
>> +	 * If this zone is not managed, then it is not in zonelist.
>>  	 * This means the page allocator ignores this zone.
>>  	 * So, zonelist must be updated after online.
>>  	 */
>> -	if (!populated_zone(zone)) {
>> +	if (!managed_zone(zone)) {
>>  		need_zonelists_rebuild = 1;
>>  		setup_zone_pageset(zone);
>>  	}
>> @@ -1985,7 +1985,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>>  	/* reinitialise watermarks and update pcp limits */
>>  	init_per_zone_wmark_min();
>>  
>> -	if (!populated_zone(zone)) {
>> +	if (!managed_zone(zone)) {
>>  		zone_pcp_reset(zone);
>>  		build_all_zonelists(NULL);
>>  	}
>
>A note that managed_zone() is a moving target w.r.t. memory ballooning.
>In extreme cases, we can have whole zones (temporarily) be completely
>!managed for that reason.
>
>IMHO memory hot(un)plug is usually the wrong place to check for
>managed_zone(), it cares about populated_zone().
>

So we need to check populated_zone when building zonelist?

>-- 
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/memory_hotplug: build zonelist for managed_zone
  2022-01-29  0:26   ` Wei Yang
@ 2022-01-31 10:24     ` Michal Hocko
  2022-02-01  6:33       ` Wei Yang
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Hocko @ 2022-01-31 10:24 UTC (permalink / raw)
  To: Wei Yang; +Cc: akpm, mgorman, linux-mm, linux-kernel

On Sat 29-01-22 00:26:28, Wei Yang wrote:
> On Thu, Jan 27, 2022 at 09:28:17AM +0100, Michal Hocko wrote:
> >On Thu 27-01-22 01:20:23, Wei Yang wrote:
> >> During memory hotplug, when online/offline a zone, we need to rebuild
> >> the zonelist for all node. There are two checks to decide whether a zone
> >> would be added to zonelist:
> >> 
> >>   * one in online_pages/offline_pages to decide necessity
> >>   * one in build_zonerefs_node to do real add
> >> 
> >> Currently we use different criteria at these two places, which is
> >> different from the original behavior.
> >> 
> >> Originally during memory hotplug, zonelist is re-built when zone hasn't
> >> been populated. This in introduced in 'commit 6811378e7d8b ("[PATCH]
> >> wait_table and zonelist initializing for memory hotadd: update zonelists")'.
> >> And at that moment, build_zonelists_node() also use populated_zone() to
> >> decide whether the zone should be added to zonelist.
> >> 
> >> While in 'commit 6aa303defb74 ("mm, vmscan: only allocate and reclaim
> >> from zones with pages managed by the buddy allocator")',
> >> build_zonelists_node() changed to use managed_zone() to add zonelist.
> >> But we still use populated_zone() to decide the necessity.
> >> 
> >> This patch restore the original behavior by using the same criteria to
> >> add a zone in zonelist during memory hotplug.
> >
> >Why?
> >
> 
> In case we online a populated zone, but not managed. Then this zone will not
> be in zonelist. Right?

yeah. We can theoretically end up with a zone without any managed menory
on the zonelists. But my primary question is why do we need this change?
Does it fix any existing problem? Does it make the code easier to
read/understand? Does it improve performance? Every patch should have a
justification. Your changelog merely lists the history and then states
the new behavior without any explanation of why that is needed or
desired. See?
-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/memory_hotplug: build zonelist for managed_zone
  2022-01-29  0:27   ` Wei Yang
@ 2022-01-31 10:53     ` David Hildenbrand
  2022-02-01  7:00       ` Wei Yang
  0 siblings, 1 reply; 9+ messages in thread
From: David Hildenbrand @ 2022-01-31 10:53 UTC (permalink / raw)
  To: Wei Yang; +Cc: akpm, mgorman, mhocko, linux-mm, linux-kernel

On 29.01.22 01:27, Wei Yang wrote:
> On Thu, Jan 27, 2022 at 09:39:56AM +0100, David Hildenbrand wrote:
>> On 27.01.22 02:20, Wei Yang wrote:
>>> During memory hotplug, when online/offline a zone, we need to rebuild
>>> the zonelist for all node. There are two checks to decide whether a zone
>>> would be added to zonelist:
>>>
>>>   * one in online_pages/offline_pages to decide necessity
>>>   * one in build_zonerefs_node to do real add
>>>
>>> Currently we use different criteria at these two places, which is
>>> different from the original behavior.
>>>
>>> Originally during memory hotplug, zonelist is re-built when zone hasn't
>>> been populated. This in introduced in 'commit 6811378e7d8b ("[PATCH]
>>> wait_table and zonelist initializing for memory hotadd: update zonelists")'.
>>> And at that moment, build_zonelists_node() also use populated_zone() to
>>> decide whether the zone should be added to zonelist.
>>>
>>> While in 'commit 6aa303defb74 ("mm, vmscan: only allocate and reclaim
>>> from zones with pages managed by the buddy allocator")',
>>> build_zonelists_node() changed to use managed_zone() to add zonelist.
>>> But we still use populated_zone() to decide the necessity.
>>>
>>> This patch restore the original behavior by using the same criteria to
>>> add a zone in zonelist during memory hotplug.
>>>
>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>> Fixes: 6aa303defb74 ("mm, vmscan: only allocate and reclaim from zones with pages managed by the buddy allocator")
>>> CC: Mel Gorman <mgorman@techsingularity.net>
>>> ---
>>>  mm/memory_hotplug.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 2a9627dc784c..8f1906b33937 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1102,11 +1102,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
>>>  	spin_unlock_irqrestore(&zone->lock, flags);
>>>  
>>>  	/*
>>> -	 * If this zone is not populated, then it is not in zonelist.
>>> +	 * If this zone is not managed, then it is not in zonelist.
>>>  	 * This means the page allocator ignores this zone.
>>>  	 * So, zonelist must be updated after online.
>>>  	 */
>>> -	if (!populated_zone(zone)) {
>>> +	if (!managed_zone(zone)) {
>>>  		need_zonelists_rebuild = 1;
>>>  		setup_zone_pageset(zone);
>>>  	}
>>> @@ -1985,7 +1985,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>>>  	/* reinitialise watermarks and update pcp limits */
>>>  	init_per_zone_wmark_min();
>>>  
>>> -	if (!populated_zone(zone)) {
>>> +	if (!managed_zone(zone)) {
>>>  		zone_pcp_reset(zone);
>>>  		build_all_zonelists(NULL);
>>>  	}
>>
>> A note that managed_zone() is a moving target w.r.t. memory ballooning.
>> In extreme cases, we can have whole zones (temporarily) be completely
>> !managed for that reason.
>>
>> IMHO memory hot(un)plug is usually the wrong place to check for
>> managed_zone(), it cares about populated_zone().
>>
> 
> So we need to check populated_zone when building zonelist?

I think the issue is that

a) We can have zones without any managed pages put present page during
boot, for example, if all pages in the zone were allocated via memblock.

b) We can have zones without any managed pages temporarily -- extreme
cases of memory ballooning / virtio-mem.


I tend to think that populated_zone() might actually be the right check
whenever building a zonelist. Because even in case of a) we might end up
freeing a memblock allocation later, suddenly having free pages in such
a zone, but the zone not in any zonelist.

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/memory_hotplug: build zonelist for managed_zone
  2022-01-31 10:24     ` Michal Hocko
@ 2022-02-01  6:33       ` Wei Yang
  0 siblings, 0 replies; 9+ messages in thread
From: Wei Yang @ 2022-02-01  6:33 UTC (permalink / raw)
  To: Michal Hocko; +Cc: Wei Yang, akpm, mgorman, linux-mm, linux-kernel

On Mon, Jan 31, 2022 at 11:24:48AM +0100, Michal Hocko wrote:
>> >> This patch restore the original behavior by using the same criteria to
>> >> add a zone in zonelist during memory hotplug.
>> >
>> >Why?
>> >
>> 
>> In case we online a populated zone, but not managed. Then this zone will not
>> be in zonelist. Right?
>
>yeah. We can theoretically end up with a zone without any managed menory
>on the zonelists. But my primary question is why do we need this change?

Currently I don't see a real case for this.

>Does it fix any existing problem? Does it make the code easier to
>read/understand? Does it improve performance? Every patch should have a
>justification. Your changelog merely lists the history and then states
>the new behavior without any explanation of why that is needed or
>desired. See?

I don't have such reason now.

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mm/memory_hotplug: build zonelist for managed_zone
  2022-01-31 10:53     ` David Hildenbrand
@ 2022-02-01  7:00       ` Wei Yang
  0 siblings, 0 replies; 9+ messages in thread
From: Wei Yang @ 2022-02-01  7:00 UTC (permalink / raw)
  To: David Hildenbrand; +Cc: Wei Yang, akpm, mgorman, mhocko, linux-mm, linux-kernel

On Mon, Jan 31, 2022 at 11:53:36AM +0100, David Hildenbrand wrote:
>On 29.01.22 01:27, Wei Yang wrote:
>> On Thu, Jan 27, 2022 at 09:39:56AM +0100, David Hildenbrand wrote:
>>> On 27.01.22 02:20, Wei Yang wrote:
>>>> During memory hotplug, when online/offline a zone, we need to rebuild
>>>> the zonelist for all node. There are two checks to decide whether a zone
>>>> would be added to zonelist:
>>>>
>>>>   * one in online_pages/offline_pages to decide necessity
>>>>   * one in build_zonerefs_node to do real add
>>>>
>>>> Currently we use different criteria at these two places, which is
>>>> different from the original behavior.
>>>>
>>>> Originally during memory hotplug, zonelist is re-built when zone hasn't
>>>> been populated. This in introduced in 'commit 6811378e7d8b ("[PATCH]
>>>> wait_table and zonelist initializing for memory hotadd: update zonelists")'.
>>>> And at that moment, build_zonelists_node() also use populated_zone() to
>>>> decide whether the zone should be added to zonelist.
>>>>
>>>> While in 'commit 6aa303defb74 ("mm, vmscan: only allocate and reclaim
>>>> from zones with pages managed by the buddy allocator")',
>>>> build_zonelists_node() changed to use managed_zone() to add zonelist.
>>>> But we still use populated_zone() to decide the necessity.
>>>>
>>>> This patch restore the original behavior by using the same criteria to
>>>> add a zone in zonelist during memory hotplug.
>>>>
>>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>>> Fixes: 6aa303defb74 ("mm, vmscan: only allocate and reclaim from zones with pages managed by the buddy allocator")
>>>> CC: Mel Gorman <mgorman@techsingularity.net>
>>>> ---
>>>>  mm/memory_hotplug.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>>> index 2a9627dc784c..8f1906b33937 100644
>>>> --- a/mm/memory_hotplug.c
>>>> +++ b/mm/memory_hotplug.c
>>>> @@ -1102,11 +1102,11 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
>>>>  	spin_unlock_irqrestore(&zone->lock, flags);
>>>>  
>>>>  	/*
>>>> -	 * If this zone is not populated, then it is not in zonelist.
>>>> +	 * If this zone is not managed, then it is not in zonelist.
>>>>  	 * This means the page allocator ignores this zone.
>>>>  	 * So, zonelist must be updated after online.
>>>>  	 */
>>>> -	if (!populated_zone(zone)) {
>>>> +	if (!managed_zone(zone)) {
>>>>  		need_zonelists_rebuild = 1;
>>>>  		setup_zone_pageset(zone);
>>>>  	}
>>>> @@ -1985,7 +1985,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>>>>  	/* reinitialise watermarks and update pcp limits */
>>>>  	init_per_zone_wmark_min();
>>>>  
>>>> -	if (!populated_zone(zone)) {
>>>> +	if (!managed_zone(zone)) {
>>>>  		zone_pcp_reset(zone);
>>>>  		build_all_zonelists(NULL);
>>>>  	}
>>>
>>> A note that managed_zone() is a moving target w.r.t. memory ballooning.
>>> In extreme cases, we can have whole zones (temporarily) be completely
>>> !managed for that reason.
>>>
>>> IMHO memory hot(un)plug is usually the wrong place to check for
>>> managed_zone(), it cares about populated_zone().
>>>
>> 
>> So we need to check populated_zone when building zonelist?
>
>I think the issue is that
>
>a) We can have zones without any managed pages put present page during
>boot, for example, if all pages in the zone were allocated via memblock.
>
>b) We can have zones without any managed pages temporarily -- extreme
>cases of memory ballooning / virtio-mem.
>
>
>I tend to think that populated_zone() might actually be the right check
>whenever building a zonelist. Because even in case of a) we might end up
>freeing a memblock allocation later, suddenly having free pages in such
>a zone, but the zone not in any zonelist.

I agree with you for this point.

>
>-- 
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-02-01  7:00 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-27  1:20 [PATCH] mm/memory_hotplug: build zonelist for managed_zone Wei Yang
2022-01-27  8:28 ` Michal Hocko
2022-01-29  0:26   ` Wei Yang
2022-01-31 10:24     ` Michal Hocko
2022-02-01  6:33       ` Wei Yang
2022-01-27  8:39 ` David Hildenbrand
2022-01-29  0:27   ` Wei Yang
2022-01-31 10:53     ` David Hildenbrand
2022-02-01  7:00       ` Wei Yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).