kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

PATCH In review Jisheng Zhang 4 participants 15 messages Dec 6, 2021 – Jan 20, 2022

Subsystems (7)

Arm (32-bit) x86 RISC-V arm64 +3 more subsystems

lore

Share & tools

lore

Jisheng Zhang

author

2021-12-06 16:05 UTC #0 lore raw

v2 cover kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.

I only modify x86, arm, arm64 and riscv, other architectures such as sh, powerpc and s390 are better to be kept kexec code as-is so they are not touched.

Since v1:
- collect Reviewed-by tag
- fix misleading commit msg.

Jisheng Zhang (5):
kexec: make crashk_res, crashk_low_res and crash_notes symbols always
visible
riscv: mm: init: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef x86/setup: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef
arm64: mm: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef
arm: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

 arch/arm/kernel/setup.c |  7 +++----
 arch/arm64/mm/init.c    |  9 +++------
 arch/riscv/mm/init.c    |  6 ++----
 arch/x86/kernel/setup.c | 10 +++-------
 include/linux/kexec.h   | 12 ++++++------
 5 files changed, 17 insertions(+), 27 deletions(-)

Signature · 7 lines

2.34.1


_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Jisheng Zhang

author

2021-12-06 16:05 UTC #1 lore raw

PATCH v2 1/5 kexec: make crashk_res, crashk_low_res and crash_notes symbols always visible

Make the forward declarations of crashk_res, crashk_low_res and crash_notes always visible. Code referring to these symbols can then just check for IS_ENABLED(CONFIG_KEXEC_CORE), instead of requiring conditional compilation using an #ifdef, thus preparing to increase compile coverage and simplify the code.

Signed-off-by Jisheng Zhang <[email protected]>

---
 include/linux/kexec.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

include/linux/kexec.h +6 -6

index 0c994ae37729..58d1b58a971e 100644
@@ -20,6 +20,12 @@
 
 #include <uapi/linux/kexec.h>
 
+/* Location of a reserved region to hold the crash kernel.
+ */
+extern struct resource crashk_res;
+extern struct resource crashk_low_res;
+extern note_buf_t __percpu *crash_notes;
+
 #ifdef CONFIG_KEXEC_CORE
 #include <linux/list.h>
 #include <linux/compat.h>

@@ -350,12 +356,6 @@ extern int kexec_load_disabled;
 #define KEXEC_FILE_FLAGS	(KEXEC_FILE_UNLOAD | KEXEC_FILE_ON_CRASH | \
 				 KEXEC_FILE_NO_INITRAMFS)
 
-/* Location of a reserved region to hold the crash kernel.
- */
-extern struct resource crashk_res;
-extern struct resource crashk_low_res;
-extern note_buf_t __percpu *crash_notes;
-
 /* flag to track if kexec reboot is in progress */
 extern bool kexec_in_progress;

Signature · 7 lines

2.34.1


_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Jisheng Zhang

author

2021-12-06 16:05 UTC #2 lore raw

PATCH v2 2/5 riscv: mm: init: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

In reply to Jisheng Zhang

Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.

Signed-off-by Jisheng Zhang <[email protected]>

---
 arch/riscv/mm/init.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

arch/riscv/mm/init.c +2 -4

index a15640eeb334..84879a5ce818 100644
@@ -748,7 +748,6 @@ static inline void setup_vm_final(void)
 }
 #endif /* CONFIG_MMU */
 
