From: Michal Hocko <mhocko@suse.com>
To: Wei Yang <richard.weiyang@gmail.com>
Cc: akpm@linux-foundation.org, mgorman@techsingularity.net,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/memory_hotplug: build zonelist for managed_zone
Date: Mon, 31 Jan 2022 11:24:48 +0100 [thread overview]
Message-ID: <Yfe48O7eBWSe0LjK@dhcp22.suse.cz> (raw)
In-Reply-To: <20220129002628.2cwr35glahq5f5md@master>
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
next prev parent reply other threads:[~2022-01-31 10:24 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yfe48O7eBWSe0LjK@dhcp22.suse.cz \
--to=mhocko@suse.com \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@techsingularity.net \
--cc=richard.weiyang@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).