Re: [PATCH 1/4] tracing/uprobes: Revert "Support mix of ftrace and perf"

[Namhyung Kim <namhyung@gmail.com>]

Hi Oleg,

On Fri, 27 Jun 2014 19:01:36 +0200, Oleg Nesterov wrote:
> This reverts commit 43fe98913c9f67e3b523615ee3316f9520a623e0.
>
> This patch is very wrong. Firstly, this change leads to unbalanced
> uprobe_unregister(). Just for example,
>
> 	# perf probe -x /lib/libc.so.6 syscall
> 	# echo 1 >> /sys/kernel/debug/tracing/events/probe_libc/enable
> 	# perf record -e probe_libc:syscall whatever
>
> after that uprobe is dead (unregistered) but the user of ftrace/perf
> can't know this, and it looks as if nobody hits this probe.

Nah, I missed to check the unregister path.. :-/

>
> This would be easy to fix, but there are other reasons why it is not
> simple to mix ftrace and perf. If nothing else, they can't share the
> same ->consumer.filter. This is fixable too, but probably we need to
> fix the poorly designed uprobe_register() interface first. At least
> "register" and "apply" should be clearly separated.

Hmm.. right.  It seems the current filter logic only cares about the
perf.  If ftrace comes after perf, it might not see some events due to
the filter, right?

>
> Cc: stable@vger.kernel.org # v3.14
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  kernel/trace/trace_uprobe.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 04fdb5d..08e7970 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -893,6 +893,9 @@ probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
>  	int ret;
>  
>  	if (file) {
> +		if (tu->tp.flags & TP_FLAG_PROFILE)
> +			return -EINTR;
> +
>  		link = kmalloc(sizeof(*link), GFP_KERNEL);
>  		if (!link)
>  			return -ENOMEM;
> @@ -901,8 +904,12 @@ probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
>  		list_add_tail_rcu(&link->list, &tu->tp.files);
>  
>  		tu->tp.flags |= TP_FLAG_TRACE;
> -	} else
> +	} else {
> +		if (tu->tp.flags & TP_FLAG_TRACE)
> +			return -EINTR;
> +
>  		tu->tp.flags |= TP_FLAG_PROFILE;
> +	}
>  
>  	ret = uprobe_buffer_enable();
>  	if (ret < 0)