-#ifdef CONFIG_KEXEC_CORE
 /*
  * reserve_crashkernel() - reserves memory for crash kernel
  *

@@ -765,6 +764,8 @@ static void __init reserve_crashkernel(void)
 
 	int ret = 0;
 
+	if (!IS_ENABLED(CONFIG_KEXEC_CORE))
+		return;
 	/*
 	 * Don't reserve a region for a crash kernel on a crash kernel
 	 * since it doesn't make much sense and we have limited memory

@@ -805,7 +806,6 @@ static void __init reserve_crashkernel(void)
 	crashk_res.start = crash_base;
 	crashk_res.end = crash_base + crash_size - 1;
 }
-#endif /* CONFIG_KEXEC_CORE */
 
 void __init paging_init(void)
 {

@@ -819,9 +819,7 @@ void __init misc_mem_init(void)
 	arch_numa_init();
 	sparse_init();
 	zone_sizes_init();
-#ifdef CONFIG_KEXEC_CORE
 	reserve_crashkernel();
-#endif
 	memblock_dump_all();
 }

Signature · 7 lines

2.34.1


_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Palmer Dabbelt

maintainer Acked-by on v2 2/5

2022-01-11 17:29 UTC #3 lore raw

Re: [PATCH v2 2/5] riscv: mm: init: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

Quoted 39 lines · jszhang at kernel.org

Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.

Signed-off-by Jisheng Zhang <[email protected]>

---
 arch/riscv/mm/init.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

arch/riscv/mm/init.c +2 -2

index a15640eeb334..84879a5ce818 100644
@@ -748,7 +748,6 @@ static inline void setup_vm_final(void)
 }
 #endif /* CONFIG_MMU */
 
-#ifdef CONFIG_KEXEC_CORE
 /*
  * reserve_crashkernel() - reserves memory for crash kernel
  *

@@ -765,6 +764,8 @@ static void __init reserve_crashkernel(void)
 
 	int ret = 0;
 
+	if (!IS_ENABLED(CONFIG_KEXEC_CORE))
+		return;
 	/*
 	 * Don't reserve a region for a crash kernel on a crash kernel
 	 * since it doesn't make much sense and we have limited memory

@@ -805,7 +806,6 @@ static void __init reserve_crashkernel(void)
 	crashk_res.start = crash_base;
 	crashk_res.end = crash_base + crash_size - 1;
 }
-#endif /* CONFIG_KEXEC_CORE */
 
 void __init paging_init(void)
 {

arch/riscv/mm/init.c void __init misc_mem_init(void) · line 824

Quoted 8 lines

arch/riscv/mm/init.c +0 -2

@@ -819,9 +819,7 @@ void __init misc_mem_init(void)
 	arch_numa_init();
 	sparse_init();
 	zone_sizes_init();
-#ifdef CONFIG_KEXEC_CORE
 	reserve_crashkernel();
-#endif
 	memblock_dump_all();
 }

Acked-by Palmer Dabbelt <[email protected]>

LMK if you wanted me to take this through the RISC-V tree, otherwise I'm going to assume all these are going in together.

Thanks!

Jisheng Zhang

author

2021-12-06 16:05 UTC #4 lore raw

PATCH v2 3/5 x86/setup: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

In reply to Jisheng Zhang

Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.

Signed-off-by Jisheng Zhang <[email protected]>

---
 arch/x86/kernel/setup.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

arch/x86/kernel/setup.c +3 -7

index c410be738ae7..56b738c1ca33 100644
@@ -390,8 +390,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)
  * --------- Crashkernel reservation ------------------------------
  */
 
-#ifdef CONFIG_KEXEC_CORE
-
 /* 16M alignment for crash kernel regions */
 #define CRASH_ALIGN		SZ_16M

@@ -469,6 +467,9 @@ static void __init reserve_crashkernel(void)
 	bool high = false;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_KEXEC_CORE))
+		return;
+
 	total_mem = memblock_phys_mem_size();
 
 	/* crashkernel=XM */

@@ -534,11 +535,6 @@ static void __init reserve_crashkernel(void)
 	crashk_res.end   = crash_base + crash_size - 1;
 	insert_resource(&iomem_resource, &crashk_res);
 }
