Re: WARNING: at mm/page_alloc.c:4514 free_area_init_node+0x4f/0x37b()

From: Minchan Kim
Date: Sun Aug 05 2012 - 20:01:18 EST


Hi Borislav,

On Thu, Aug 02, 2012 at 01:06:41PM +0200, Borislav Petkov wrote:
> On Thu, Aug 02, 2012 at 08:33:35AM +0900, Minchan Kim wrote:
> > Hello Borislav,
> >
> > On Wed, Aug 01, 2012 at 07:38:37PM +0200, Borislav Petkov wrote:
> > > Hi,
> > >
> > > I'm hitting the WARN_ON in $Subject with latest linus:
> > > v3.5-8833-g2d534926205d on a 4-node AMD system. As it looks from
> > > dmesg, it is happening on node 0, 1 and 2 but not on 3. Probably the
> > > pgdat->nr_zones thing but I'll have to add more dbg code to be sure.
> >
> > As I look the code quickly, free_area_init_node initializes node_id and
> > node_start_pfn doublely. They were initialized by setup_node_data.
> >
> > Could you test below patch? It's not a totally right way to fix it but
> > I want to confirm why it happens.
> >
> > (I'm on vacation now so please understand that it hard to reach me)
>
> I sincerely hope you're not going to interrupt your vacation because of
> this.
>
> :-).
>
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 889532b..009ac28 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -4511,7 +4511,7 @@ void __paginginit free_area_init_node(int nid, unsigned long *zones_size,
> > pg_data_t *pgdat = NODE_DATA(nid);
> >
> > /* pg_data_t should be reset to zero when it's allocated */
> > - WARN_ON(pgdat->nr_zones || pgdat->node_start_pfn || pgdat->classzone_idx);
> > + WARN_ON(pgdat->nr_zones || pgdat->classzone_idx);
> >
> > pgdat->node_id = nid;
> > pgdat->node_start_pfn = node_start_pfn;
>
> Yep, you were right: ->node_start_pfn is set. I added additional debug
> output for more info:
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 889532b8e6c1..c249abe4fee2 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4511,7 +4511,17 @@ void __paginginit free_area_init_node(int nid, unsigned long *zones_size,
> pg_data_t *pgdat = NODE_DATA(nid);
>
> /* pg_data_t should be reset to zero when it's allocated */
> - WARN_ON(pgdat->nr_zones || pgdat->node_start_pfn || pgdat->classzone_idx);
> + WARN_ON(pgdat->nr_zones || pgdat->classzone_idx);
> +
> + if (pgdat->node_start_pfn)
> + pr_warn("%s: pgdat->node_start_pfn: %lu\n", __func__, pgdat->node_start_pfn);
> +
> + if (pgdat->nr_zones)
> + pr_warn("%s: pgdat->nr_zones: %d\n", __func__, pgdat->nr_zones);
> +
> + if (pgdat->classzone_idx)
> + pr_warn("%s: pgdat->classzone_idx: %d\n", __func__, pgdat->classzone_idx);
> +
>
> pgdat->node_id = nid;
> pgdat->node_start_pfn = node_start_pfn;
>
>
>
> Here's what it says:
>
> [ 0.000000] On node 0 totalpages: 4193848
> [ 0.000000] DMA zone: 64 pages used for memmap
> [ 0.000000] DMA zone: 6 pages reserved
> [ 0.000000] DMA zone: 3890 pages, LIFO batch:0
> [ 0.000000] DMA32 zone: 16320 pages used for memmap
> [ 0.000000] DMA32 zone: 798464 pages, LIFO batch:31
> [ 0.000000] Normal zone: 52736 pages used for memmap
> [ 0.000000] Normal zone: 3322368 pages, LIFO batch:31
> [ 0.000000] free_area_init_node: pgdat->node_start_pfn: 4423680 <----
> [ 0.000000] On node 1 totalpages: 4194304
> [ 0.000000] Normal zone: 65536 pages used for memmap
> [ 0.000000] Normal zone: 4128768 pages, LIFO batch:31
> [ 0.000000] free_area_init_node: pgdat->node_start_pfn: 8617984 <----
> [ 0.000000] On node 2 totalpages: 4194304
> [ 0.000000] Normal zone: 65536 pages used for memmap
> [ 0.000000] Normal zone: 4128768 pages, LIFO batch:31
> [ 0.000000] free_area_init_node: pgdat->node_start_pfn: 12812288 <----
> [ 0.000000] On node 3 totalpages: 4194304
> [ 0.000000] Normal zone: 65536 pages used for memmap
> [ 0.000000] Normal zone: 4128768 pages, LIFO batch:31
> [ 0.000000] ACPI: PM-Timer IO Port: 0x2008
> [ 0.000000] ACPI: Local APIC address 0xfee00000
>
> Thanks.

Thanks for looking at this!

As soon as I come back from vacation, I see this BUG carefully and think patch I sent
is good. The patch's goal is to detect for uninitialized pgdat structure
when it was allocated. So it checks some variables randomly but unfortunately,
pgdat's members like node_start_pfn are closely related to boot arch code
so some members could be used by arch code before reaching generic mm code.
It was a Tejun's concern and he was correct.

I think nr_zones and classzone_idx should be initialized by only generic MM code
during boot sequence, not memory hotplug so that patch would be okay.

Linus already applied the patch in rc-1 but he might need better changelog.
I am not sure I send this patch to whom, Linus or Andrew?
Anyway, Please use below if really need it.

Thanks!