[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20240525184414.a9e1953e0a9cd390b3e75513@kernel.org>
Date: Sat, 25 May 2024 18:44:14 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, Florent Revest
<revest@...omium.org>, linux-trace-kernel@...r.kernel.org, LKML
<linux-kernel@...r.kernel.org>, Martin KaFai Lau <martin.lau@...ux.dev>,
bpf <bpf@...r.kernel.org>, Sven Schnelle <svens@...ux.ibm.com>, Alexei
Starovoitov <ast@...nel.org>, Jiri Olsa <jolsa@...nel.org>, Arnaldo
Carvalho de Melo <acme@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
Alan Maguire <alan.maguire@...cle.com>, Mark Rutland
<mark.rutland@....com>, Peter Zijlstra <peterz@...radead.org>, Thomas
Gleixner <tglx@...utronix.de>, Guo Ren <guoren@...nel.org>
Subject: Re: [PATCH v10 07/36] function_graph: Allow multiple users to
attach to function graph
On Fri, 24 May 2024 21:32:08 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:
> On Tue, 7 May 2024 23:09:22 +0900
> "Masami Hiramatsu (Google)" <mhiramat@...nel.org> wrote:
>
> > @@ -109,6 +244,21 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func,
> > if (!current->ret_stack)
> > return -EBUSY;
> >
> > + /*
> > + * At first, check whether the previous fgraph callback is pushed by
> > + * the fgraph on the same function entry.
> > + * But if @func is the self tail-call function, we also need to ensure
> > + * the ret_stack is not for the previous call by checking whether the
> > + * bit of @fgraph_idx is set or not.
> > + */
> > + ret_stack = get_ret_stack(current, current->curr_ret_stack, &offset);
> > + if (ret_stack && ret_stack->func == func &&
> > + get_fgraph_type(current, offset + FGRAPH_FRAME_OFFSET) == FGRAPH_TYPE_BITMAP &&
> > + !is_fgraph_index_set(current, offset + FGRAPH_FRAME_OFFSET, fgraph_idx))
> > + return offset + FGRAPH_FRAME_OFFSET;
> > +
> > + val = (FGRAPH_TYPE_RESERVED << FGRAPH_TYPE_SHIFT) | FGRAPH_FRAME_OFFSET;
> > +
> > BUILD_BUG_ON(SHADOW_STACK_SIZE % sizeof(long));
>
> I'm trying to figure out what the above is trying to do. This gets called
> once in function_graph_enter() (or function_graph_enter_ops()). What
> exactly are you trying to catch here?
Aah, good catch! This was originally for catching the self tail-call case with
multiple fgraph callback on the same function, but it was my misread.
In later patch ([12/36]), we introduced function_graph_enter_ops() so that
we can skip checking hash table and directly pass the fgraph_ops to user
callback. I thought this function_graph_enter_ops() is used even if multiple
fgraph is set on the same function. In this case, we always need to check the
stack can be reused(pushed by other fgraph_ops on the same function) or not.
But as we discussed, the function_graph_enter_ops() is used only when only
one fgraph is set on the function (if there are multiple fgraphs are set on
the same function, use function_graph_enter() ), we are sure that
ftrace_push_return_trace() is called only once on hooking the function entry.
Thus we don't need to reuse it.
>
> Is it from this email:
>
> https://lore.kernel.org/all/20231110105154.df937bf9f200a0c16806c522@kernel.org/
>
> As that's the last version before you added the above code.
>
> But you also noticed it may not be needed, but triggered a crash without it
> in v3:
>
> https://lore.kernel.org/all/20231205234511.3839128259dfec153ea7da81@kernel.org/
>
> I removed this code in my version and it runs just fine. Perhaps there was
> another bug that this was hiding that you fixed in later versions?
No problem. I think we can remove this block safely.
Thank you,
>
> -- Steve
>
--
Masami Hiramatsu (Google) <mhiramat@...nel.org>
Powered by blists - more mailing lists