Received-SPF: pass (google.com: domain of linux-kernel+bounces-159105-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <171318533841.254850.15841395205784342850.stgit@devnote2> <171318575984.254850.17464878774926779209.stgit@devnote2>
In-Reply-To: <171318575984.254850.17464878774926779209.stgit@devnote2>
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date: Thu, 25 Apr 2024 13:15:08 -0700
Message-ID: <CAEf4BzZz_4RGyam5GW6Do3Z-sCtk2Cj2D6rYyciYOcJihKdDww@mail.gmail.com>
Subject: Re: [PATCH v9 36/36] fgraph: Skip recording calltime/rettime if it is
 not nneeded
To: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>, Steven Rostedt <rostedt@goodmis.org>, 
	Florent Revest <revest@chromium.org>, linux-trace-kernel@vger.kernel.org, 
	LKML <linux-kernel@vger.kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>, 
	bpf <bpf@vger.kernel.org>, Sven Schnelle <svens@linux.ibm.com>, 
	Alexei Starovoitov <ast@kernel.org>, Jiri Olsa <jolsa@kernel.org>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, 
	Alan Maguire <alan.maguire@oracle.com>, Mark Rutland <mark.rutland@arm.com>, 
	Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>, Guo Ren <guoren@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 15, 2024 at 6:25=E2=80=AFAM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
>
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Skip recording calltime and rettime if the fgraph_ops does not need it.
> This is a kind of performance optimization for fprobe. Since the fprobe
> user does not use these entries, recording timestamp in fgraph is just
> a overhead (e.g. eBPF, ftrace). So introduce the skip_timestamp flag,
> and all fgraph_ops sets this flag, skip recording calltime and rettime.
>
> Suggested-by: Jiri Olsa <olsajiri@gmail.com>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Changes in v9:
>   - Newly added.
> ---
>  include/linux/ftrace.h |    2 ++
>  kernel/trace/fgraph.c  |   46 +++++++++++++++++++++++++++++++++++++++---=
----
>  kernel/trace/fprobe.c  |    1 +
>  3 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index d845a80a3d56..06fc7cbef897 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -1156,6 +1156,8 @@ struct fgraph_ops {
>         struct ftrace_ops               ops; /* for the hash lists */
>         void                            *private;
>         int                             idx;
> +       /* If skip_timestamp is true, this does not record timestamps. */
> +       bool                            skip_timestamp;
>  };
>
>  void *fgraph_reserve_data(int idx, int size_bytes);
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 7556fbbae323..a5722537bb79 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -131,6 +131,7 @@ DEFINE_STATIC_KEY_FALSE(kill_ftrace_graph);
>  int ftrace_graph_active;
>
>  static struct fgraph_ops *fgraph_array[FGRAPH_ARRAY_SIZE];
> +static bool fgraph_skip_timestamp;
>
>  /* LRU index table for fgraph_array */
>  static int fgraph_lru_table[FGRAPH_ARRAY_SIZE];
> @@ -475,7 +476,7 @@ void ftrace_graph_stop(void)
>  static int
>  ftrace_push_return_trace(unsigned long ret, unsigned long func,
>                          unsigned long frame_pointer, unsigned long *retp=
,
> -                        int fgraph_idx)
> +                        int fgraph_idx, bool skip_ts)
>  {
>         struct ftrace_ret_stack *ret_stack;
>         unsigned long long calltime;
> @@ -498,8 +499,12 @@ ftrace_push_return_trace(unsigned long ret, unsigned=
 long func,
>         ret_stack =3D get_ret_stack(current, current->curr_ret_stack, &in=
dex);
>         if (ret_stack && ret_stack->func =3D=3D func &&
>             get_fgraph_type(current, index + FGRAPH_RET_INDEX) =3D=3D FGR=
APH_TYPE_BITMAP &&
> -           !is_fgraph_index_set(current, index + FGRAPH_RET_INDEX, fgrap=
h_idx))
> +           !is_fgraph_index_set(current, index + FGRAPH_RET_INDEX, fgrap=
h_idx)) {
> +               /* If previous one skips calltime, update it. */
> +               if (!skip_ts && !ret_stack->calltime)
> +                       ret_stack->calltime =3D trace_clock_local();
>                 return index + FGRAPH_RET_INDEX;
> +       }
>
>         val =3D (FGRAPH_TYPE_RESERVED << FGRAPH_TYPE_SHIFT) | FGRAPH_RET_=
INDEX;
>
> @@ -517,7 +522,10 @@ ftrace_push_return_trace(unsigned long ret, unsigned=
 long func,
>                 return -EBUSY;
>         }
>
> -       calltime =3D trace_clock_local();
> +       if (skip_ts)

would it be ok to add likely() here to keep the least-overhead code path li=
near?

> +               calltime =3D 0LL;
> +       else
> +               calltime =3D trace_clock_local();
>
>         index =3D READ_ONCE(current->curr_ret_stack);
>         ret_stack =3D RET_STACK(current, index);
> @@ -601,7 +609,8 @@ int function_graph_enter_regs(unsigned long ret, unsi=
gned long func,
>         trace.func =3D func;
>         trace.depth =3D ++current->curr_ret_depth;
>
> -       index =3D ftrace_push_return_trace(ret, func, frame_pointer, retp=
, 0);
> +       index =3D ftrace_push_return_trace(ret, func, frame_pointer, retp=
, 0,
> +                                        fgraph_skip_timestamp);
>         if (index < 0)
>                 goto out;
>
> @@ -654,7 +663,8 @@ int function_graph_enter_ops(unsigned long ret, unsig=
ned long func,
>                 return -ENODEV;
>
>         /* Use start for the distance to ret_stack (skipping over reserve=
) */
> -       index =3D ftrace_push_return_trace(ret, func, frame_pointer, retp=
, gops->idx);
> +       index =3D ftrace_push_return_trace(ret, func, frame_pointer, retp=
, gops->idx,
> +                                        gops->skip_timestamp);
>         if (index < 0)
>                 return index;
>         type =3D get_fgraph_type(current, index);
> @@ -732,6 +742,7 @@ ftrace_pop_return_trace(struct ftrace_graph_ret *trac=
e, unsigned long *ret,
>         *ret =3D ret_stack->ret;
>         trace->func =3D ret_stack->func;
>         trace->calltime =3D ret_stack->calltime;
> +       trace->rettime =3D 0;
>         trace->overrun =3D atomic_read(&current->trace_overrun);
>         trace->depth =3D current->curr_ret_depth;
>         /*
> @@ -792,7 +803,6 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs,=
 unsigned long frame_pointe
>                 return (unsigned long)panic;
>         }
>
> -       trace.rettime =3D trace_clock_local();
>         if (fregs)
>                 ftrace_regs_set_instruction_pointer(fregs, ret);
>
> @@ -808,6 +818,8 @@ __ftrace_return_to_handler(struct ftrace_regs *fregs,=
 unsigned long frame_pointe
>                         continue;
>                 if (gops =3D=3D &fgraph_stub)
>                         continue;
> +               if (!trace.rettime && !gops->skip_timestamp)

In addition to the above, do you mind adding unlikely() here as well?

> +                       trace.rettime =3D trace_clock_local();
>
>                 gops->retfunc(&trace, gops, fregs);
>         }

[...]