Re: [PATCH v3 1/2] arm64: kdump: Provide default size when crashkernel=Y,low is not specified

[Baoquan He]

On 08/02/22 at 06:12pm, Leizhen (ThunderTown) wrote:
......snip...  
> >> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> >> index 339ee84e5a61a0b..5390f361208ccf7 100644
> >> --- a/arch/arm64/mm/init.c
> >> +++ b/arch/arm64/mm/init.c
> >> @@ -96,6 +96,14 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit = PHYS_MASK + 1;
> >>  #define CRASH_ADDR_LOW_MAX		arm64_dma_phys_limit
> >>  #define CRASH_ADDR_HIGH_MAX		(PHYS_MASK + 1)
> >>  
> >> +/*
> >> + * This is an empirical value in x86_64 and taken here directly. Please
> >> + * refer to the code comment in reserve_crashkernel_low() of x86_64 for more
> >> + * details.
> > 
> > Honestly, I read that comment and I'm none the wiser. What does "due to
> > mapping restrictions" mean? The remainder of the comment appears to be
> 
> Because the comments you read is addressed to CRASH_ADDR_LOW_MAX, not
> for DEFAULT_CRASH_KERNEL_LOW_SIZE. Please see the following patch:
> 
> 94fb9334182284 x86/crash: Allocate enough low memory when crashkernel=high
> 
> > specific to x86 and refers to jump ranges with 5-level page-tables.
> > 
> >> +#define DEFAULT_CRASH_KERNEL_LOW_SIZE	\
> >> +	max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20)
> > 
> > So why does this value make sense for arm64? We have considerable platform
> > fragmentation^Wdiversity compared to x86 and picking a one-size-fits-all
> > default is more likely to cause weird problems when it doesn't work imo. I'd
> > actually prefer that the default is something that fails obviously (e.g. 0)
> > and we force an appropriate size to be specified.

The default value mainly serves the crashkernel=xM case, not just for
crashkernel=,high specified while no crahskernel=,low. The simplest
usage of crashkernel reservation is user only need set crashkernel=xM in
cmdline, we will try to get available memory from low memory region
firstly, if failed, go above 4G to find again. If we finally get memory
from above 4G, then the default low memory is needed. E.g if
crashkernel=512M is set, and no sufficient memory under 4G. With it,
user don't need to know about crashkernel=,high or ,low, and even memory
has type of high, low, or dma stuff.

> > 
> > On the other hand, if you can convince me that having a global constant is
> > the right way forward, then please move this out of the arch code.
> 
> Yes, the default value may not be the same as that of x86. For example,
> 128 MB may be sufficient.
> 
> So we need to discuss first, do we need a default value? Personally, I
> don't think it hurts.

Yes, we can discuss. Welcome anyone to help provide information how we
should take care to make a small but enough value. In fact, on x86_64,
we didn't set the value at one time. It was set as 72M at the beginning,
later Joerg found it's not enough, we finally decided to make it as
256M. People who mind the wasting of 256M can use crashkernel=,high and
crashkernel=,low pair to specify the value exactly.

commit c729de8fcea3 ("x86, kdump: Set crashkernel_low automatically")
commit 94fb93341822 ("x86/crash: Allocate enough low memory when crashkernel=high")

Thanks
Baoquan