Messages in this thread | | | Date | Tue, 18 Jun 2019 12:23:22 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH 10/21] tracing/probe: Split trace_event related data from trace_probe |
| |
On Wed, 19 Jun 2019 01:14:09 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote:
> On Mon, 17 Jun 2019 21:56:43 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: >
> > > +static nokprobe_inline struct trace_kprobe * > > > +trace_kprobe_primary_from_call(struct trace_event_call *call) > > > +{ > > > + struct trace_probe *tp = trace_probe_primary_from_call(call); > > > + > > > + return container_of(tp, struct trace_kprobe, tp); > > > > > > Hmm, is there a possibility that trace_probe_primary_from_call() may > > not have a primary? > > Good question! Of course if given event_call is not a kprobe event, > it doesn't have primary (or any) trace_probe. But that must not happen > unless user misuses it. > And that list never be the empty, when the last trace probe is released, > the event_call also unregistered and released. See unregister_trace_kprobe() > for details. If there is no siblings on the list, the event_call is also > unregistered before unregistering kprobes, and after unregistering kprobes > the list is unlinked. > (Note that unregister_kprobe() will wait a quiescence period > before return. This means all probe handlers are done before that.)
Yeah, I thought something like that. But perhaps the trace_probe_primary_from_call() code should add a WARN_ON() is the list is empty.
>
> > > > > > - ret = __enable_trace_kprobe(tk); > > > - if (ret) { > > > + enabled = false; > > > + list_for_each_entry(pos, trace_probe_probe_list(tp), list) { > > > + tk = container_of(pos, struct trace_kprobe, tp); > > > + ecode = __enable_trace_kprobe(tk); > > > + if (ecode) > > > + ret = ecode; /* Save the last error code */ > > > + else > > > + enabled = true; > > > > So, if we have some enabled but return an error code, what should a > > caller think of that? Wouldn't it be an inconsistent state? > > Oops, good catch! > This part is related to caller (ftrace/perf) so should be more careful. > Usually, kprobe enablement should not fail. If one of them has > gone (like a probe on unloaded module), it can be fail but that > should be ignored. I would like to add some additional check so that > - If all kprobes are on the module which is unloaded, enablement > must be failed and return error. > - If any kprobe is enabled, and others are on non-exist modules, > it should succeeded and return OK. > - If any kprobe caused an error not because of unloaded module, > all other enablement should be canceled and return error. > > Is that OK for you? >
Sounds good to me.
-- Steve
| |