From: David Hildenbrand <david@redhat.com>
To: Oscar Salvador <osalvador@suse.de>,
Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@kernel.org>,
Wei Yang <richard.weiyang@gmail.com>,
Miaohe Lin <linmiaohe@huawei.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] mm/page_alloc: Do not calculate node's total pages and memmap pages when empty
Date: Thu, 28 Apr 2022 14:13:46 +0200 [thread overview]
Message-ID: <f31caf6a-fb13-0be3-9fa2-0b4959cc0810@redhat.com> (raw)
In-Reply-To: <20220307150725.6810-2-osalvador@suse.de>
On 07.03.22 16:07, Oscar Salvador wrote:
> free_area_init_node() calls calculate_node_totalpages() and
> free_area_init_core(). The former to get node's {spanned,present}_pages,
> and the latter to calculate, among other things, how many pages per zone
> we spent on memmap_pages, which is used to substract zone's free pages.
>
> On memoryless-nodes, it is pointless to perform such a bunch of work, so
> make sure we skip the calculations when having a node or empty zone.
>
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
Sorry, I'm late with review. My mailbox got flooded.
> ---
> mm/page_alloc.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 967085c1c78a..0b7d176a8990 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7312,6 +7312,10 @@ static void __init calculate_node_totalpages(struct pglist_data *pgdat,
> unsigned long realtotalpages = 0, totalpages = 0;
> enum zone_type i;
>
> + /* Skip calculation for memoryless nodes */
> + if (node_start_pfn == node_end_pfn)
> + goto no_pages;
> +
Just a NIT:
E.g., in zone_spanned_pages_in_node() we test for
!node_start_pfn && !node_end_pfn
In update_pgdat_span(), we set
node_start_pfn = node_end_pfn = 0;
when we find an empty node during memory unplug.
Therefore, I wonder if a helper "is_memoryless_node()" or "node_empty()"
might be reasonable, that just checks for either
!node_start_pfn && !node_end_pfn
or
node_start_pfn == node_end_pfn
> for (i = 0; i < MAX_NR_ZONES; i++) {
> struct zone *zone = pgdat->node_zones + i;
> unsigned long zone_start_pfn, zone_end_pfn;
> @@ -7344,6 +7348,7 @@ static void __init calculate_node_totalpages(struct pglist_data *pgdat,
> realtotalpages += real_size;
> }
>
> +no_pages:
> pgdat->node_spanned_pages = totalpages;
> pgdat->node_present_pages = realtotalpages;
> pr_debug("On node %d totalpages: %lu\n", pgdat->node_id, realtotalpages);
> @@ -7562,6 +7567,10 @@ static void __init free_area_init_core(struct pglist_data *pgdat)
> size = zone->spanned_pages;
> freesize = zone->present_pages;
>
> + /* No pages? Nothing to calculate then. */
> + if (!size)
> + goto no_pages;
> +
> /*
> * Adjust freesize so that it accounts for how much memory
> * is used by this zone for memmap. This affects the watermark
> @@ -7597,6 +7606,7 @@ static void __init free_area_init_core(struct pglist_data *pgdat)
> * when the bootmem allocator frees pages into the buddy system.
> * And all highmem pages will be managed by the buddy system.
> */
> +no_pages:
> zone_init_internals(zone, j, nid, freesize);
>
> if (!size)
We have another size check below. We essentially have right now:
"
if (!size)
goto no_pages;
[code]
no_pages:
zone_init_internals(zone, j, nid, freesize);
if (!size)
continue
[more code]
"
IMHO, it would be nicer to avoid the label/goto by just doing a:
"
if (!size) {
zone_init_internals(zone, j, nid, 0);
continue;
}
[code]
zone_init_internals(zone, j, nid, freesize);
[more code]
"
Or factoring out [code] into a separate function.
Anyhow, the change itself looks sane.
--
Thanks,
David / dhildenb
next prev parent reply other threads:[~2022-04-28 12:14 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-07 15:07 [PATCH 0/3] A minor hotplug refactoring Oscar Salvador
2022-03-07 15:07 ` [PATCH 1/3] mm/page_alloc: Do not calculate node's total pages and memmap pages when empty Oscar Salvador
2022-04-28 12:13 ` David Hildenbrand [this message]
2022-05-05 9:09 ` Oscar Salvador
2022-03-07 15:07 ` [PATCH 2/3] mm/memory_hotplug: Reset node's state when empty during offline Oscar Salvador
2022-04-28 12:30 ` David Hildenbrand
2022-05-05 10:37 ` Oscar Salvador
2022-03-07 15:07 ` [PATCH 3/3] mm/memory_hotplug: Refactor hotadd_init_pgdat and try_online_node Oscar Salvador
2022-04-28 12:35 ` David Hildenbrand
2022-05-05 9:17 ` Oscar Salvador
2022-04-27 21:05 ` [PATCH 0/3] A minor hotplug refactoring Andrew Morton
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=f31caf6a-fb13-0be3-9fa2-0b4959cc0810@redhat.com \
--to=david@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=linmiaohe@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=osalvador@suse.de \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.