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!
--
Oscar Salvador
SUSE Labs
