Re: [PATCH] mm: disable kernelcore=mirror when no mirror memory

[Mike Rapoport <rppt@kernel.org>]

On Mon, Jul 31, 2023 at 09:39:37AM +0800, Kefeng Wang wrote:
> 
> On 2023/7/30 14:53, Mike Rapoport wrote:
> > On Sat, Jul 29, 2023 at 04:57:17PM +0800, Kefeng Wang wrote:
> > > 
> > > 
> > > On 2023/7/29 16:12, Mike Rapoport wrote:
> > > > On Fri, Jul 28, 2023 at 12:01:24PM +0800, Wupeng Ma wrote:
> > > > > From: Ma Wupeng <mawupeng1@huawei.com>
> > > > > 
> > > > > For system with kernelcore=mirror enabled while no mirrored memory is
> > > > > reported by efi. This could lead to kernel OOM during startup since
> > > > > all memory beside zone DMA are in the movable zone and this prevents
> > > > > the kernel to use it.
> > > > > 
> > > > > Zone DMA/DMA32 initialization is independent of mirrored memory and
> > > > > their max pfn is set in zone_sizes_init(). Since kernel can fallback
> > > > > to zone DMA/DMA32 if there is no memory in zone Normal, these zones
> > > > > are seen as mirrored memory no mather their memory attributes are.
> > > > 
> > > > Using kernelcore= and movablecore= always come with the risk there will be
> > > > to little memory for the kernel to use. Even if EFI reports mirrored memory
> > > > it's possible to have OOM with kernelcore=mirror because there could be
> > > > just not enough mirrored memory.
> > > 
> > > 
> > > Only x86/arm64 support kernelcore=mirror, system_has_some_mirror is
> > > false by default, so it should no issue for now, but it is better to
> > > move this check into find_zone_movable_pfns_for_nodes().
> >
> > Sorry, I meant that system_has_some_mirror is false by default, and both
> > x86/arm64 parse early parameters before they set up memblock, so
> > system_has_some_mirror will be always false at this point.
> 
> Clear, so let's move check into find_zone_movable_pfns_for_nodes()(no test),
> is this ok?

This looks fine to me, just one nit below.
 
> diff --git a/mm/internal.h b/mm/internal.h
> index a7d9e980429a..1599becc9079 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -1005,6 +1005,7 @@ static inline bool gup_must_unshare(struct
> vm_area_struct *vma,
>  }
> 
>  extern bool mirrored_kernelcore;
> +extern bool system_has_some_mirror;

I prefer keeping system_has_some_mirror static and adding, say,
memblock_has_mirror(). 
 
>  static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma)
>  {
> diff --git a/mm/memblock.c b/mm/memblock.c
> index f9e61e565a53..e7a7a65415fb 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -156,10 +156,10 @@ static __refdata struct memblock_type *memblock_memory
> = &memblock.memory;
>  	} while (0)
> 
>  static int memblock_debug __initdata_memblock;
> -static bool system_has_some_mirror __initdata_memblock;
>  static int memblock_can_resize __initdata_memblock;
>  static int memblock_memory_in_slab __initdata_memblock;
>  static int memblock_reserved_in_slab __initdata_memblock;
> +bool system_has_some_mirror __initdata_memblock;
> 
>  static enum memblock_flags __init_memblock choose_memblock_flags(void)
>  {
> diff --git a/mm/mm_init.c b/mm/mm_init.c
> index a1963c3322af..c444da6065a6 100644
> --- a/mm/mm_init.c
> +++ b/mm/mm_init.c
> @@ -377,6 +377,11 @@ static void __init
> find_zone_movable_pfns_for_nodes(void)
>  	if (mirrored_kernelcore) {
>  		bool mem_below_4gb_not_mirrored = false;
> 
> +		if (!system_has_some_mirror) {
> +			pr_warn("The system has no mirror memory, ignore kernelcore=mirror.\n");
> +			goto out;
> +		}
> +
>  		for_each_mem_region(r) {
>  			if (memblock_is_mirror(r))
>  				continue;
> 

-- 
Sincerely yours,
Mike.