diff mbox series

[v4,09/29] arm64: disable recordmcount with DYNAMIC_FTRACE_WITH_REGS

Message ID 20200929214631.3516445-10-samitolvanen@google.com
State New, archived
Headers show
Series Add support for Clang LTO | expand

Commit Message

Sami Tolvanen Sept. 29, 2020, 9:46 p.m. UTC
Select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY to disable
recordmcount when DYNAMIC_FTRACE_WITH_REGS is selected.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/arm64/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

Comments

Mark Rutland Sept. 30, 2020, 9:58 a.m. UTC | #1
Hi Sami,

On Tue, Sep 29, 2020 at 02:46:11PM -0700, Sami Tolvanen wrote:
> Select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY to disable
> recordmcount when DYNAMIC_FTRACE_WITH_REGS is selected.

Could you please add an explanation as to /why/ this is necessary in the
commit message? I couldn't figure this out form the commit message
alone, and reading the cover letter also didn't help.

If the minimum required GCC version supports patchable-function-entry
I'd be happy to make that a requirement for dynamic ftrace on arm64, as
then we'd only need to support one mechanism, and can get rid of some
redundant code. We already default to it when present anyhow.

> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  arch/arm64/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 6d232837cbee..ad522b021f35 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -155,6 +155,8 @@ config ARM64
>  	select HAVE_DYNAMIC_FTRACE
>  	select HAVE_DYNAMIC_FTRACE_WITH_REGS \
>  		if $(cc-option,-fpatchable-function-entry=2)
> +	select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY \
> +		if DYNAMIC_FTRACE_WITH_REGS

This doesn't look quite right to me. Presumably we shouldn't allow
DYNAMIC_FTRACE_WITH_REGS to be selected if HAVE_DYNAMIC_FTRACE_WITH_REGS
isn't.

Thanks,
Mark.
Sami Tolvanen Sept. 30, 2020, 5:10 p.m. UTC | #2
Hi Mark,

On Wed, Sep 30, 2020 at 2:59 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Sami,
>
> On Tue, Sep 29, 2020 at 02:46:11PM -0700, Sami Tolvanen wrote:
> > Select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY to disable
> > recordmcount when DYNAMIC_FTRACE_WITH_REGS is selected.
>
> Could you please add an explanation as to /why/ this is necessary in the
> commit message? I couldn't figure this out form the commit message
> alone, and reading the cover letter also didn't help.

Sorry about that, I'll add a better explanation in the next version.

Note that without LTO, this change is not strictly necessary as
there's no harm in running recordmcount even if it's not needed. It
might slow down the build slightly, but I suspect a few thousand
invocations of the program won't take that long. However, with LTO we
need to disable recordmcount because it doesn't understand LLVM
bitcode.

> If the minimum required GCC version supports patchable-function-entry
> I'd be happy to make that a requirement for dynamic ftrace on arm64, as
> then we'd only need to support one mechanism, and can get rid of some
> redundant code. We already default to it when present anyhow.

That would be great, but Documentation/process/changes.rst suggests
the minimum gcc version is 4.9, and according to Godbolt we would need
gcc >= 8 for -fpatchable-function-entry:

  https://godbolt.org/z/jdzcMW

> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 6d232837cbee..ad522b021f35 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -155,6 +155,8 @@ config ARM64
> >       select HAVE_DYNAMIC_FTRACE
> >       select HAVE_DYNAMIC_FTRACE_WITH_REGS \
> >               if $(cc-option,-fpatchable-function-entry=2)
> > +     select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY \
> > +             if DYNAMIC_FTRACE_WITH_REGS
>
> This doesn't look quite right to me. Presumably we shouldn't allow
> DYNAMIC_FTRACE_WITH_REGS to be selected if HAVE_DYNAMIC_FTRACE_WITH_REGS
> isn't.

This won't allow DYNAMIC_FTRACE_WITH_REGS to be selected without
HAVE_DYNAMIC_FTRACE_WITH_REGS. Testing with a compiler that does
support -fpatchable-function-entry, I get the following, as expected:

$ grep -E '(DYNAMIC_FTRACE|MCOUNT_USE)' .config
CONFIG_HAVE_DYNAMIC_FTRACE=y
CONFIG_HAVE_DYNAMIC_FTRACE_WITH_REGS=y
CONFIG_DYNAMIC_FTRACE=y
CONFIG_DYNAMIC_FTRACE_WITH_REGS=y
CONFIG_FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY=y

And if the compiler doesn't support -fpatchable-function-entry, we
would end up with the following:

$ grep -E '(DYNAMIC_FTRACE|MCOUNT_USE)' .config
CONFIG_HAVE_DYNAMIC_FTRACE=y
CONFIG_DYNAMIC_FTRACE=y
CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT=y

Sami
diff mbox series

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6d232837cbee..ad522b021f35 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -155,6 +155,8 @@  config ARM64
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_DYNAMIC_FTRACE_WITH_REGS \
 		if $(cc-option,-fpatchable-function-entry=2)
+	select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY \
+		if DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS
 	select HAVE_FAST_GUP
 	select HAVE_FTRACE_MCOUNT_RECORD