-#else
-static void __init reserve_crashkernel(void)
-{
-}
-#endif
 
 static struct resource standard_io_resources[] = {
 	{ .name = "dma1", .start = 0x00, .end = 0x1f,

Signature · 7 lines

2.34.1


_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Jisheng Zhang

author

2021-12-06 16:05 UTC #5 lore raw

PATCH v2 4/5 arm64: mm: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

In reply to Jisheng Zhang

Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.

Signed-off-by Jisheng Zhang <[email protected]>

Acked-by Catalin Marinas <[email protected]>

---
 arch/arm64/mm/init.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

arch/arm64/mm/init.c +3 -6

index a8834434af99..b4b8b4dc2d31 100644
@@ -64,7 +64,6 @@ EXPORT_SYMBOL(memstart_addr);
  */
 phys_addr_t arm64_dma_phys_limit __ro_after_init;
 
-#ifdef CONFIG_KEXEC_CORE
 /*
  * reserve_crashkernel() - reserves memory for crash kernel
  *

@@ -78,6 +77,9 @@ static void __init reserve_crashkernel(void)
 	unsigned long long crash_max = arm64_dma_phys_limit;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_KEXEC_CORE))
+		return;
+
 	ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
 				&crash_size, &crash_base);
 	/* no crashkernel= or invalid value specified */

@@ -110,11 +112,6 @@ static void __init reserve_crashkernel(void)
 	crashk_res.start = crash_base;
 	crashk_res.end = crash_base + crash_size - 1;
 }
-#else
-static void __init reserve_crashkernel(void)
-{
-}
-#endif /* CONFIG_KEXEC_CORE */
 
 /*
  * Return the maximum physical address for a zone accessible by the given bits

Signature · 7 lines

2.34.1


_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Jisheng Zhang

author

2021-12-06 16:05 UTC #6 lore raw

PATCH v2 5/5 arm: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

In reply to Jisheng Zhang

Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.

Signed-off-by Jisheng Zhang <[email protected]>

Reviewed-by Russell King (Oracle) <[email protected]>

---
 arch/arm/kernel/setup.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

arch/arm/kernel/setup.c +3 -4

index 284a80c0b6e1..f9a054a344c0 100644
@@ -973,7 +973,6 @@ static int __init init_machine_late(void)
 }
 late_initcall(init_machine_late);
 
-#ifdef CONFIG_KEXEC
 /*
  * The crash region must be aligned to 128MB to avoid
  * zImage relocating below the reserved region.

@@ -1001,6 +1000,9 @@ static void __init reserve_crashkernel(void)
 	unsigned long long total_mem;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_KEXEC_CORE))
+		return;
+
 	total_mem = get_total_mem();
 	ret = parse_crashkernel(boot_command_line, total_mem,
 				&crash_size, &crash_base);

@@ -1056,9 +1058,6 @@ static void __init reserve_crashkernel(void)
 		insert_resource(&iomem_resource, &crashk_boot_res);
 	}
 }
-#else
-static inline void reserve_crashkernel(void) {}
-#endif /* CONFIG_KEXEC */
 
 void __init hyp_mode_check(void)
 {

Signature · 7 lines

2.34.1


_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Baoquan He

2022-01-16 13:38 UTC #7 lore raw

In reply to Jisheng Zhang

Hi Jisheng,

Quoted 3 lines · Jisheng Zhang

Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.

I go through this patchset, You mention the benefits it brings are
1) simplity the code;
2) increase compile coverage;

For benefit 1), it mainly removes the dummy function in x86, arm and arm64, right?

For benefit 2), increasing compile coverage, could you tell more how it achieves and why it matters? What if people disables CONFIG_KEXEC_CORE in purpose? Please forgive my poor compiling knowledge.

Thanks
Baoquan

Jisheng Zhang

author

2022-01-18 14:13 UTC #8 lore raw

Quoted 1 line · Baoquan He

Hi Jisheng,

Hi Baoquan,

Quoted 6 lines · Jisheng Zhang

Quoted 3 lines
Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.

Quoted 6 lines

