Re: [PATCH 10/21] tracing/probe: Split trace_event related data from trace_probe

From: Steven Rostedt <rostedt@goodmis.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>,
	linux-kernel@vger.kernel.org,
	Tom Zanussi <tom.zanussi@linux.intel.com>,
	Ravi Bangoria <ravi.bangoria@linux.ibm.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>
Subject: Re: [PATCH 10/21] tracing/probe: Split trace_event related data from trace_probe
Date: Mon, 17 Jun 2019 21:56:43 -0400	[thread overview]
Message-ID: <20190617215643.05a33541@oasis.local.home> (raw)
In-Reply-To: <155931589667.28323.6107724588059072406.stgit@devnote2>

On Sat,  1 Jun 2019 00:18:16 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Split the trace_event related data from trace_probe data structure
> and introduce trace_probe_event data structure for its folder.
> This trace_probe_event data structure can have multiple trace_probe.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_kprobe.c |   99 ++++++++++++++++++++++-------------
>  kernel/trace/trace_probe.c  |   53 +++++++++++++------
>  kernel/trace/trace_probe.h  |   48 +++++++++++++----
>  kernel/trace/trace_uprobe.c |  123 +++++++++++++++++++++++++++++--------------
>  4 files changed, 221 insertions(+), 102 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 9d483ad9bb6c..633edb88cd0e 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -180,9 +180,17 @@ unsigned long trace_kprobe_address(struct trace_kprobe *tk)
>  	return addr;
>  }
>  
> +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?

> +}
> +
>  bool trace_kprobe_on_func_entry(struct trace_event_call *call)
>  {
> -	struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
> +	struct trace_kprobe *tk = trace_kprobe_primary_from_call(call);
>  
>  	return kprobe_on_func_entry(tk->rp.kp.addr,
>  			tk->rp.kp.addr ? NULL : tk->rp.kp.symbol_name,
> @@ -191,7 +199,7 @@ bool trace_kprobe_on_func_entry(struct trace_event_call *call)
>  
>  bool trace_kprobe_error_injectable(struct trace_event_call *call)
>  {
> -	struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
> +	struct trace_kprobe *tk = trace_kprobe_primary_from_call(call);
>  
>  	return within_error_injection_list(trace_kprobe_address(tk));
>  }
> @@ -295,28 +303,40 @@ static inline int __enable_trace_kprobe(struct trace_kprobe *tk)
>   * Enable trace_probe
>   * if the file is NULL, enable "perf" handler, or enable "trace" handler.
>   */
> -static int
> -enable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file)
> +static int enable_trace_kprobe(struct trace_event_call *call,
> +				struct trace_event_file *file)
>  {
> -	bool enabled = trace_probe_is_enabled(&tk->tp);
> -	int ret = 0;
> +	struct trace_probe *pos, *tp = trace_probe_primary_from_call(call);
> +	struct trace_kprobe *tk;
> +	bool enabled = trace_probe_is_enabled(tp);
> +	int ret = 0, ecode;
>  
>  	if (file) {
> -		ret = trace_probe_add_file(&tk->tp, file);
> +		ret = trace_probe_add_file(tp, file);
>  		if (ret)
>  			return ret;
>  	} else
> -		trace_probe_set_flag(&tk->tp, TP_FLAG_PROFILE);
> +		trace_probe_set_flag(tp, TP_FLAG_PROFILE);
>  
>  	if (enabled)
>  		return 0;
>  
> -	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?

-- Steve

> +	}
> +
> +	if (!enabled) {
> +		/* No probe is enabled. Roll back */
>  		if (file)
> -			trace_probe_remove_file(&tk->tp, file);
> +			trace_probe_remove_file(tp, file);
>  		else
> -			trace_probe_clear_flag(&tk->tp, TP_FLAG_PROFILE);
> +			trace_probe_clear_flag(tp, TP_FLAG_PROFILE);
>  	}
>  
>

> +static inline struct trace_probe_event *
> +trace_probe_event_from_call(struct trace_event_call *event_call)
> +{
> +	return container_of(event_call, struct trace_probe_event, call);
> +}
> +
> +static inline struct trace_probe *
> +trace_probe_primary_from_call(struct trace_event_call *call)
> +{
> +	struct trace_probe_event *tpe = trace_probe_event_from_call(call);
> +
> +	return list_first_entry(&tpe->probes, struct trace_probe, list);
> +}
> +
> +static inline struct list_head *trace_probe_probe_list(struct trace_probe *tp)
> +{
> +	return &tp->event->probes;
>  }
>  