Re: [PATCH v3 07/26] mm: drop CONFIG_HAVE_ARCH_NODEDATA_EXTENSION

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 2 Aug 2024 10:49:22 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> > --- a/mm/mm_init.c
> > +++ b/mm/mm_init.c
> > @@ -1838,11 +1838,10 @@ void __init free_area_init(unsigned long *max_zone_pfn)
> >  
> >  		if (!node_online(nid)) {
> >  			/* Allocator not initialized yet */
> > -			pgdat = arch_alloc_nodedata(nid);
> > +			pgdat = memblock_alloc(sizeof(*pgdat), SMP_CACHE_BYTES);
> >  			if (!pgdat)
> >  				panic("Cannot allocate %zuB for node %d.\n",
> >  				       sizeof(*pgdat), nid);
> > -			arch_refresh_nodedata(nid, pgdat);
> 
> This allocates pgdat but never sets node_data[nid] to it
> and promptly leaks it on the line below. 
> 
> Just to sanity check this I spun up a qemu machine with no memory
> initially present on some nodes and it went boom as you'd expect.
> 
> I tested with addition of
> 			NODE_DATA(nid) = pgdat;
> and it all seems to work as expected.

Thanks, I added that.  It blew up on x86_64 allnoconfig because
node_data[] (and hence NODE_DATA()) isn't an lvalue when CONFIG_NUMA=n.

I'll put some #ifdef CONFIG_NUMAs in there for now but

a) NODE_DATA() is upper-case. Implies "constant".  Shouldn't be assigned to.

b) NODE_DATA() should be non-lvalue when CONFIG_NUMA=y also.  But no,
   we insist on implementing things in cpp instead of in C.

c) In fact assigning to anything which ends in "()" is nuts.  Please
   clean up my tempfix.

c) Mike, generally I'm wondering if there's a bunch of code here
   which isn't needed on CONFIG_NUMA=n.  Please check all of this for
   unneeded bloatiness.