I go through this patchset, You mention the benefits it brings are
1) simplity the code;
2) increase compile coverage;
For benefit 1), it mainly removes the dummy function in x86, arm and arm64, right?

Another benefit: remove those #ifdef #else #endif usage. Recently, I fixed a bug due to lots of "#ifdefs":
http://lists.infradead.org/pipermail/linux-riscv/2021-December/010607.html

Quoted 4 lines

For benefit 2), increasing compile coverage, could you tell more how it achieves and why it matters? What if people disables CONFIG_KEXEC_CORE in purpose? Please forgive my poor compiling knowledge.

Just my humble opinion, let's compare the code::

#ifdef CONFIG_KEXEC_CORE

code block A;

#endif

If KEXEC_CORE is disabled, code block A won't be compiled at all, the preprocessor will remove code block A;

If we convert the code to:

if (IS_ENABLED(CONFIG_KEXEC_CORE)) {
	code block A;
}

Even if KEXEC_CORE is disabled, code block A is still compiled.

Thanks

Baoquan He

2022-01-19 08:08 UTC #9 lore raw

Quoted 15 lines · Jisheng Zhang

Quoted 1 line · Baoquan He
Hi Jisheng,
Hi Baoquan,
Quoted 10 lines · Jisheng Zhang
Quoted 3 lines
Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.
I go through this patchset, You mention the benefits it brings are
1) simplity the code;
2) increase compile coverage;

Quoted 6 lines

Quoted 2 lines
For benefit 1), it mainly removes the dummy function in x86, arm and arm64, right?
Another benefit: remove those #ifdef #else #endif usage. Recently, I fixed a bug due to lots of "#ifdefs":
http://lists.infradead.org/pipermail/linux-riscv/2021-December/010607.html

Glad to know the fix. While, sometime the ifdeffery is necessary. I am sorry about the one in riscv and you have fixed, it's truly a bug . But, the increasing compile coverage at below you tried to make, it may cause issue. Please see below my comment.

Quoted 17 lines

Quoted 5 lines
For benefit 2), increasing compile coverage, could you tell more how it achieves and why it matters? What if people disables CONFIG_KEXEC_CORE in purpose? Please forgive my poor compiling knowledge.
Just my humble opinion, let's compare the code::
#ifdef CONFIG_KEXEC_CORE
code block A;
#endif
If KEXEC_CORE is disabled, code block A won't be compiled at all, the preprocessor will remove code block A;

Quoted 7 lines

If we convert the code to:
if (IS_ENABLED(CONFIG_KEXEC_CORE)) {
	code block A;
}
Even if KEXEC_CORE is disabled, code block A is still compiled.

This is what I am worried about. Before, if CONFIG_KEXEC_CORE is unset, those relevant codes are not compiled in. I can't see what benefit is brought in if compiled in the unneeded code block. Do I miss anything?

Alexandre Ghiti

2022-01-19 08:52 UTC #10 lore raw

Hi Baoquan,

Quoted 52 lines · Baoquan He

Quoted 15 lines · Jisheng Zhang
Quoted 1 line · Baoquan He
Hi Jisheng,
Hi Baoquan,
Quoted 10 lines · Jisheng Zhang
Quoted 3 lines
Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.
I go through this patchset, You mention the benefits it brings are
1) simplity the code;
2) increase compile coverage;
Quoted 6 lines
Quoted 2 lines
For benefit 1), it mainly removes the dummy function in x86, arm and arm64, right?
Another benefit: remove those #ifdef #else #endif usage. Recently, I fixed a bug due to lots of "#ifdefs":
http://lists.infradead.org/pipermail/linux-riscv/2021-December/010607.html
Glad to know the fix. While, sometime the ifdeffery is necessary. I am sorry about the one in riscv and you have fixed, it's truly a bug . But, the increasing compile coverage at below you tried to make, it may cause issue. Please see below my comment.
Quoted 23 lines
Quoted 5 lines
For benefit 2), increasing compile coverage, could you tell more how it achieves and why it matters? What if people disables CONFIG_KEXEC_CORE in purpose? Please forgive my poor compiling knowledge.
Just my humble opinion, let's compare the code::
#ifdef CONFIG_KEXEC_CORE
code block A;
#endif
If KEXEC_CORE is disabled, code block A won't be compiled at all, the preprocessor will remove code block A;
If we convert the code to:
if (IS_ENABLED(CONFIG_KEXEC_CORE)) {
      code block A;
}

