| Message ID | 20220307150725.6810-2-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A minor hotplug refactoring | expand |
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.
On Thu, Apr 28, 2022 at 02:13:46PM +0200, David Hildenbrand wrote: > Sorry, I'm late with review. My mailbox got flooded. Hi David :) > > + /* 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 Yeah, I thoguth about that as well, but given the few places we check for it I was hesitant to add it. But it might make the situation more clear, so I will go with a helper. > > +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. I did not think about how a refactor would look, so for now I will go with your first proposal. If I see that a refactor is due, I will think more about it. thanks!
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; + 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)
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> --- mm/page_alloc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)