linux-kernel - Re: [PATCH 1/3] mm/page_alloc: Do not calculate node's total pages and memmap pages when empty

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YnOUPoqM3RncmraS@localhost.localdomain>
Date:   Thu, 5 May 2022 11:09:18 +0200
From:   Oscar Salvador <osalvador@...e.de>
To:     David Hildenbrand <david@...hat.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Michal Hocko <mhocko@...nel.org>,
        Wei Yang <richard.weiyang@...il.com>,
        Miaohe Lin <linmiaohe@...wei.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] mm/page_alloc: Do not calculate node's total pages
 and memmap pages when empty

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