| Message ID | 20220621041717.6355-2-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Minor memoryhotplug refactoring | expand |
On 21.06.22 06:17, 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> > --- > mm/page_alloc.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index e008a3df0485..2b9b2422ba32 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -7361,6 +7361,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 (pgdat_is_empty(pgdat)) > + 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; > @@ -7393,6 +7397,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); > @@ -7610,6 +7615,12 @@ 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) { > + zone_init_internals(zone, j, nid, 0); > + continue; > + } > + > /* > * Adjust freesize so that it accounts for how much memory > * is used by this zone for memmap. This affects the watermark > @@ -7647,9 +7658,6 @@ static void __init free_area_init_core(struct pglist_data *pgdat) > */ > zone_init_internals(zone, j, nid, freesize); > > - if (!size) > - continue; > - > set_pageblock_order(); > setup_usemap(zone); > init_currently_empty_zone(zone, zone->zone_start_pfn, size); > @@ -7730,7 +7738,7 @@ static void __init free_area_init_node(int nid) > pgdat->node_start_pfn = start_pfn; > pgdat->per_cpu_nodestats = NULL; > > - if (start_pfn != end_pfn) { > + if (!pgdat_is_empty(pgdat)) { > pr_info("Initmem setup node %d [mem %#018Lx-%#018Lx]\n", nid, > (u64)start_pfn << PAGE_SHIFT, > end_pfn ? ((u64)end_pfn << PAGE_SHIFT) - 1 : 0); It's worth noting that the check in pgdat_is_empty() is slightly different. I *think* it doesn't matter in practice, yet I wonder if we should simply fixup (currently unused) pgdat_is_empty(). Anyhow Reviewed-by: David Hildenbrand <david@redhat.com>
On Tue, Jun 21, 2022 at 09:44:47AM +0200, David Hildenbrand wrote: > > > It's worth noting that the check in pgdat_is_empty() is slightly > different. I *think* it doesn't matter in practice, yet I wonder if we > should simply fixup (currently unused) pgdat_is_empty(). I guess we could change it to static inline bool pgdat_is_empty(pg_data_t *pgdat) { return node_start_pfn(pgdat->node_id) == node_end_pfn(pgdat->node_id) } ? And maybe even rename it to to node_is_empty (not sure why but I tend to like that more than pgdat) I could squeeze a "fixup" patch for that before this one. > > Anyhow > > Reviewed-by: David Hildenbrand <david@redhat.com> Thanks!
On Wed, Jun 22, 2022 at 05:47:22AM +0200, Oscar Salvador wrote: > On Tue, Jun 21, 2022 at 09:44:47AM +0200, David Hildenbrand wrote: > > > > > > It's worth noting that the check in pgdat_is_empty() is slightly > > different. I *think* it doesn't matter in practice, yet I wonder if we > > should simply fixup (currently unused) pgdat_is_empty(). > > I guess we could change it to > > static inline bool pgdat_is_empty(pg_data_t *pgdat) > { > return node_start_pfn(pgdat->node_id) == node_end_pfn(pgdat->node_id) > } > > ? And maybe even rename it to to node_is_empty (not sure why but I tend to like At least I like this name (node_is_empty) as well. Thanks. > that more than pgdat) > > I could squeeze a "fixup" patch for that before this one. > > > > > Anyhow > > > > Reviewed-by: David Hildenbrand <david@redhat.com> > > Thanks! > > > -- > Oscar Salvador > SUSE Labs >
On 22.06.22 05:56, Muchun Song wrote: > On Wed, Jun 22, 2022 at 05:47:22AM +0200, Oscar Salvador wrote: >> On Tue, Jun 21, 2022 at 09:44:47AM +0200, David Hildenbrand wrote: >>> >>> >>> It's worth noting that the check in pgdat_is_empty() is slightly >>> different. I *think* it doesn't matter in practice, yet I wonder if we >>> should simply fixup (currently unused) pgdat_is_empty(). >> >> I guess we could change it to >> >> static inline bool pgdat_is_empty(pg_data_t *pgdat) >> { >> return node_start_pfn(pgdat->node_id) == node_end_pfn(pgdat->node_id) >> } >> >> ? And maybe even rename it to to node_is_empty (not sure why but I tend to like > > At least I like this name (node_is_empty) as well. > Let's try keeping it consistent. I think node_is_empty() might indicate that we're punching in a node id instead of a pgdat.
On Wed, Jun 22, 2022 at 10:31:12AM +0200, David Hildenbrand wrote: > On 22.06.22 05:56, Muchun Song wrote: > > On Wed, Jun 22, 2022 at 05:47:22AM +0200, Oscar Salvador wrote: > >> On Tue, Jun 21, 2022 at 09:44:47AM +0200, David Hildenbrand wrote: > >>> > >>> > >>> It's worth noting that the check in pgdat_is_empty() is slightly > >>> different. I *think* it doesn't matter in practice, yet I wonder if we > >>> should simply fixup (currently unused) pgdat_is_empty(). > >> > >> I guess we could change it to > >> > >> static inline bool pgdat_is_empty(pg_data_t *pgdat) > >> { > >> return node_start_pfn(pgdat->node_id) == node_end_pfn(pgdat->node_id) > >> } > >> > >> ? And maybe even rename it to to node_is_empty (not sure why but I tend to like > > > > At least I like this name (node_is_empty) as well. > > > > Let's try keeping it consistent. I think node_is_empty() might indicate > that we're punching in a node id instead of a pgdat. > I suspect Oscar will change the argument to "nid" as well, like: static inline bool node_is_empty(int nid) { return node_start_pfn(nid) == node_end_pfn(nid); } Does this look good? Thanks. > > -- > Thanks, > > David / dhildenb > >
On 22.06.22 10:54, Muchun Song wrote: > On Wed, Jun 22, 2022 at 10:31:12AM +0200, David Hildenbrand wrote: >> On 22.06.22 05:56, Muchun Song wrote: >>> On Wed, Jun 22, 2022 at 05:47:22AM +0200, Oscar Salvador wrote: >>>> On Tue, Jun 21, 2022 at 09:44:47AM +0200, David Hildenbrand wrote: >>>>> >>>>> >>>>> It's worth noting that the check in pgdat_is_empty() is slightly >>>>> different. I *think* it doesn't matter in practice, yet I wonder if we >>>>> should simply fixup (currently unused) pgdat_is_empty(). >>>> >>>> I guess we could change it to >>>> >>>> static inline bool pgdat_is_empty(pg_data_t *pgdat) >>>> { >>>> return node_start_pfn(pgdat->node_id) == node_end_pfn(pgdat->node_id) >>>> } >>>> >>>> ? And maybe even rename it to to node_is_empty (not sure why but I tend to like >>> >>> At least I like this name (node_is_empty) as well. >>> >> >> Let's try keeping it consistent. I think node_is_empty() might indicate >> that we're punching in a node id instead of a pgdat. >> > > I suspect Oscar will change the argument to "nid" as well, like: > > static inline bool node_is_empty(int nid) > { > return node_start_pfn(nid) == node_end_pfn(nid); > } > > Does this look good? Then we have to lookup the pgdat multiple times for (IMHO) no real compelling reason.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index e008a3df0485..2b9b2422ba32 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -7361,6 +7361,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 (pgdat_is_empty(pgdat)) + 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; @@ -7393,6 +7397,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); @@ -7610,6 +7615,12 @@ 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) { + zone_init_internals(zone, j, nid, 0); + continue; + } + /* * Adjust freesize so that it accounts for how much memory * is used by this zone for memmap. This affects the watermark @@ -7647,9 +7658,6 @@ static void __init free_area_init_core(struct pglist_data *pgdat) */ zone_init_internals(zone, j, nid, freesize); - if (!size) - continue; - set_pageblock_order(); setup_usemap(zone); init_currently_empty_zone(zone, zone->zone_start_pfn, size); @@ -7730,7 +7738,7 @@ static void __init free_area_init_node(int nid) pgdat->node_start_pfn = start_pfn; pgdat->per_cpu_nodestats = NULL; - if (start_pfn != end_pfn) { + if (!pgdat_is_empty(pgdat)) { pr_info("Initmem setup node %d [mem %#018Lx-%#018Lx]\n", nid, (u64)start_pfn << PAGE_SHIFT, end_pfn ? ((u64)end_pfn << PAGE_SHIFT) - 1 : 0);
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 | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)