Quoted 7 lines

Quoted 1 line
Even if KEXEC_CORE is disabled, code block A is still compiled.
This is what I am worried about. Before, if CONFIG_KEXEC_CORE is unset, those relevant codes are not compiled in. I can't see what benefit is brought in if compiled in the unneeded code block. Do I miss anything?

This is explained in Documentation/process/coding-style.rst "21) Conditional Compilation".

Alex

Quoted 5 lines

_______________________________________________
linux-riscv mailing list
linux-riscv at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Baoquan He

2022-01-19 09:33 UTC #11 lore raw

Quoted 57 lines · Alexandre Ghiti

Hi Baoquan,
Quoted 54 lines · Baoquan He
Quoted 15 lines · Jisheng Zhang
Quoted 1 line · Baoquan He
Hi Jisheng,
Hi Baoquan,
Quoted 10 lines · Jisheng Zhang
Quoted 3 lines
Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.
I go through this patchset, You mention the benefits it brings are
1) simplity the code;
2) increase compile coverage;
Quoted 6 lines
Quoted 2 lines
For benefit 1), it mainly removes the dummy function in x86, arm and arm64, right?
Another benefit: remove those #ifdef #else #endif usage. Recently, I fixed a bug due to lots of "#ifdefs":
http://lists.infradead.org/pipermail/linux-riscv/2021-December/010607.html
Glad to know the fix. While, sometime the ifdeffery is necessary. I am sorry about the one in riscv and you have fixed, it's truly a bug . But, the increasing compile coverage at below you tried to make, it may cause issue. Please see below my comment.
Quoted 24 lines
Quoted 5 lines
For benefit 2), increasing compile coverage, could you tell more how it achieves and why it matters? What if people disables CONFIG_KEXEC_CORE in purpose? Please forgive my poor compiling knowledge.
Just my humble opinion, let's compare the code::
#ifdef CONFIG_KEXEC_CORE
code block A;
#endif
If KEXEC_CORE is disabled, code block A won't be compiled at all, the preprocessor will remove code block A;
If we convert the code to:
if (IS_ENABLED(CONFIG_KEXEC_CORE)) {
      code block A;
}
Even if KEXEC_CORE is disabled, code block A is still compiled.

Quoted 8 lines

Quoted 5 lines
This is what I am worried about. Before, if CONFIG_KEXEC_CORE is unset, those relevant codes are not compiled in. I can't see what benefit is brought in if compiled in the unneeded code block. Do I miss anything?
This is explained in Documentation/process/coding-style.rst "21) Conditional Compilation".

Thanks for the pointer, Alex.

I read that part, while my confusion isn't gone. With the current code, CONFIG_KEXEC_CORE is set,
- reserve_crashkernel_low() and reserve_crashkernel() compiled in.
CONFIG_KEXEC_CORE is unset,
- reserve_crashkernel_low() and reserve_crashkernel() compiled out.

After this patch applied, does it have the same effect as the old code?

arch/x86/kernel/setup.c:

before
======
#ifdef CONFIG_KEXEC_CORE
static int __init reserve_crashkernel_low(void)

{
	......
}
static void __init reserve_crashkernel(void)
{
	......
}

#else
static void __init reserve_crashkernel(void)
{
}
#endif

after
=======
static int __init reserve_crashkernel_low(void)

{
	......
}
static void __init reserve_crashkernel(void)
{
	......
	if (!IS_ENABLED(CONFIG_KEXEC_CORE))
                return;
	......
}

