Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <530d1ca4-6e05-b237-e0a9-c43d61767c4d@redhat.com>
Date:   Thu, 3 Feb 2022 10:25:51 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Content-Language: en-US
To:     Wei Yang <richard.weiyang@gmail.com>, akpm@linux-foundation.org,
        mhocko@suse.com, mgorman@techsingularity.net
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20220203020022.3044-1-richard.weiyang@gmail.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH] mm/page_alloc: add zone to zonelist if populated
In-Reply-To: <20220203020022.3044-1-richard.weiyang@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 03.02.22 03:00, Wei Yang wrote:
> During memory hotplug, when online/offline a zone, we need to rebuild
> the zonelist for all nodes. Current behavior would lose a valid zone in
> zonelist since only pick up managed_zone.
> 
> There are two cases for a zone with memory but still !managed.
> 
>   * all pages were allocated via memblock
>   * all pages were taken by ballooning / virtio-mem
> 
> This state maybe temporary, since both of them may release some memory.
> Then it end up with a managed zone not in zonelist.
> 
> This is introduced in 'commit 6aa303defb74 ("mm, vmscan: only allocate
> and reclaim from zones with pages managed by the buddy allocator")'.
> This patch restore the behavior.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Mel Gorman <mgorman@techsingularity.net>
> CC: David Hildenbrand <david@redhat.com>
> Fixes: 6aa303defb74 ("mm, vmscan: only allocate and reclaim from zones with pages managed by the buddy allocator")

That commit mentions that there used to be some ppc64 cases with fadump
where it might have been a real problem. Unfortunately, that commit
doesn't really tell what the performance implications are.

We'd have to know how many "permanent memblock" allocations we have,
that can never get freed.

> ---
>  mm/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index de15021a2887..b433a57ee76f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6092,7 +6092,7 @@ static int build_zonerefs_node(pg_data_t *pgdat, struct zoneref *zonerefs)
>  	do {
>  		zone_type--;
>  		zone = pgdat->node_zones + zone_type;
> -		if (managed_zone(zone)) {
> +		if (populated_zone(zone)) {
>  			zoneref_set_zone(zone, &zonerefs[nr_zones++]);
>  			check_highest_zone(zone_type);
>  		}

The comment above the function also expresses that "Add all populated
zones of a node to the zonelist.", so one way or the other, that should
be made consistent.

-- 
Thanks,

David / dhildenb