2020-10-31 07:40:59

by chenzhou

Subject: [PATCH v13 1/8] x86: kdump: replace the hard-coded alignment with macro CRASH_ALIGN

Move CRASH_ALIGN to header asm/kexec.h and replace the hard-coded
alignment with macro CRASH_ALIGN in function reserve_crashkernel().

Suggested-by: Dave Young <[email protected]>
Signed-off-by: Chen Zhou <[email protected]>
Tested-by: John Donnelly <[email protected]>
---
arch/x86/include/asm/kexec.h | 3 +++
arch/x86/kernel/setup.c | 5 +----
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
index 6802c59e8252..8cf9d3fd31c7 100644
--- a/arch/x86/include/asm/kexec.h
+++ b/arch/x86/include/asm/kexec.h
@@ -18,6 +18,9 @@

# define KEXEC_CONTROL_CODE_MAX_SIZE 2048

+/* 2M alignment for crash kernel regions */
+#define CRASH_ALIGN SZ_16M
+
#ifndef __ASSEMBLY__

#include <linux/string.h>
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 84f581c91db4..bf373422dc8a 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -395,9 +395,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)

#ifdef CONFIG_KEXEC_CORE

-/* 16M alignment for crash kernel regions */
-#define CRASH_ALIGN SZ_16M
-
/*
* Keep the crash kernel below this limit.
*
@@ -515,7 +512,7 @@ static void __init reserve_crashkernel(void)
} else {
unsigned long long start;

- start = memblock_phys_alloc_range(crash_size, SZ_1M, crash_base,
+ start = memblock_phys_alloc_range(crash_size, CRASH_ALIGN, crash_base,
crash_base + crash_size);
if (start != crash_base) {
pr_info("crashkernel reservation failed - memory is in use.\n");
--
2.20.1

2020-11-11 01:42:26

by Baoquan He

Subject: Re: [PATCH v13 1/8] x86: kdump: replace the hard-coded alignment with macro CRASH_ALIGN

On 10/31/20 at 03:44pm, Chen Zhou wrote:
> Move CRASH_ALIGN to header asm/kexec.h and replace the hard-coded
> alignment with macro CRASH_ALIGN in function reserve_crashkernel().

Seems you tell what you have done in this patch, but don't like adding
several more words to tell why it's done like that. Please see below
inline comments.

In other patches, I can also see this similar problem.

>
> Suggested-by: Dave Young <[email protected]>
> Signed-off-by: Chen Zhou <[email protected]>
> Tested-by: John Donnelly <[email protected]>
> ---
> arch/x86/include/asm/kexec.h | 3 +++
> arch/x86/kernel/setup.c | 5 +----
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> index 6802c59e8252..8cf9d3fd31c7 100644
> --- a/arch/x86/include/asm/kexec.h
> +++ b/arch/x86/include/asm/kexec.h
> @@ -18,6 +18,9 @@
>
> # define KEXEC_CONTROL_CODE_MAX_SIZE 2048
>
> +/* 2M alignment for crash kernel regions */
> +#define CRASH_ALIGN SZ_16M
> +
> #ifndef __ASSEMBLY__
>
> #include <linux/string.h>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 84f581c91db4..bf373422dc8a 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -395,9 +395,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)
>
> #ifdef CONFIG_KEXEC_CORE
>
> -/* 16M alignment for crash kernel regions */
> -#define CRASH_ALIGN SZ_16M
> -
> /*
> * Keep the crash kernel below this limit.
> *
> @@ -515,7 +512,7 @@ static void __init reserve_crashkernel(void)
> } else {
> unsigned long long start;
>
> - start = memblock_phys_alloc_range(crash_size, SZ_1M, crash_base,
> + start = memblock_phys_alloc_range(crash_size, CRASH_ALIGN, crash_base,
> crash_base + crash_size);

Here, SZ_1M is replaced with CRASH_ALIGN which is 16M. I remember I ever
commented that this had better be told in patch log.

> if (start != crash_base) {
> pr_info("crashkernel reservation failed - memory is in use.\n");
> --
> 2.20.1
>
>
> _______________________________________________
> kexec mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/kexec
>

2020-11-11 13:30:27

by chenzhou

Subject: Re: [PATCH v13 1/8] x86: kdump: replace the hard-coded alignment with macro CRASH_ALIGN

Hi Baoquan,


On 2020/11/11 9:38, Baoquan He wrote:
> On 10/31/20 at 03:44pm, Chen Zhou wrote:
>> Move CRASH_ALIGN to header asm/kexec.h and replace the hard-coded
>> alignment with macro CRASH_ALIGN in function reserve_crashkernel().
> Seems you tell what you have done in this patch, but don't like adding
> several more words to tell why it's done like that. Please see below
> inline comments.
>
> In other patches, I can also see this similar problem.
Thanks for your review. I will update relevant commit messages.
>
>> Suggested-by: Dave Young <[email protected]>
>> Signed-off-by: Chen Zhou <[email protected]>
>> Tested-by: John Donnelly <[email protected]>
>> ---
>> arch/x86/include/asm/kexec.h | 3 +++
>> arch/x86/kernel/setup.c | 5 +----
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
>> index 6802c59e8252..8cf9d3fd31c7 100644
>> --- a/arch/x86/include/asm/kexec.h
>> +++ b/arch/x86/include/asm/kexec.h
>> @@ -18,6 +18,9 @@
>>
>> # define KEXEC_CONTROL_CODE_MAX_SIZE 2048
>>
>> +/* 2M alignment for crash kernel regions */
>> +#define CRASH_ALIGN SZ_16M
>> +
>> #ifndef __ASSEMBLY__
>>
>> #include <linux/string.h>
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index 84f581c91db4..bf373422dc8a 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -395,9 +395,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)
>>
>> #ifdef CONFIG_KEXEC_CORE
>>
>> -/* 16M alignment for crash kernel regions */
>> -#define CRASH_ALIGN SZ_16M
>> -
>> /*
>> * Keep the crash kernel below this limit.
>> *
>> @@ -515,7 +512,7 @@ static void __init reserve_crashkernel(void)
>> } else {
>> unsigned long long start;
>>
>> - start = memblock_phys_alloc_range(crash_size, SZ_1M, crash_base,
>> + start = memblock_phys_alloc_range(crash_size, CRASH_ALIGN, crash_base,
>> crash_base + crash_size);
> Here, SZ_1M is replaced with CRASH_ALIGN which is 16M. I remember I ever
> commented that this had better be told in patch log.
got it.

Thanks,
Chen Zhou
>
>> if (start != crash_base) {
>> pr_info("crashkernel reservation failed - memory is in use.\n");
>> --
>> 2.20.1
>>
>>
>> _______________________________________________
>> kexec mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/kexec
>>
> .
>

2020-11-12 08:02:21

by Mike Rapoport

Subject: Re: [PATCH v13 1/8] x86: kdump: replace the hard-coded alignment with macro CRASH_ALIGN

Hi,

On Sat, Oct 31, 2020 at 03:44:30PM +0800, Chen Zhou wrote:
> Move CRASH_ALIGN to header asm/kexec.h and replace the hard-coded
> alignment with macro CRASH_ALIGN in function reserve_crashkernel().
>
> Suggested-by: Dave Young <[email protected]>
> Signed-off-by: Chen Zhou <[email protected]>
> Tested-by: John Donnelly <[email protected]>
> ---
> arch/x86/include/asm/kexec.h | 3 +++
> arch/x86/kernel/setup.c | 5 +----
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> index 6802c59e8252..8cf9d3fd31c7 100644
> --- a/arch/x86/include/asm/kexec.h
> +++ b/arch/x86/include/asm/kexec.h
> @@ -18,6 +18,9 @@
>
> # define KEXEC_CONTROL_CODE_MAX_SIZE 2048
>
> +/* 2M alignment for crash kernel regions */
> +#define CRASH_ALIGN SZ_16M

