diff mbox series

riscv: enable DEBUG_FORCE_FUNCTION_ALIGN_64B

Message ID 20230727160356.3874-1-jszhang@kernel.org (mailing list archive)
State Accepted
Commit 74f438e754832620a73d0cd95eee85d46a5f4161
Headers show
Series riscv: enable DEBUG_FORCE_FUNCTION_ALIGN_64B | expand

Checks

Context Check Description
conchuod/cover_letter success Single patches do not need cover letters
conchuod/tree_selection success Guessed tree name to be for-next at HEAD 471aba2e4760
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 4 and now 4
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 2813 this patch: 9
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 15884 this patch: 9
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 3 this patch: 3
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Jisheng Zhang July 27, 2023, 4:03 p.m. UTC
Allow to force all function address 64B aligned as it is possible for
other architectures. This may be useful when verify if performance
bump is caused by function alignment changes.

Before commit 1bf18da62106 ("lib/Kconfig.debug: add ARCH dependency
for FUNCTION_ALIGN option"), riscv supports enabling the
DEBUG_FORCE_FUNCTION_ALIGN_64B option, but after that commit, each
arch needs to claim the support explicitly. I tried the config file in
[1] for both RV64 and RV32, I can't reproduce the build error as [1],
there is no reason for not allowing to enforce this function alignment.

Link: https://lore.kernel.org/lkml/202202271612.W32UJAj2-lkp@intel.com/ [1]
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 lib/Kconfig.debug | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Conor Dooley July 29, 2023, 10:34 a.m. UTC | #1
Hey,

On Fri, Jul 28, 2023 at 12:03:56AM +0800, Jisheng Zhang wrote:
> Allow to force all function address 64B aligned as it is possible for
> other architectures. This may be useful when verify if performance
> bump is caused by function alignment changes.
> 
> Before commit 1bf18da62106 ("lib/Kconfig.debug: add ARCH dependency
> for FUNCTION_ALIGN option"), riscv supports enabling the
> DEBUG_FORCE_FUNCTION_ALIGN_64B option, but after that commit, each
> arch needs to claim the support explicitly. I tried the config file in
> [1] for both RV64 and RV32, I can't reproduce the build error as [1],
> there is no reason for not allowing to enforce this function alignment.
> 
> Link: https://lore.kernel.org/lkml/202202271612.W32UJAj2-lkp@intel.com/ [1]

This is a CSKY randconfig, is there any particular reason that running
that randconfig (over a year later) and on a different architecture
would trigger whatever the condition was?

The original commit here seems far too penal - why was it not just
disabled on CSKY??? I tried looking a bit on lore, but didn't see
anything explaining the subset of supported archs they picked.
I did see Guo Ren wondering if rv32 would be similarly problematic - but
since this is something likely to just trip up randconfigs, I think we
should go for it and if rv32 becomes a problem, restrict this to 64-bit.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  lib/Kconfig.debug | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index fbc89baf7de6..39ffd218e960 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -502,7 +502,7 @@ config SECTION_MISMATCH_WARN_ONLY
>  
>  config DEBUG_FORCE_FUNCTION_ALIGN_64B
>  	bool "Force all function address 64B aligned"
> -	depends on EXPERT && (X86_64 || ARM64 || PPC32 || PPC64 || ARC || S390)
> +	depends on EXPERT && (X86_64 || ARM64 || PPC32 || PPC64 || ARC || RISCV || S390)
>  	select FUNCTION_ALIGNMENT_64B
>  	help
>  	  There are cases that a commit from one domain changes the function
> -- 
> 2.40.1
>
Jisheng Zhang July 30, 2023, 2:25 p.m. UTC | #2
On Sat, Jul 29, 2023 at 11:34:38AM +0100, Conor Dooley wrote:
> Hey,
> 
> On Fri, Jul 28, 2023 at 12:03:56AM +0800, Jisheng Zhang wrote:
> > Allow to force all function address 64B aligned as it is possible for
> > other architectures. This may be useful when verify if performance
> > bump is caused by function alignment changes.
> > 
> > Before commit 1bf18da62106 ("lib/Kconfig.debug: add ARCH dependency
> > for FUNCTION_ALIGN option"), riscv supports enabling the
> > DEBUG_FORCE_FUNCTION_ALIGN_64B option, but after that commit, each
> > arch needs to claim the support explicitly. I tried the config file in
> > [1] for both RV64 and RV32, I can't reproduce the build error as [1],
> > there is no reason for not allowing to enforce this function alignment.
> > 
> > Link: https://lore.kernel.org/lkml/202202271612.W32UJAj2-lkp@intel.com/ [1]
> 
> This is a CSKY randconfig, is there any particular reason that running
> that randconfig (over a year later) and on a different architecture
> would trigger whatever the condition was?

Just use the randconfig and then s/CSKY/RISCV to check whether RV32
and RV64 can reproduce the compile error ;)

> 
> The original commit here seems far too penal - why was it not just
> disabled on CSKY??? I tried looking a bit on lore, but didn't see
> anything explaining the subset of supported archs they picked.
> I did see Guo Ren wondering if rv32 would be similarly problematic - but
> since this is something likely to just trip up randconfigs, I think we
> should go for it and if rv32 becomes a problem, restrict this to 64-bit.
> 
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Thanks,
> Conor.
> 
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> >  lib/Kconfig.debug | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index fbc89baf7de6..39ffd218e960 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -502,7 +502,7 @@ config SECTION_MISMATCH_WARN_ONLY
> >  
> >  config DEBUG_FORCE_FUNCTION_ALIGN_64B
> >  	bool "Force all function address 64B aligned"
> > -	depends on EXPERT && (X86_64 || ARM64 || PPC32 || PPC64 || ARC || S390)
> > +	depends on EXPERT && (X86_64 || ARM64 || PPC32 || PPC64 || ARC || RISCV || S390)
> >  	select FUNCTION_ALIGNMENT_64B
> >  	help
> >  	  There are cases that a commit from one domain changes the function
> > -- 
> > 2.40.1
> >
patchwork-bot+linux-riscv@kernel.org Aug. 30, 2023, 1:20 p.m. UTC | #3
Hello:

This patch was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Fri, 28 Jul 2023 00:03:56 +0800 you wrote:
> Allow to force all function address 64B aligned as it is possible for
> other architectures. This may be useful when verify if performance
> bump is caused by function alignment changes.
> 
> Before commit 1bf18da62106 ("lib/Kconfig.debug: add ARCH dependency
> for FUNCTION_ALIGN option"), riscv supports enabling the
> DEBUG_FORCE_FUNCTION_ALIGN_64B option, but after that commit, each
> arch needs to claim the support explicitly. I tried the config file in
> [1] for both RV64 and RV32, I can't reproduce the build error as [1],
> there is no reason for not allowing to enforce this function alignment.
> 
> [...]

Here is the summary with links:
  - riscv: enable DEBUG_FORCE_FUNCTION_ALIGN_64B
    https://git.kernel.org/riscv/c/74f438e75483

You are awesome, thank you!
Palmer Dabbelt Aug. 30, 2023, 1:24 p.m. UTC | #4
On Fri, 28 Jul 2023 00:03:56 +0800, Jisheng Zhang wrote:
> Allow to force all function address 64B aligned as it is possible for
> other architectures. This may be useful when verify if performance
> bump is caused by function alignment changes.
> 
> Before commit 1bf18da62106 ("lib/Kconfig.debug: add ARCH dependency
> for FUNCTION_ALIGN option"), riscv supports enabling the
> DEBUG_FORCE_FUNCTION_ALIGN_64B option, but after that commit, each
> arch needs to claim the support explicitly. I tried the config file in
> [1] for both RV64 and RV32, I can't reproduce the build error as [1],
> there is no reason for not allowing to enforce this function alignment.
> 
> [...]

Applied, thanks!

[1/1] riscv: enable DEBUG_FORCE_FUNCTION_ALIGN_64B
      https://git.kernel.org/palmer/c/74f438e75483

Best regards,
diff mbox series

Patch

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index fbc89baf7de6..39ffd218e960 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -502,7 +502,7 @@  config SECTION_MISMATCH_WARN_ONLY
 
 config DEBUG_FORCE_FUNCTION_ALIGN_64B
 	bool "Force all function address 64B aligned"
-	depends on EXPERT && (X86_64 || ARM64 || PPC32 || PPC64 || ARC || S390)
+	depends on EXPERT && (X86_64 || ARM64 || PPC32 || PPC64 || ARC || RISCV || S390)
 	select FUNCTION_ALIGNMENT_64B
 	help
 	  There are cases that a commit from one domain changes the function