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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

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.
>>
>> Yes, this is a big problem, could we add an option to move some
>> ZONE_MOVABLE pages into ZONE_NORMAL(MIGRATE_MOVABLE) when low free
>> memory?>
>>>> To solve this problem, disable kernelcore=mirror when there is no real
>>>> mirrored memory exists.
>>>>
>>>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>>>> ---
>>>>    mm/internal.h | 2 ++
>>>>    mm/memblock.c | 2 +-
>>>>    mm/mm_init.c  | 6 +++++-
>>>>    3 files changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/internal.h b/mm/internal.h
>>>> index a7d9e980429a..98a03ac74ca7 100644
>>>> --- a/mm/internal.h
>>>> +++ b/mm/internal.h
>>>> @@ -374,6 +374,8 @@ static inline void clear_zone_contiguous(struct zone *zone)
>>>>    	zone->contiguous = false;
>>>>    }
>>>> +extern bool system_has_some_mirror;
>>>> +
>>>>    extern int __isolate_free_page(struct page *page, unsigned int order);
>>>>    extern void __putback_isolated_page(struct page *page, unsigned int order,
>>>>    				    int mt);
>>>> 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..6267b9f75927 100644
>>>> --- a/mm/mm_init.c
>>>> +++ b/mm/mm_init.c
>>>> @@ -269,7 +269,11 @@ static int __init cmdline_parse_kernelcore(char *p)
>>>>    {
>>>>    	/* parse kernelcore=mirror */
>>>>    	if (parse_option_str(p, "mirror")) {
>>>> -		mirrored_kernelcore = true;
>>>> +		if (system_has_some_mirror)
>>>> +			mirrored_kernelcore = true;
>>>
>>> On many architectures early parameters are parsed before memblock is setup,
>>> so system_has_some_mirror will always be true.
>>
>> 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?

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;

  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;