Jisheng Zhang

author

2022-01-19 11:44 UTC #12 lore raw

Quoted 64 lines · Baoquan He

Quoted 63 lines · Alexandre Ghiti
Hi Baoquan,
Quoted 59 lines · Baoquan He
Quoted 15 lines · Jisheng Zhang
Quoted 1 line · Baoquan He
Hi Jisheng,
Hi Baoquan,
Quoted 10 lines · Jisheng Zhang
Quoted 3 lines
Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.
I go through this patchset, You mention the benefits it brings are
1) simplity the code;
2) increase compile coverage;
Quoted 6 lines
Quoted 2 lines
For benefit 1), it mainly removes the dummy function in x86, arm and arm64, right?
Another benefit: remove those #ifdef #else #endif usage. Recently, I fixed a bug due to lots of "#ifdefs":
http://lists.infradead.org/pipermail/linux-riscv/2021-December/010607.html
Glad to know the fix. While, sometime the ifdeffery is necessary. I am sorry about the one in riscv and you have fixed, it's truly a bug . But, the increasing compile coverage at below you tried to make, it may cause issue. Please see below my comment.
Quoted 17 lines
Quoted 5 lines
For benefit 2), increasing compile coverage, could you tell more how it achieves and why it matters? What if people disables CONFIG_KEXEC_CORE in purpose? Please forgive my poor compiling knowledge.
Just my humble opinion, let's compare the code::
#ifdef CONFIG_KEXEC_CORE
code block A;
#endif
If KEXEC_CORE is disabled, code block A won't be compiled at all, the preprocessor will remove code block A;
Quoted 7 lines
If we convert the code to:
if (IS_ENABLED(CONFIG_KEXEC_CORE)) {
      code block A;
}
Even if KEXEC_CORE is disabled, code block A is still compiled.
This is what I am worried about. Before, if CONFIG_KEXEC_CORE is unset, those relevant codes are not compiled in. I can't see what benefit is brought in if compiled in the unneeded code block. Do I miss anything?

Quoted 8 lines

Quoted 2 lines
This is explained in Documentation/process/coding-style.rst "21) Conditional Compilation".
Thanks for the pointer, Alex.
I read that part, while my confusion isn't gone. With the current code, CONFIG_KEXEC_CORE is set,
- reserve_crashkernel_low() and reserve_crashkernel() compiled in.

Although the code block will be compiled, but the code block will be optimized out.

Quoted 4 lines

CONFIG_KEXEC_CORE is unset,
- reserve_crashkernel_low() and reserve_crashkernel() compiled out.
After this patch applied, does it have the same effect as the old code?

I compared the .o, and can confirm they acchieve the same effect.

Quoted 34 lines

arch/x86/kernel/setup.c:
before
======
#ifdef CONFIG_KEXEC_CORE
static int __init reserve_crashkernel_low(void)
{
	......
}
static void __init reserve_crashkernel(void)
{
	......
}
#else
static void __init reserve_crashkernel(void)
{
}
#endif
after
=======
static int __init reserve_crashkernel_low(void)
{
	......
}
static void __init reserve_crashkernel(void)
{
	......
	if (!IS_ENABLED(CONFIG_KEXEC_CORE))
                return;
	......
}

Baoquan He

2022-01-20 09:45 UTC #13 lore raw

Quoted 77 lines · Jisheng Zhang

