Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456953 > unrolled thread

Re: [PATCH 03/34] mm, vmscan: move LRU lists to node

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 03/34] mm, vmscan: move LRU lists to node Mel Gorman <mgorman@techsingularity.net> - 2016-08-05 10:50 +0200
    Re: [PATCH 03/34] mm, vmscan: move LRU lists to node James Hogan <james.hogan@imgtec.com> - 2016-08-05 13:00 +0200
      Re: [PATCH 03/34] mm, vmscan: move LRU lists to node Mel Gorman <mgorman@techsingularity.net> - 2016-08-05 14:00 +0200
        Re: [PATCH 03/34] mm, vmscan: move LRU lists to node James Hogan <james.hogan@imgtec.com> - 2016-08-05 14:10 +0200

#1456953 — Re: [PATCH 03/34] mm, vmscan: move LRU lists to node

On Thu, Aug 04, 2016 at 09:59:17PM +0100, James Hogan wrote:
> > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> 
> This breaks boot on metag architecture:
> Oops: err 0007 (Data access general read/write fault) addr 00233008 [#1]
> 
> It appears to be in node_page_state_snapshot() (via
> pgdat_reclaimable()), and have come via mm_init. Here's the relevant
> bit of the backtrace:
> 
>     node_page_state_snapshot@0x4009c884(enum node_stat_item item =
> ???, struct pglist_data * pgdat = ???) + 0x48
>     pgdat_reclaimable(struct pglist_data * pgdat = 0x402517a0)
>     show_free_areas(unsigned int filter = 0) + 0x2cc
>     show_mem(unsigned int filter = 0) + 0x18
>     mm_init@0x4025c3d4()
>     start_kernel() + 0x204
> 
> __per_cpu_offset[0] == 0x233000 (close to bad addr),
> pgdat->per_cpu_nodestats = NULL. and setup_per_cpu_pageset()
> definitely hasn't been called yet (mm_init is called before
> setup_per_cpu_pageset()).
> 
> Any ideas what the correct solution is (and why presumably others
> haven't seen the same issue on other architectures?).
> 

metag calls show_mem in mem_init() before the pagesets are initialised.
What's surprising is that it worked for the zone stats as it appears
that calling zone_reclaimable() from that context should also have
broken. Did anything change recently that would have avoided the
zone->pageset dereference in zone_reclaimable() before?

The easiest option would be to not call show_mem from arch code until
after the pagesets are setup.

-- 
Mel Gorman
SUSE Labs

[toc] | [next] | [standalone]

#1457025

[Multipart message — attachments visible in raw view] — view raw

On Fri, Aug 05, 2016 at 09:41:15AM +0100, Mel Gorman wrote:
> On Thu, Aug 04, 2016 at 09:59:17PM +0100, James Hogan wrote:
> > > Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > 
> > This breaks boot on metag architecture:
> > Oops: err 0007 (Data access general read/write fault) addr 00233008 [#1]
> > 
> > It appears to be in node_page_state_snapshot() (via
> > pgdat_reclaimable()), and have come via mm_init. Here's the relevant
> > bit of the backtrace:
> > 
> >     node_page_state_snapshot@0x4009c884(enum node_stat_item item =
> > ???, struct pglist_data * pgdat = ???) + 0x48
> >     pgdat_reclaimable(struct pglist_data * pgdat = 0x402517a0)
> >     show_free_areas(unsigned int filter = 0) + 0x2cc
> >     show_mem(unsigned int filter = 0) + 0x18
> >     mm_init@0x4025c3d4()
> >     start_kernel() + 0x204
> > 
> > __per_cpu_offset[0] == 0x233000 (close to bad addr),
> > pgdat->per_cpu_nodestats = NULL. and setup_per_cpu_pageset()
> > definitely hasn't been called yet (mm_init is called before
> > setup_per_cpu_pageset()).
> > 
> > Any ideas what the correct solution is (and why presumably others
> > haven't seen the same issue on other architectures?).
> > 
> 
> metag calls show_mem in mem_init() before the pagesets are initialised.

Indeed, I didn't spot yesterday evening that this appears to be
different to other arches.

> What's surprising is that it worked for the zone stats as it appears
> that calling zone_reclaimable() from that context should also have
> broken. Did anything change recently that would have avoided the
> zone->pageset dereference in zone_reclaimable() before?

It appears that zone_pcp_init() was already setting zone->pageset to
&boot_pageset, via paging_init():

zone_pcp_init@0x40265d54(struct zone * zone = ???)
free_area_init_core@0x40265c18(struct pglist_data * pgdat = ???) + 0x138
free_area_init_node(int nid = 0, unsigned long * zones_size = ???, unsigned long node_start_pfn = ???, unsigned long * zholes_size = ???) + 0x1a0
free_area_init_nodes(unsigned long * max_zone_pfn = ???) + 0x440
paging_init(unsigned long mem_end = 0x4fe00000) + 0x378
setup_arch(char ** cmdline_p = 0x4024e038) + 0x2b8
start_kernel() + 0x54

setup_arch() is called prior to mm_init(), which explains why it wasn't
crashing before.

> The easiest option would be to not call show_mem from arch code until
> after the pagesets are setup.

Since no other arches seem to do show_mem earily during boot like metag,
and doing so doesn't really add much value, I'm happy to remove it
anyway.

However could your change break other things and need fixing anyway?

Thanks!
James

[toc] | [prev] | [next] | [standalone]

#1457055

On Fri, Aug 05, 2016 at 11:52:57AM +0100, James Hogan wrote:
> > What's surprising is that it worked for the zone stats as it appears
> > that calling zone_reclaimable() from that context should also have
> > broken. Did anything change recently that would have avoided the
> > zone->pageset dereference in zone_reclaimable() before?
> 
> It appears that zone_pcp_init() was already setting zone->pageset to
> &boot_pageset, via paging_init():
> 

/me slaps self

Of course.

> > The easiest option would be to not call show_mem from arch code until
> > after the pagesets are setup.
> 
> Since no other arches seem to do show_mem earily during boot like metag,
> and doing so doesn't really add much value, I'm happy to remove it
> anyway.
> 

Thanks. Can I assume you'll merge such a patch or should I roll one?

> However could your change break other things and need fixing anyway?
> 

Not that I'm aware of. There would have to be a node-based stat that has
meaning that early in boot to have an effect. If one happened to added
then it would need fixing but until then the complexity is unnecessary.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1457057

[Multipart message — attachments visible in raw view] — view raw

On Fri, Aug 05, 2016 at 12:55:26PM +0100, Mel Gorman wrote:
> On Fri, Aug 05, 2016 at 11:52:57AM +0100, James Hogan wrote:
> > > What's surprising is that it worked for the zone stats as it appears
> > > that calling zone_reclaimable() from that context should also have
> > > broken. Did anything change recently that would have avoided the
> > > zone->pageset dereference in zone_reclaimable() before?
> > 
> > It appears that zone_pcp_init() was already setting zone->pageset to
> > &boot_pageset, via paging_init():
> > 
> 
> /me slaps self
> 
> Of course.
> 
> > > The easiest option would be to not call show_mem from arch code until
> > > after the pagesets are setup.
> > 
> > Since no other arches seem to do show_mem earily during boot like metag,
> > and doing so doesn't really add much value, I'm happy to remove it
> > anyway.
> > 
> 
> Thanks. Can I assume you'll merge such a patch or should I roll one?

Yep, I'll take care of it.

> 
> > However could your change break other things and need fixing anyway?
> > 
> 
> Not that I'm aware of. There would have to be a node-based stat that has
> meaning that early in boot to have an effect. If one happened to added
> then it would need fixing but until then the complexity is unnecessary.

Okay, thanks for the help,

Cheers
James

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web