| Message ID | 20220711090319.1604-2-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: kdump: Function supplement and performance optimization | expand |
On Mon, Jul 11, 2022 at 05:03:18PM +0800, Zhen Lei wrote: > To be consistent with the implementation of x86 and improve cross-platform > user experience. Try to allocate at least 256 MiB low memory automatically > for the case that crashkernel=,high is explicitly specified, while > crashkenrel=,low is omitted. > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > Acked-by: Baoquan He <bhe@redhat.com> > --- > Documentation/admin-guide/kernel-parameters.txt | 8 +------- > arch/arm64/mm/init.c | 12 +++++++++++- > 2 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 2522b11e593f239..65a2c3a22a4b57d 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -843,7 +843,7 @@ > available. > It will be ignored if crashkernel=X is specified. > crashkernel=size[KMG],low > - [KNL, X86-64] range under 4G. When crashkernel=X,high > + [KNL, X86-64, ARM64] range under 4G. When crashkernel=X,high > is passed, kernel could allocate physical memory region > above 4G, that cause second kernel crash on system > that require some amount of low memory, e.g. swiotlb > @@ -857,12 +857,6 @@ > It will be ignored when crashkernel=X,high is not used > or memory reserved is below 4G. > > - [KNL, ARM64] range in low memory. > - This one lets the user specify a low range in the > - DMA zone for the crash dump kernel. > - It will be ignored when crashkernel=X,high is not used > - or memory reserved is located in the DMA zones. > - > cryptomgr.notests > [KNL] Disable crypto self-tests > > 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 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. 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. Will
On 2022/8/2 16:37, Will Deacon wrote: > On Mon, Jul 11, 2022 at 05:03:18PM +0800, Zhen Lei wrote: >> To be consistent with the implementation of x86 and improve cross-platform >> user experience. Try to allocate at least 256 MiB low memory automatically >> for the case that crashkernel=,high is explicitly specified, while >> crashkenrel=,low is omitted. >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> Acked-by: Baoquan He <bhe@redhat.com> >> --- >> Documentation/admin-guide/kernel-parameters.txt | 8 +------- >> arch/arm64/mm/init.c | 12 +++++++++++- >> 2 files changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >> index 2522b11e593f239..65a2c3a22a4b57d 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -843,7 +843,7 @@ >> available. >> It will be ignored if crashkernel=X is specified. >> crashkernel=size[KMG],low >> - [KNL, X86-64] range under 4G. When crashkernel=X,high >> + [KNL, X86-64, ARM64] range under 4G. When crashkernel=X,high >> is passed, kernel could allocate physical memory region >> above 4G, that cause second kernel crash on system >> that require some amount of low memory, e.g. swiotlb >> @@ -857,12 +857,6 @@ >> It will be ignored when crashkernel=X,high is not used >> or memory reserved is below 4G. >> >> - [KNL, ARM64] range in low memory. >> - This one lets the user specify a low range in the >> - DMA zone for the crash dump kernel. >> - It will be ignored when crashkernel=X,high is not used >> - or memory reserved is located in the DMA zones. >> - >> cryptomgr.notests >> [KNL] Disable crypto self-tests >> >> 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. > > 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. > > Will > . >
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
On Mon, Jul 11, 2022 at 05:03:18PM +0800, Zhen Lei wrote: > 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. > + */ > +#define DEFAULT_CRASH_KERNEL_LOW_SIZE \ > + max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20) I agree with Will here, we need a better comment and we might as well change the default value to something else until someone tells us that the default is not large enough. The default swiotlb size is 64M, so we need to cover that. The extra 8MB for any additional low allocations are ok as well but the 256MB doesn't make much sense to me, or at least not together with the rest. If the main kernel got a command line option for a larger swiotlb, does the crash kernel boot with the same command line? If not, we can just go for a fixed 128M value here, which is double the default swiotlb buffer.
On Mon, Jul 11, 2022 at 05:03:18PM +0800, Zhen Lei wrote: > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 2522b11e593f239..65a2c3a22a4b57d 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -843,7 +843,7 @@ > available. > It will be ignored if crashkernel=X is specified. > crashkernel=size[KMG],low > - [KNL, X86-64] range under 4G. When crashkernel=X,high > + [KNL, X86-64, ARM64] range under 4G. When crashkernel=X,high > is passed, kernel could allocate physical memory region > above 4G, that cause second kernel crash on system > that require some amount of low memory, e.g. swiotlb > @@ -857,12 +857,6 @@ > It will be ignored when crashkernel=X,high is not used > or memory reserved is below 4G. > > - [KNL, ARM64] range in low memory. > - This one lets the user specify a low range in the > - DMA zone for the crash dump kernel. > - It will be ignored when crashkernel=X,high is not used > - or memory reserved is located in the DMA zones. > - > cryptomgr.notests > [KNL] Disable crypto self-tests > > 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. > + */ > +#define DEFAULT_CRASH_KERNEL_LOW_SIZE \ > + max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20) > + > static int __init reserve_crashkernel_low(unsigned long long low_size) > { > unsigned long long low_base; > @@ -147,7 +155,9 @@ static void __init reserve_crashkernel(void) > * is not allowed. > */ > ret = parse_crashkernel_low(cmdline, 0, &crash_low_size, &crash_base); > - if (ret && (ret != -ENOENT)) > + if (ret == -ENOENT) > + crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE; > + else if (ret) > return; BTW, since we want a default low allocation, I think we should change the checking logic slightly. Currently we have: if ((crash_base >= CRASH_ADDR_LOW_MAX) && crash_low_size && reserve_crashkernel_low(crash_low_size)) { ... If crash_base is just below CRASH_ADDR_LOW_MAX, we deem it sufficient but a crashkernel trying to allocate 64MB of swiotlb may fail. So maybe change this to crash_base >= CRASH_ADDR_LOW_MAX - crash_low_size.
On 2022/11/8 1:18, Catalin Marinas wrote: > On Mon, Jul 11, 2022 at 05:03:18PM +0800, Zhen Lei wrote: >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >> index 2522b11e593f239..65a2c3a22a4b57d 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -843,7 +843,7 @@ >> available. >> It will be ignored if crashkernel=X is specified. >> crashkernel=size[KMG],low >> - [KNL, X86-64] range under 4G. When crashkernel=X,high >> + [KNL, X86-64, ARM64] range under 4G. When crashkernel=X,high >> is passed, kernel could allocate physical memory region >> above 4G, that cause second kernel crash on system >> that require some amount of low memory, e.g. swiotlb >> @@ -857,12 +857,6 @@ >> It will be ignored when crashkernel=X,high is not used >> or memory reserved is below 4G. >> >> - [KNL, ARM64] range in low memory. >> - This one lets the user specify a low range in the >> - DMA zone for the crash dump kernel. >> - It will be ignored when crashkernel=X,high is not used >> - or memory reserved is located in the DMA zones. >> - >> cryptomgr.notests >> [KNL] Disable crypto self-tests >> >> 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. >> + */ >> +#define DEFAULT_CRASH_KERNEL_LOW_SIZE \ >> + max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20) >> + >> static int __init reserve_crashkernel_low(unsigned long long low_size) >> { >> unsigned long long low_base; >> @@ -147,7 +155,9 @@ static void __init reserve_crashkernel(void) >> * is not allowed. >> */ >> ret = parse_crashkernel_low(cmdline, 0, &crash_low_size, &crash_base); >> - if (ret && (ret != -ENOENT)) >> + if (ret == -ENOENT) >> + crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE; >> + else if (ret) >> return; > > BTW, since we want a default low allocation, I think we should change > the checking logic slightly. Currently we have: > > if ((crash_base >= CRASH_ADDR_LOW_MAX) && > crash_low_size && reserve_crashkernel_low(crash_low_size)) { > ... > > If crash_base is just below CRASH_ADDR_LOW_MAX, we deem it sufficient > but a crashkernel trying to allocate 64MB of swiotlb may fail. So maybe > change this to crash_base >= CRASH_ADDR_LOW_MAX - crash_low_size. The equal sign needs to be removed. The situation should be the allocation of "crashkernel=X,high". This possibility is too small, the high memory is unlikely to be that small. memblock_phys_alloc_range() always search for memory block from high addresses to low addresses. In the initial phase, high-end memory is not fragmented. Of course, the modification can make people look more reassuring. OK, I'll update it. >
On 2022/11/7 22:07, Catalin Marinas wrote: > On Mon, Jul 11, 2022 at 05:03:18PM +0800, Zhen Lei wrote: >> 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. >> + */ >> +#define DEFAULT_CRASH_KERNEL_LOW_SIZE \ >> + max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20) > > I agree with Will here, we need a better comment and we might as well > change the default value to something else until someone tells us that > the default is not large enough. The default swiotlb size is 64M, so we > need to cover that. The extra 8MB for any additional low allocations are > ok as well but the 256MB doesn't make much sense to me, or at least not > together with the rest. > > If the main kernel got a command line option for a larger swiotlb, does > the crash kernel boot with the same command line? If not, we can just go Sometimes the image is shared, but the command line is often different. The command line of the crash kernel is specified by kexec --append=. > for a fixed 128M value here, which is double the default swiotlb buffer. Sorry, I missed this e-mail. Yes, fixed 128M would be better. I haven't seen anyone adding "swiotlb=" to the crash kernel. And, if the default size isn't enough, he can use crashkernel=Y,low. >
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 2522b11e593f239..65a2c3a22a4b57d 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -843,7 +843,7 @@ available. It will be ignored if crashkernel=X is specified. crashkernel=size[KMG],low - [KNL, X86-64] range under 4G. When crashkernel=X,high + [KNL, X86-64, ARM64] range under 4G. When crashkernel=X,high is passed, kernel could allocate physical memory region above 4G, that cause second kernel crash on system that require some amount of low memory, e.g. swiotlb @@ -857,12 +857,6 @@ It will be ignored when crashkernel=X,high is not used or memory reserved is below 4G. - [KNL, ARM64] range in low memory. - This one lets the user specify a low range in the - DMA zone for the crash dump kernel. - It will be ignored when crashkernel=X,high is not used - or memory reserved is located in the DMA zones. - cryptomgr.notests [KNL] Disable crypto self-tests 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. + */ +#define DEFAULT_CRASH_KERNEL_LOW_SIZE \ + max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20) + static int __init reserve_crashkernel_low(unsigned long long low_size) { unsigned long long low_base; @@ -147,7 +155,9 @@ static void __init reserve_crashkernel(void) * is not allowed. */ ret = parse_crashkernel_low(cmdline, 0, &crash_low_size, &crash_base); - if (ret && (ret != -ENOENT)) + if (ret == -ENOENT) + crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE; + else if (ret) return; crash_max = CRASH_ADDR_HIGH_MAX;