Quoted 64 lines · Baoquan He
Quoted 63 lines · Alexandre Ghiti
Hi Baoquan,
Quoted 59 lines · Baoquan He
Quoted 15 lines · Jisheng Zhang
Quoted 1 line · Baoquan He
Hi Jisheng,
Hi Baoquan,
Quoted 10 lines · Jisheng Zhang
Quoted 3 lines
Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.
I go through this patchset, You mention the benefits it brings are
1) simplity the code;
2) increase compile coverage;
Quoted 6 lines
Quoted 2 lines
For benefit 1), it mainly removes the dummy function in x86, arm and arm64, right?
Another benefit: remove those #ifdef #else #endif usage. Recently, I fixed a bug due to lots of "#ifdefs":
http://lists.infradead.org/pipermail/linux-riscv/2021-December/010607.html
Glad to know the fix. While, sometime the ifdeffery is necessary. I am sorry about the one in riscv and you have fixed, it's truly a bug . But, the increasing compile coverage at below you tried to make, it may cause issue. Please see below my comment.
Quoted 17 lines
Quoted 5 lines
For benefit 2), increasing compile coverage, could you tell more how it achieves and why it matters? What if people disables CONFIG_KEXEC_CORE in purpose? Please forgive my poor compiling knowledge.
Just my humble opinion, let's compare the code::
#ifdef CONFIG_KEXEC_CORE
code block A;
#endif
If KEXEC_CORE is disabled, code block A won't be compiled at all, the preprocessor will remove code block A;
Quoted 7 lines
If we convert the code to:
if (IS_ENABLED(CONFIG_KEXEC_CORE)) {
      code block A;
}
Even if KEXEC_CORE is disabled, code block A is still compiled.
This is what I am worried about. Before, if CONFIG_KEXEC_CORE is unset, those relevant codes are not compiled in. I can't see what benefit is brought in if compiled in the unneeded code block. Do I miss anything?
Quoted 8 lines
Quoted 2 lines
This is explained in Documentation/process/coding-style.rst "21) Conditional Compilation".
Thanks for the pointer, Alex.
I read that part, while my confusion isn't gone. With the current code, CONFIG_KEXEC_CORE is set,
- reserve_crashkernel_low() and reserve_crashkernel() compiled in.
Although the code block will be compiled, but the code block will be optimized out.

Quoted 6 lines

Quoted 4 lines
CONFIG_KEXEC_CORE is unset,
- reserve_crashkernel_low() and reserve_crashkernel() compiled out.
After this patch applied, does it have the same effect as the old code?
I compared the .o, and can confirm they acchieve the same effect.

Checked the .o, it's truly as you said. I didn't know this before, thank you and Alex, learned this now.

Seems only static function has this effect. I tested your x86 patch, those two functions are all optimized out. If I remove the static, the entire reserve_crashkernel_low() exists, while reserve_crashkernel() will be optimized as a empty function.

Baoquan He

Acked-by on the v2 series

2022-01-20 09:50 UTC #14 lore raw

In reply to Jisheng Zhang

Quoted 3 lines · Jisheng Zhang

Replace the conditional compilation using "#ifdef CONFIG_KEXEC_CORE" by a check for "IS_ENABLED(CONFIG_KEXEC_CORE)", to simplify the code and increase compile coverage.

Only checked the x86 patch, but the whole patchset looks good to me, thanks, Jisheng.

Acked-by Baoquan He <[email protected]>

Maybe Andrew can help pick this whole series lest each patch need be taken care of by its own ARCH maintainer.

Quoted 27 lines

I only modify x86, arm, arm64 and riscv, other architectures such as sh, powerpc and s390 are better to be kept kexec code as-is so they are not touched.
Since v1:
- collect Reviewed-by tag
- fix misleading commit msg.
Jisheng Zhang (5):
kexec: make crashk_res, crashk_low_res and crash_notes symbols always
visible
riscv: mm: init: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef x86/setup: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef
arm64: mm: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef
arm: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef
 arch/arm/kernel/setup.c |  7 +++----
 arch/arm64/mm/init.c    |  9 +++------
 arch/riscv/mm/init.c    |  6 ++----
 arch/x86/kernel/setup.c | 10 +++-------
 include/linux/kexec.h   | 12 ++++++------
 5 files changed, 17 insertions(+), 27 deletions(-)
Signature · 1 line
2.34.1