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 <[email protected]>
---
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 16 July 2018 at 16:16, Pavel Tatashin <[email protected]> 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 <[email protected]>
Tested-by: Matt Hart <[email protected]>
> ---
> 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
>
--
Matt Hart
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 <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> 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
>
--
Michal Hocko
SUSE Labs
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 <[email protected]>
>
> Acked-by: Michal Hocko <[email protected]>
>
Thank you.
Pavel