On Mon, 29 Mar 2021 16:05:32 +0300
"Yordan Karadzhov (VMware)" <y.kar...@gmail.com> wrote:
> Currently the logic for dealing with the options for function tracing
> has two different implementations. One is used when we set the flags
> (in "static int func_set_flag()") and another used when we initialize
> the tracer (in "static int function_trace_init()"). Those two
> implementations are meant to do essentially the same thing and they
> are both not very convenient for adding new options. In this patch
> we add a helper function that provides a single implementation of
> the logic for dealing with the options and we make it such that new
> options can be easily added.
>
> Signed-off-by: Yordan Karadzhov (VMware) <y.kar...@gmail.com>
> ---
> kernel/trace/trace_functions.c | 66 ++++++++++++++++++++--------------
> 1 file changed, 40 insertions(+), 26 deletions(-)
>
> diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> index f93723ca66bc..6c912eb0508a 100644
> --- a/kernel/trace/trace_functions.c
> +++ b/kernel/trace/trace_functions.c
> @@ -27,13 +27,17 @@ function_trace_call(unsigned long ip, unsigned long
> parent_ip,
> static void
> function_stack_trace_call(unsigned long ip, unsigned long parent_ip,
> struct ftrace_ops *op, struct ftrace_regs *fregs);
> +static ftrace_func_t select_trace_function(u32 flags_val);
> static struct tracer_flags func_flags;
>
> /* Our option */
> enum {
> + TRACE_FUNC_NO_OPTS = 0x0, /* No flags set. */
> TRACE_FUNC_OPT_STACK = 0x1,
> };
>
> +#define TRACE_FUNC_OPT_MASK (TRACE_FUNC_OPT_STACK)
> +
> int ftrace_allocate_ftrace_ops(struct trace_array *tr)
> {
> struct ftrace_ops *ops;
> @@ -97,12 +101,9 @@ static int function_trace_init(struct trace_array *tr)
> if (!tr->ops)
> return -ENOMEM;
>
> - /* Currently only the global instance can do stack tracing */
> - if (tr->flags & TRACE_ARRAY_FL_GLOBAL &&
> - func_flags.val & TRACE_FUNC_OPT_STACK)
> - func = function_stack_trace_call;
> - else
> - func = function_trace_call;
> + func = select_trace_function(func_flags.val);
> + if (!func)
> + return -EINVAL;
>
> ftrace_init_array_ops(tr, func);
>
> @@ -205,6 +206,18 @@ function_stack_trace_call(unsigned long ip, unsigned
> long parent_ip,
> local_irq_restore(flags);
> }
>
> +static ftrace_func_t select_trace_function(u32 flags_val)
> +{
> + switch (flags_val & TRACE_FUNC_OPT_MASK) {
> + case TRACE_FUNC_NO_OPTS:
> + return function_trace_call;
> + case TRACE_FUNC_OPT_STACK:
> + return function_stack_trace_call;
> + default:
> + return NULL;
> + }
> +}
Is there a reason why you defined this function here and not before its
first use? When defining functions, I tend to try to define them before
their first use to not need to declare the static prototype above.
The reasons for doing the static prototype and using a static function is
usually because of #ifdef around the first use, and keeping the function
from being hidden by the #ifdef, or the static function already exists, and
then gets used in a function before it, where it's just easier to add the
static declaration than moving the function.
> +
> static struct tracer_opt func_opts[] = {
> #ifdef CONFIG_STACKTRACE
> { TRACER_OPT(func_stack_trace, TRACE_FUNC_OPT_STACK) },
> @@ -213,7 +226,7 @@ static struct tracer_opt func_opts[] = {
> };
>
> static struct tracer_flags func_flags = {
> - .val = 0, /* By default: all flags disabled */
> + .val = TRACE_FUNC_NO_OPTS, /* By default: all flags disabled */
> .opts = func_opts
> };
>
> @@ -235,30 +248,31 @@ static struct tracer function_trace;
> static int
> func_set_flag(struct trace_array *tr, u32 old_flags, u32 bit, int set)
> {
> - switch (bit) {
> - case TRACE_FUNC_OPT_STACK:
> - /* do nothing if already set */
> - if (!!set == !!(func_flags.val & TRACE_FUNC_OPT_STACK))
> - break;
> + ftrace_func_t func;
> + u32 new_flags_val;
Nit, but the variable should just be "new_flags", which is consistent with
old_flags. In the kernel we don't need to the variable names to be so
verbose.
>
> - /* We can change this flag when not running. */
> - if (tr->current_trace != &function_trace)
> - break;
> + /* Do nothing if already set. */
> + if (!!set == !!(func_flags.val & bit))
> + return 0;
>
> - unregister_ftrace_function(tr->ops);
> + /* We can change this flag only when not running. */
> + if (tr->current_trace != &function_trace)
> + return 0;
>
> - if (set) {
> - tr->ops->func = function_stack_trace_call;
> - register_ftrace_function(tr->ops);
> - } else {
> - tr->ops->func = function_trace_call;
> - register_ftrace_function(tr->ops);
> - }
> + new_flags_val = (func_flags.val & ~(1UL << (bit - 1)));
> + new_flags_val |= (set << (bit - 1));
bit is already the mask, no need to shift it, nor there's no reason for the
extra set of parenthesis. And the above can be done in one line.
new_flags = (func_flags.val & ~bit) | (set ? bit : 0);
-- Steve
>
> - break;
> - default:
> + func = select_trace_function(new_flags_val);
> + if (!func)
> return -EINVAL;
> - }
> +
> + /* Check if there's anything to change. */
> + if (tr->ops->func == func)
> + return 0;
> +
> + unregister_ftrace_function(tr->ops);
> + tr->ops->func = func;
> + register_ftrace_function(tr->ops);
>
> return 0;
> }
