Re: [RFC][PATCH 07/14] fgraph: Add new fgraph_ops structure to enable function graph hooks

[Joel Fernandes]

On Wed, Nov 21, 2018 at 08:27:15PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
> 
> Currently the registering of function graph is to pass in a entry and return
> function. We need to have a way to associate those functions together where
> the entry can determine to run the return hook. Having a structure that
> contains both functions will facilitate the process of converting the code
> to be able to do such.
> 
> This is similar to the way function hooks are enabled (it passes in
> ftrace_ops). Instead of passing in the functions to use, a single structure
> is passed in to the registering function.
> 
> The unregister function is now passed in the fgraph_ops handle. When we
> allow more than one callback to the function graph hooks, this will let the
> system know which one to remove.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> ---
>  include/linux/ftrace.h               | 24 +++++++++++++++++-------
>  kernel/trace/fgraph.c                |  9 ++++-----
>  kernel/trace/ftrace.c                | 10 +++++++---
>  kernel/trace/trace_functions_graph.c | 21 ++++++++++++++++-----
>  kernel/trace/trace_irqsoff.c         | 10 +++++++---
>  kernel/trace/trace_sched_wakeup.c    | 10 +++++++---
>  kernel/trace/trace_selftest.c        |  8 ++++++--
>  7 files changed, 64 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index f98063e273e5..477ff9412d26 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -749,6 +749,18 @@ typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *); /* entry */
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  
> +struct fgraph_ops {
> +	trace_func_graph_ent_t		entryfunc;
> +	trace_func_graph_ret_t		retfunc;
> +	struct fgraph_ops __rcu		*next;
> +	unsigned long			flags;
> +	void				*private;
> +#ifdef CONFIG_DYNAMIC_FTRACE
> +	struct ftrace_ops_hash		local_hash;
> +	struct ftrace_ops_hash		*func_hash;
> +#endif
> +};
> +
>  /*
>   * Stack of return addresses for functions
>   * of a thread.
> @@ -792,8 +804,9 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
>  
>  #define FTRACE_RETFUNC_DEPTH 50
>  #define FTRACE_RETSTACK_ALLOC_SIZE 32
> -extern int register_ftrace_graph(trace_func_graph_ret_t retfunc,
> -				trace_func_graph_ent_t entryfunc);
> +
> +extern int register_ftrace_graph(struct fgraph_ops *ops);
> +extern void unregister_ftrace_graph(struct fgraph_ops *ops);
>  
>  extern bool ftrace_graph_is_dead(void);
>  extern void ftrace_graph_stop(void);
> @@ -802,8 +815,6 @@ extern void ftrace_graph_stop(void);
>  extern trace_func_graph_ret_t ftrace_graph_return;
>  extern trace_func_graph_ent_t ftrace_graph_entry;
>  
> -extern void unregister_ftrace_graph(void);
> -
>  extern void ftrace_graph_init_task(struct task_struct *t);
>  extern void ftrace_graph_exit_task(struct task_struct *t);
>  extern void ftrace_graph_init_idle_task(struct task_struct *t, int cpu);
> @@ -830,12 +841,11 @@ static inline void ftrace_graph_init_task(struct task_struct *t) { }
>  static inline void ftrace_graph_exit_task(struct task_struct *t) { }
>  static inline void ftrace_graph_init_idle_task(struct task_struct *t, int cpu) { }
>  
> -static inline int register_ftrace_graph(trace_func_graph_ret_t retfunc,
> -			  trace_func_graph_ent_t entryfunc)
> +static inline int register_ftrace_graph(struct fgraph_ops *ops);
>  {
>  	return -1;
>  }
> -static inline void unregister_ftrace_graph(void) { }
> +static inline void unregister_ftrace_graph(struct fgraph_ops *ops) { }
>  
>  static inline int task_curr_ret_stack(struct task_struct *tsk)
>  {
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index b9c7dbbbdd96..f3a89ecac671 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -491,8 +491,7 @@ static int start_graph_tracing(void)
>  	return ret;
>  }
>  
> -int register_ftrace_graph(trace_func_graph_ret_t retfunc,
> -			trace_func_graph_ent_t entryfunc)
> +int register_ftrace_graph(struct fgraph_ops *gops)
>  {
>  	int ret = 0;
>  
> @@ -513,7 +512,7 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
>  		goto out;
>  	}
>  
> -	ftrace_graph_return = retfunc;
> +	ftrace_graph_return = gops->retfunc;
>  
>  	/*
>  	 * Update the indirect function to the entryfunc, and the
> @@ -521,7 +520,7 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
>  	 * call the update fgraph entry function to determine if
>  	 * the entryfunc should be called directly or not.
>  	 */
> -	__ftrace_graph_entry = entryfunc;
> +	__ftrace_graph_entry = gops->entryfunc;
>  	ftrace_graph_entry = ftrace_graph_entry_test;
>  	update_function_graph_func();
>  
> @@ -531,7 +530,7 @@ int register_ftrace_graph(trace_func_graph_ret_t retfunc,
>  	return ret;
>  }
>  
> -void unregister_ftrace_graph(void)
> +void unregister_ftrace_graph(struct fgraph_ops *gops)
>  {
>  	mutex_lock(&ftrace_lock);
>  
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 64e635994648..d057dde081e7 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -849,15 +849,19 @@ static void profile_graph_return(struct ftrace_graph_ret *trace)
>  	local_irq_restore(flags);
>  }
>  
> +static struct fgraph_ops fprofiler_ops = {
> +	.entryfunc = &profile_graph_entry,
> +	.retfunc = &profile_graph_return,
> +};
> +
>  static int register_ftrace_profiler(void)
>  {
> -	return register_ftrace_graph(&profile_graph_return,
> -				     &profile_graph_entry);
> +	return register_ftrace_graph(&fprofiler_ops);
>  }
>  
>  static void unregister_ftrace_profiler(void)
>  {
> -	unregister_ftrace_graph();
> +	unregister_ftrace_graph(&fprofiler_ops);
>  }
>  #else
>  static struct ftrace_ops ftrace_profile_ops __read_mostly = {
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 0e0ff08357cf..7c7fd13d2373 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -336,17 +336,25 @@ static void trace_graph_thresh_return(struct ftrace_graph_ret *trace)
>  		trace_graph_return(trace);
>  }
>  
> +static struct fgraph_ops funcgraph_threash_ops = {

minor nit: should be funcgraph_thresh_ops ?

thanks,

 - Joel