All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chengming Zhou <zhouchengming@bytedance.com>
To: Mark Rutland <mark.rutland@arm.com>, rostedt@goodmis.org
Cc: mingo@redhat.com, catalin.marinas@arm.com, will@kernel.org,
	tglx@linutronix.de, bp@alien8.de, dave.hansen@linux.intel.com,
	x86@kernel.org, hpa@zytor.com, broonie@kernel.org,
	ardb@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, duanxiongchun@bytedance.com,
	songmuchun@bytedance.com
Subject: Re: [External] Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable
Date: Wed, 20 Apr 2022 23:30:03 +0800	[thread overview]
Message-ID: <5cf976c4-c555-c546-e82c-7482b50fa14a@bytedance.com> (raw)
In-Reply-To: <Yl6f0XKoRxNhgGPv@FVFF77S0Q05N>

Hi,

On 2022/4/19 19:41, Mark Rutland wrote:
> On Sat, Apr 09, 2022 at 11:35:53PM +0800, Chengming Zhou wrote:
>> The ftrace_[enable,disable]_ftrace_graph_caller() are used to do
>> special hooks for graph tracer, which are not needed on some ARCHs
>> that use graph_ops:func function to install return_hooker.
>>
>> So introduce the weak version in ftrace core code to cleanup
>> in x86.
>>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>> ---
>> v4:
>>  - put weak ftrace_enable,disable_ftrace_graph_caller() in
>>    fgraph.c instead of ftrace.c as suggested by Steve.
>>
>> v3:
>>  - consolidate two #if into a single #if, suggested by Steve. Thanks.
>> ---
>>  arch/x86/kernel/ftrace.c | 17 ++---------------
>>  kernel/trace/fgraph.c    | 18 ++++++++++++++++++
>>  2 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
>> index 1e31c7d21597..b09d73c2ba89 100644
>> --- a/arch/x86/kernel/ftrace.c
>> +++ b/arch/x86/kernel/ftrace.c
>> @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
>>  
>>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>  
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> -
>> -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
>> +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
>>  extern void ftrace_graph_call(void);
>>  static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
>>  {
>> @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void)
>>  
>>  	return ftrace_mod_jmp(ip, &ftrace_stub);
>>  }
>> -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>> -int ftrace_enable_ftrace_graph_caller(void)
>> -{
>> -	return 0;
>> -}
>> -
>> -int ftrace_disable_ftrace_graph_caller(void)
>> -{
>> -	return 0;
>> -}
>> -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>> -#endif /* !CONFIG_DYNAMIC_FTRACE */
>> +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>>  
>>  /*
>>   * Hook the return address and push it in the stack of return addrs
>> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
>> index 8f4fb328133a..289311680c29 100644
>> --- a/kernel/trace/fgraph.c
>> +++ b/kernel/trace/fgraph.c
>> @@ -30,6 +30,24 @@ int ftrace_graph_active;
>>  /* Both enabled by default (can be cleared by function_graph tracer flags */
>>  static bool fgraph_sleep_time = true;
>>  
>> +/*
>> + * archs can override this function if they must do something
>> + * to enable hook for graph tracer.
>> + */
>> +int __weak ftrace_enable_ftrace_graph_caller(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +/*
>> + * archs can override this function if they must do something
>> + * to disable hook for graph tracer.
>> + */
>> +int __weak ftrace_disable_ftrace_graph_caller(void)
>> +{
>> +	return 0;
>> +}
> 
> IIUC an arch should either:
> 
> * Have ftrace_graph_call()
> 
> * Have both ftrace_enable_ftrace_graph_caller() and
>   ftrace_disable_ftrace_graph_caller()
> 
> ... and I can't think of a reason an arch would need both ftrace_graph_call()
> *and* the enable/disable functions.

This way is more precise, but we have to add "#define ftrace_graph_call ftrace_graph_call"
in all ARCH's asm/ftrace.h correctly.

Previously we only need to define override ftrace_[enable,disable]_ftrace_graph_caller()
in ARCH's ftrace.c, looks like a little more complexity?

> 
> Given that, could we drop the `__weak` and place these within ifdeffery, i.e.
> make the above:
> 
> | #ifndef ftrace_graph_call
> | int ftrace_enable_ftrace_graph_caller(void) { return 0; }
> | int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> | #endif /* ftrace_graph_call *. 
> 
> That way we'd catch cases when:
> 
> * An architecture meant to provide one of these functions, but forgot (e.g. the
>   name got typo'd)
> 
> * An architecture provides an unnecessary implementation of either of these
>   functions.

I tried this way today, and it works too. I'm ok with both way. Maybe I should
send v5 with this patch unchanged first. Looking forward to Steve's opinion.

Thanks.

> 
> Regardless, this looks ok to me. Steve, are you happy with this? I suspect we'd
> need to take this via the arm64 tree with the next patch, so we'd need your Ack.
> 
> Thanks,
> Mark.
> 
>> +
>>  /**
>>   * ftrace_graph_stop - set to permanently disable function graph tracing
>>   *
>> -- 
>> 2.35.1
>>

WARNING: multiple messages have this Message-ID
From: Chengming Zhou <zhouchengming@bytedance.com>
To: Mark Rutland <mark.rutland@arm.com>, rostedt@goodmis.org
Cc: mingo@redhat.com, catalin.marinas@arm.com, will@kernel.org,
	tglx@linutronix.de, bp@alien8.de, dave.hansen@linux.intel.com,
	x86@kernel.org, hpa@zytor.com, broonie@kernel.org,
	ardb@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, duanxiongchun@bytedance.com,
	songmuchun@bytedance.com
Subject: Re: [External] Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable
Date: Wed, 20 Apr 2022 23:30:03 +0800	[thread overview]
Message-ID: <5cf976c4-c555-c546-e82c-7482b50fa14a@bytedance.com> (raw)
In-Reply-To: <Yl6f0XKoRxNhgGPv@FVFF77S0Q05N>

Hi,

On 2022/4/19 19:41, Mark Rutland wrote:
> On Sat, Apr 09, 2022 at 11:35:53PM +0800, Chengming Zhou wrote:
>> The ftrace_[enable,disable]_ftrace_graph_caller() are used to do
>> special hooks for graph tracer, which are not needed on some ARCHs
>> that use graph_ops:func function to install return_hooker.
>>
>> So introduce the weak version in ftrace core code to cleanup
>> in x86.
>>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>> ---
>> v4:
>>  - put weak ftrace_enable,disable_ftrace_graph_caller() in
>>    fgraph.c instead of ftrace.c as suggested by Steve.
>>
>> v3:
>>  - consolidate two #if into a single #if, suggested by Steve. Thanks.
>> ---
>>  arch/x86/kernel/ftrace.c | 17 ++---------------
>>  kernel/trace/fgraph.c    | 18 ++++++++++++++++++
>>  2 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
>> index 1e31c7d21597..b09d73c2ba89 100644
>> --- a/arch/x86/kernel/ftrace.c
>> +++ b/arch/x86/kernel/ftrace.c
>> @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
>>  
>>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>  
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> -
>> -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
>> +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
>>  extern void ftrace_graph_call(void);
>>  static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
>>  {
>> @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void)
>>  
>>  	return ftrace_mod_jmp(ip, &ftrace_stub);
>>  }
>> -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>> -int ftrace_enable_ftrace_graph_caller(void)
>> -{
>> -	return 0;
>> -}
>> -
>> -int ftrace_disable_ftrace_graph_caller(void)
>> -{
>> -	return 0;
>> -}
>> -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>> -#endif /* !CONFIG_DYNAMIC_FTRACE */
>> +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>>  
>>  /*
>>   * Hook the return address and push it in the stack of return addrs
>> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
>> index 8f4fb328133a..289311680c29 100644
>> --- a/kernel/trace/fgraph.c
>> +++ b/kernel/trace/fgraph.c
>> @@ -30,6 +30,24 @@ int ftrace_graph_active;
>>  /* Both enabled by default (can be cleared by function_graph tracer flags */
>>  static bool fgraph_sleep_time = true;
>>  
>> +/*
>> + * archs can override this function if they must do something
>> + * to enable hook for graph tracer.
>> + */
>> +int __weak ftrace_enable_ftrace_graph_caller(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +/*
>> + * archs can override this function if they must do something
>> + * to disable hook for graph tracer.
>> + */
>> +int __weak ftrace_disable_ftrace_graph_caller(void)
>> +{
>> +	return 0;
>> +}
> 
> IIUC an arch should either:
> 
> * Have ftrace_graph_call()
> 
> * Have both ftrace_enable_ftrace_graph_caller() and
>   ftrace_disable_ftrace_graph_caller()
> 
> ... and I can't think of a reason an arch would need both ftrace_graph_call()
> *and* the enable/disable functions.

This way is more precise, but we have to add "#define ftrace_graph_call ftrace_graph_call"
in all ARCH's asm/ftrace.h correctly.

Previously we only need to define override ftrace_[enable,disable]_ftrace_graph_caller()
in ARCH's ftrace.c, looks like a little more complexity?

> 
> Given that, could we drop the `__weak` and place these within ifdeffery, i.e.
> make the above:
> 
> | #ifndef ftrace_graph_call
> | int ftrace_enable_ftrace_graph_caller(void) { return 0; }
> | int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> | #endif /* ftrace_graph_call *. 
> 
> That way we'd catch cases when:
> 
> * An architecture meant to provide one of these functions, but forgot (e.g. the
>   name got typo'd)
> 
> * An architecture provides an unnecessary implementation of either of these
>   functions.

I tried this way today, and it works too. I'm ok with both way. Maybe I should
send v5 with this patch unchanged first. Looking forward to Steve's opinion.

Thanks.

> 
> Regardless, this looks ok to me. Steve, are you happy with this? I suspect we'd
> need to take this via the arm64 tree with the next patch, so we'd need your Ack.
> 
> Thanks,
> Mark.
> 
>> +
>>  /**
>>   * ftrace_graph_stop - set to permanently disable function graph tracing
>>   *
>> -- 
>> 2.35.1
>>

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

  reply	other threads:[~2022-04-20 15:30 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-09 15:35 [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou
2022-04-09 15:35 ` Chengming Zhou
2022-04-09 15:35 ` [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly Chengming Zhou
2022-04-09 15:35   ` Chengming Zhou
2022-04-19 12:55   ` Mark Rutland
2022-04-19 12:55     ` Mark Rutland
2022-04-19 13:27     ` [External] " Chengming Zhou
2022-04-19 13:27       ` Chengming Zhou
2022-04-18 13:24 ` [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou
2022-04-18 13:24   ` Chengming Zhou
2022-04-19 11:41 ` Mark Rutland
2022-04-19 11:41   ` Mark Rutland
2022-04-20 15:30   ` Chengming Zhou [this message]
2022-04-20 15:30     ` [External] " Chengming Zhou

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5cf976c4-c555-c546-e82c-7482b50fa14a@bytedance.com \
    --to=zhouchengming@bytedance.com \
    --cc=ardb@kernel.org \
    --cc=bp@alien8.de \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=duanxiongchun@bytedance.com \
    --cc=hpa@zytor.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=rostedt@goodmis.org \
    --cc=songmuchun@bytedance.com \
    --cc=tglx@linutronix.de \
    --cc=will@kernel.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.