| Message ID | 20180716151630.770-1-pasha.tatashin@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16 July 2018 at 16:16, Pavel Tatashin <pasha.tatashin@oracle.com> wrote: > Moving zero_resv_unavail before memmap_init_zone(), caused a regression on > x86-32. Automated bisection on my boards in KernelCI spotted this regression, so I did a manual test with this patch and confirm it fixes boots on my i386 devices. > > The cause is that we access struct pages before they are allocated when > CONFIG_FLAT_NODE_MEM_MAP is used. > > free_area_init_nodes() > zero_resv_unavail() > mm_zero_struct_page(pfn_to_page(pfn)); <- struct page is not alloced > free_area_init_node() > if CONFIG_FLAT_NODE_MEM_MAP > alloc_node_mem_map() > memblock_virt_alloc_node_nopanic() <- struct page alloced here > > On the other hand memblock_virt_alloc_node_nopanic() zeroes all the memory > that it returns, so we do not need to do zero_resv_unavail() here. > > Fixes: e181ae0c5db9 ("mm: zero unavailable pages before memmap init") > Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com> Tested-by: Matt Hart <matt@mattface.org> > --- > include/linux/mm.h | 2 +- > mm/page_alloc.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index a0fbb9ffe380..3982c83fdcbf 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2132,7 +2132,7 @@ extern int __meminit __early_pfn_to_nid(unsigned long pfn, > struct mminit_pfnnid_cache *state); > #endif > > -#ifdef CONFIG_HAVE_MEMBLOCK > +#if defined(CONFIG_HAVE_MEMBLOCK) && !defined(CONFIG_FLAT_NODE_MEM_MAP) > void zero_resv_unavail(void); > #else > static inline void zero_resv_unavail(void) {} > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 5d800d61ddb7..a790ef4be74e 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -6383,7 +6383,7 @@ void __paginginit free_area_init_node(int nid, unsigned long *zones_size, > free_area_init_core(pgdat); > } > > -#ifdef CONFIG_HAVE_MEMBLOCK > +#if defined(CONFIG_HAVE_MEMBLOCK) && !defined(CONFIG_FLAT_NODE_MEM_MAP) > /* > * Only struct pages that are backed by physical memory are zeroed and > * initialized by going through __init_single_page(). But, there are some > @@ -6421,7 +6421,7 @@ void __paginginit zero_resv_unavail(void) > if (pgcnt) > pr_info("Reserved but unavailable: %lld pages", pgcnt); > } > -#endif /* CONFIG_HAVE_MEMBLOCK */ > +#endif /* CONFIG_HAVE_MEMBLOCK && !CONFIG_FLAT_NODE_MEM_MAP */ > > #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP > > -- > 2.18.0 >
On Mon 16-07-18 11:16:30, Pavel Tatashin wrote: > Moving zero_resv_unavail before memmap_init_zone(), caused a regression on > x86-32. > > The cause is that we access struct pages before they are allocated when > CONFIG_FLAT_NODE_MEM_MAP is used. > > free_area_init_nodes() > zero_resv_unavail() > mm_zero_struct_page(pfn_to_page(pfn)); <- struct page is not alloced > free_area_init_node() > if CONFIG_FLAT_NODE_MEM_MAP > alloc_node_mem_map() > memblock_virt_alloc_node_nopanic() <- struct page alloced here > > On the other hand memblock_virt_alloc_node_nopanic() zeroes all the memory > that it returns, so we do not need to do zero_resv_unavail() here. This all is subtle as hell and almost impossible to build a sane code on top. Your patch sounds good as a stop gap fix but we really need something resembling an actual design rather than ad-hoc hacks piled on top of each other. > Fixes: e181ae0c5db9 ("mm: zero unavailable pages before memmap init") > Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com> Acked-by: Michal Hocko <mhocko@suse.com> > --- > include/linux/mm.h | 2 +- > mm/page_alloc.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index a0fbb9ffe380..3982c83fdcbf 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2132,7 +2132,7 @@ extern int __meminit __early_pfn_to_nid(unsigned long pfn, > struct mminit_pfnnid_cache *state); > #endif > > -#ifdef CONFIG_HAVE_MEMBLOCK > +#if defined(CONFIG_HAVE_MEMBLOCK) && !defined(CONFIG_FLAT_NODE_MEM_MAP) > void zero_resv_unavail(void); > #else > static inline void zero_resv_unavail(void) {} > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 5d800d61ddb7..a790ef4be74e 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -6383,7 +6383,7 @@ void __paginginit free_area_init_node(int nid, unsigned long *zones_size, > free_area_init_core(pgdat); > } > > -#ifdef CONFIG_HAVE_MEMBLOCK > +#if defined(CONFIG_HAVE_MEMBLOCK) && !defined(CONFIG_FLAT_NODE_MEM_MAP) > /* > * Only struct pages that are backed by physical memory are zeroed and > * initialized by going through __init_single_page(). But, there are some > @@ -6421,7 +6421,7 @@ void __paginginit zero_resv_unavail(void) > if (pgcnt) > pr_info("Reserved but unavailable: %lld pages", pgcnt); > } > -#endif /* CONFIG_HAVE_MEMBLOCK */ > +#endif /* CONFIG_HAVE_MEMBLOCK && !CONFIG_FLAT_NODE_MEM_MAP */ > > #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP > > -- > 2.18.0 >
On 07/16/2018 12:09 PM, Michal Hocko wrote: > On Mon 16-07-18 11:16:30, Pavel Tatashin wrote: >> Moving zero_resv_unavail before memmap_init_zone(), caused a regression on >> x86-32. >> >> The cause is that we access struct pages before they are allocated when >> CONFIG_FLAT_NODE_MEM_MAP is used. >> >> free_area_init_nodes() >> zero_resv_unavail() >> mm_zero_struct_page(pfn_to_page(pfn)); <- struct page is not alloced >> free_area_init_node() >> if CONFIG_FLAT_NODE_MEM_MAP >> alloc_node_mem_map() >> memblock_virt_alloc_node_nopanic() <- struct page alloced here >> >> On the other hand memblock_virt_alloc_node_nopanic() zeroes all the memory >> that it returns, so we do not need to do zero_resv_unavail() here. > > This all is subtle as hell and almost impossible to build a sane code on > top. Your patch sounds good as a stop gap fix but we really need > something resembling an actual design rather than ad-hoc hacks piled on > top of each other.z I totally agree, I started working on figuring out how to simply and improve memmap_init_zone(). But part of the mess is that we simply have too many memmap layouts. We must start removing some of them. But, that also requires an analysis of how to unify them. > >> Fixes: e181ae0c5db9 ("mm: zero unavailable pages before memmap init") >> Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com> > > Acked-by: Michal Hocko <mhocko@suse.com> > Thank you. Pavel
diff --git a/include/linux/mm.h b/include/linux/mm.h index a0fbb9ffe380..3982c83fdcbf 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2132,7 +2132,7 @@ extern int __meminit __early_pfn_to_nid(unsigned long pfn, struct mminit_pfnnid_cache *state); #endif -#ifdef CONFIG_HAVE_MEMBLOCK +#if defined(CONFIG_HAVE_MEMBLOCK) && !defined(CONFIG_FLAT_NODE_MEM_MAP) void zero_resv_unavail(void); #else static inline void zero_resv_unavail(void) {} diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 5d800d61ddb7..a790ef4be74e 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -6383,7 +6383,7 @@ void __paginginit free_area_init_node(int nid, unsigned long *zones_size, free_area_init_core(pgdat); } -#ifdef CONFIG_HAVE_MEMBLOCK +#if defined(CONFIG_HAVE_MEMBLOCK) && !defined(CONFIG_FLAT_NODE_MEM_MAP) /* * Only struct pages that are backed by physical memory are zeroed and * initialized by going through __init_single_page(). But, there are some @@ -6421,7 +6421,7 @@ void __paginginit zero_resv_unavail(void) if (pgcnt) pr_info("Reserved but unavailable: %lld pages", pgcnt); } -#endif /* CONFIG_HAVE_MEMBLOCK */ +#endif /* CONFIG_HAVE_MEMBLOCK && !CONFIG_FLAT_NODE_MEM_MAP */ #ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP
Moving zero_resv_unavail before memmap_init_zone(), caused a regression on x86-32. The cause is that we access struct pages before they are allocated when CONFIG_FLAT_NODE_MEM_MAP is used. free_area_init_nodes() zero_resv_unavail() mm_zero_struct_page(pfn_to_page(pfn)); <- struct page is not alloced free_area_init_node() if CONFIG_FLAT_NODE_MEM_MAP alloc_node_mem_map() memblock_virt_alloc_node_nopanic() <- struct page alloced here On the other hand memblock_virt_alloc_node_nopanic() zeroes all the memory that it returns, so we do not need to do zero_resv_unavail() here. Fixes: e181ae0c5db9 ("mm: zero unavailable pages before memmap init") Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com> --- include/linux/mm.h | 2 +- mm/page_alloc.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)