Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 5 May 2022 11:09:18 +0200
From:   Oscar Salvador <osalvador@suse.de>
To:     David Hildenbrand <david@redhat.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        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
Message-ID: <YnOUPoqM3RncmraS@localhost.localdomain>
References: <20220307150725.6810-1-osalvador@suse.de>
 <20220307150725.6810-2-osalvador@suse.de>
 <f31caf6a-fb13-0be3-9fa2-0b4959cc0810@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f31caf6a-fb13-0be3-9fa2-0b4959cc0810@redhat.com>
Precedence: bulk

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