Please update the comment to match the code.

> +
> #ifndef __ASSEMBLY__
>
> #include <linux/string.h>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 84f581c91db4..bf373422dc8a 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -395,9 +395,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)
>
> #ifdef CONFIG_KEXEC_CORE
>
> -/* 16M alignment for crash kernel regions */
> -#define CRASH_ALIGN SZ_16M
> -
> /*
> * Keep the crash kernel below this limit.
> *
> @@ -515,7 +512,7 @@ static void __init reserve_crashkernel(void)
> } else {
> unsigned long long start;
>
> - start = memblock_phys_alloc_range(crash_size, SZ_1M, crash_base,
> + start = memblock_phys_alloc_range(crash_size, CRASH_ALIGN, crash_base,
> crash_base + crash_size);
> if (start != crash_base) {
> pr_info("crashkernel reservation failed - memory is in use.\n");
> --
> 2.20.1
>

--
Sincerely yours,
Mike.

2020-11-12 08:16:57

by chenzhou

Subject: Re: [PATCH v13 1/8] x86: kdump: replace the hard-coded alignment with macro CRASH_ALIGN

On 2020/11/12 15:58, Mike Rapoport wrote:
> Hi,
>
> On Sat, Oct 31, 2020 at 03:44:30PM +0800, Chen Zhou wrote:
>> Move CRASH_ALIGN to header asm/kexec.h and replace the hard-coded
>> alignment with macro CRASH_ALIGN in function reserve_crashkernel().
>>
>> Suggested-by: Dave Young <[email protected]>
>> Signed-off-by: Chen Zhou <[email protected]>
>> Tested-by: John Donnelly <[email protected]>
>> ---
>> arch/x86/include/asm/kexec.h | 3 +++
>> arch/x86/kernel/setup.c | 5 +----
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
>> index 6802c59e8252..8cf9d3fd31c7 100644
>> --- a/arch/x86/include/asm/kexec.h
>> +++ b/arch/x86/include/asm/kexec.h
>> @@ -18,6 +18,9 @@
>>
>> # define KEXEC_CONTROL_CODE_MAX_SIZE 2048
>>
>> +/* 2M alignment for crash kernel regions */
>> +#define CRASH_ALIGN SZ_16M
> Please update the comment to match the code.
Ok, thanks for pointing this mistake.

Thanks,
Chen Zhou
>
>> +
>> #ifndef __ASSEMBLY__
>>
>> #include <linux/string.h>
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index 84f581c91db4..bf373422dc8a 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -395,9 +395,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)
>>
>> #ifdef CONFIG_KEXEC_CORE
>>
>> -/* 16M alignment for crash kernel regions */
>> -#define CRASH_ALIGN SZ_16M
>> -
>> /*
>> * Keep the crash kernel below this limit.
>> *
>> @@ -515,7 +512,7 @@ static void __init reserve_crashkernel(void)
>> } else {
>> unsigned long long start;
>>
>> - start = memblock_phys_alloc_range(crash_size, SZ_1M, crash_base,
>> + start = memblock_phys_alloc_range(crash_size, CRASH_ALIGN, crash_base,
>> crash_base + crash_size);
>> if (start != crash_base) {
>> pr_info("crashkernel reservation failed - memory is in use.\n");
>> --
>> 2.20.1
>>