[PATCH v2 0/5] kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

[PATCH v2 0/5] kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

[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 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(-)

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jisheng Zhang]

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 <jszhang@kernel.org>
---
 include/linux/kexec.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 0c994ae37729..58d1b58a971e 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -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;
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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 <jszhang@kernel.org>
---
 arch/riscv/mm/init.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index a15640eeb334..84879a5ce818 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -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();
 }
 
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Palmer Dabbelt]

On Mon, 06 Dec 2021 08:05:11 PST (-0800), jszhang@kernel.org wrote:
> 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 <jszhang@kernel.org>
> ---
>  arch/riscv/mm/init.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index a15640eeb334..84879a5ce818 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -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();
>  }

Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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 <jszhang@kernel.org>
---
 arch/x86/kernel/setup.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index c410be738ae7..56b738c1ca33 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -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,
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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 <jszhang@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/mm/init.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index a8834434af99..b4b8b4dc2d31 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -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
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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 <jszhang@kernel.org>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 arch/arm/kernel/setup.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 284a80c0b6e1..f9a054a344c0 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -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)
 {
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

[Baoquan He]

Hi Jisheng,

On 12/07/21 at 12:05am, Jisheng Zhang wrote:
> 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

[Jisheng Zhang]

On Sun, Jan 16, 2022 at 09:38:47PM +0800, Baoquan He wrote:
> Hi Jisheng,

Hi Baoquan,

> 
> On 12/07/21 at 12:05am, Jisheng Zhang wrote:
> > 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?

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

> 
> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

[Baoquan He]

On 01/18/22 at 10:13pm, Jisheng Zhang wrote:
> On Sun, Jan 16, 2022 at 09:38:47PM +0800, Baoquan He wrote:
> > Hi Jisheng,
> 
> Hi Baoquan,
> 
> > 
> > On 12/07/21 at 12:05am, Jisheng Zhang wrote:
> > > 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?
> 
> 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.

> 
> > 
> > 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.

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?


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

[Alexandre Ghiti]

Hi Baoquan,

On Wed, Jan 19, 2022 at 9:11 AM Baoquan He <bhe@redhat.com> wrote:
>
> On 01/18/22 at 10:13pm, Jisheng Zhang wrote:
> > On Sun, Jan 16, 2022 at 09:38:47PM +0800, Baoquan He wrote:
> > > Hi Jisheng,
> >
> > Hi Baoquan,
> >
> > >
> > > On 12/07/21 at 12:05am, Jisheng Zhang wrote:
> > > > 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?
> >
> > 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.
>
> >
> > >
> > > 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.
>
> 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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

[Baoquan He]

On 01/19/22 at 09:52am, Alexandre Ghiti wrote:
> Hi Baoquan,
> 
> On Wed, Jan 19, 2022 at 9:11 AM Baoquan He <bhe@redhat.com> wrote:
> >
> > On 01/18/22 at 10:13pm, Jisheng Zhang wrote:
> > > On Sun, Jan 16, 2022 at 09:38:47PM +0800, Baoquan He wrote:
> > > > Hi Jisheng,
> > >
> > > Hi Baoquan,
> > >
> > > >
> > > > On 12/07/21 at 12:05am, Jisheng Zhang wrote:
> > > > > 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?
> > >
> > > 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.
> >
> > >
> > > >
> > > > 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.
> >
> > 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;
	......
}


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

[Jisheng Zhang]

On Wed, Jan 19, 2022 at 05:33:22PM +0800, Baoquan He wrote:
> On 01/19/22 at 09:52am, Alexandre Ghiti wrote:
> > Hi Baoquan,
> > 
> > On Wed, Jan 19, 2022 at 9:11 AM Baoquan He <bhe@redhat.com> wrote:
> > >
> > > On 01/18/22 at 10:13pm, Jisheng Zhang wrote:
> > > > On Sun, Jan 16, 2022 at 09:38:47PM +0800, Baoquan He wrote:
> > > > > Hi Jisheng,
> > > >
> > > > Hi Baoquan,
> > > >
> > > > >
> > > > > On 12/07/21 at 12:05am, Jisheng Zhang wrote:
> > > > > > 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?
> > > >
> > > > 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.
> > >
> > > >
> > > > >
> > > > > 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.
> > >
> > > 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.

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

> 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.

> 
> 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;
> 	......
> }
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

[Baoquan He]

On 01/19/22 at 07:44pm, Jisheng Zhang wrote:
> On Wed, Jan 19, 2022 at 05:33:22PM +0800, Baoquan He wrote:
> > On 01/19/22 at 09:52am, Alexandre Ghiti wrote:
> > > Hi Baoquan,
> > > 
> > > On Wed, Jan 19, 2022 at 9:11 AM Baoquan He <bhe@redhat.com> wrote:
> > > >
> > > > On 01/18/22 at 10:13pm, Jisheng Zhang wrote:
> > > > > On Sun, Jan 16, 2022 at 09:38:47PM +0800, Baoquan He wrote:
> > > > > > Hi Jisheng,
> > > > >
> > > > > Hi Baoquan,
> > > > >
> > > > > >
> > > > > > On 12/07/21 at 12:05am, Jisheng Zhang wrote:
> > > > > > > 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?
> > > > >
> > > > > 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.
> > > >
> > > > >
> > > > > >
> > > > > > 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.
> > > >
> > > > 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.
> 
> Although the code block will be compiled, but the code block will be
> optimized out.
> 
> > 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. 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] kexec: use IS_ENABLED(CONFIG_KEXEC_CORE) instead of #ifdef

[Baoquan He]

On 12/07/21 at 12:05am, Jisheng Zhang wrote:
> 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 <bhe@redhat.com>

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

> 
> 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(-)
> 
> -- 
> 2.34.1
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel