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@xxxxxxxxx>
>>>> Fixes: 6aa303defb74 ("mm, vmscan: only allocate and reclaim from zones with pages managed by the buddy allocator")
>>>> CC: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> 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
