Re: [PATCH] mm/memory_hotplug: build zonelist for managed_zone

[Wei Yang <richard.weiyang@gmail.com>]

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