Support multi-buffer on uprobe-based dynamic events by
using ftrace_event_file.
This patch is based kprobe-based dynamic events multibuffer
support work initially, commited by Masami(commit 41a7dd420c),
but revised as below:
Oleg changed the kprobe-based multibuffer design from
array-pointers of ftrace_event_file into simple list,
so this patch also change to the list degisn.
rcu_read_lock/unlock added into uprobe_trace_func/uretprobe_trace_func,
to synchronize with ftrace_event_file list add and delete.
Even though we allow multi-uprobes instances now,
but TP_FLAG_PROFILE/TP_FLAG_TRACE are still mutually exclusive
in probe_event_enable currently, this means we cannot allow
one user is using uprobe-tracer, and another user is using
perf-probe on same uprobe concurrently.
(Perhaps this will be fix in future, kprobe dont't have this
limitation now)
Signed-off-by: zhangwei(Jovi) <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Srikar Dronamraju <[email protected]>
---
kernel/trace/trace_uprobe.c | 118 +++++++++++++++++++++++++++++++++++--------
1 file changed, 97 insertions(+), 21 deletions(-)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 32494fb0..dbbb4a9 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -53,6 +53,7 @@ struct trace_uprobe {
struct list_head list;
struct ftrace_event_class class;
struct ftrace_event_call call;
+ struct list_head files;
struct trace_uprobe_filter filter;
struct uprobe_consumer consumer;
struct inode *inode;
@@ -65,6 +66,11 @@ struct trace_uprobe {
struct probe_arg args[];
};
+struct event_file_link {
+ struct ftrace_event_file *file;
+ struct list_head list;
+};
+
#define SIZEOF_TRACE_UPROBE(n) \
(offsetof(struct trace_uprobe, args) + \
(sizeof(struct probe_arg) * (n)))
@@ -124,6 +130,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
goto error;
INIT_LIST_HEAD(&tu->list);
+ INIT_LIST_HEAD(&tu->files);
tu->consumer.handler = uprobe_dispatcher;
if (is_ret)
tu->consumer.ret_handler = uretprobe_dispatcher;
@@ -511,7 +518,8 @@ static const struct file_operations uprobe_profile_ops = {
};
static void uprobe_trace_print(struct trace_uprobe *tu,
- unsigned long func, struct pt_regs *regs)
+ unsigned long func, struct pt_regs *regs,
+ struct ftrace_event_file *ftrace_file)
{
struct uprobe_trace_entry_head *entry;
struct ring_buffer_event *event;
@@ -520,9 +528,12 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
int size, i;
struct ftrace_event_call *call = &tu->call;
+ WARN_ON(call != ftrace_file->event_call);
+
size = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));
- event = trace_current_buffer_lock_reserve(&buffer, call->event.type,
- size + tu->size, 0, 0);
+ event = trace_event_buffer_lock_reserve(&buffer, ftrace_file,
+ call->event.type,
+ size + tu->size, 0, 0);
if (!event)
return;
@@ -546,15 +557,28 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
/* uprobe handler */
static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
{
- if (!is_ret_probe(tu))
- uprobe_trace_print(tu, 0, regs);
+ struct event_file_link *link;
+
+ if (is_ret_probe(tu))
+ return 0;
+
+ rcu_read_lock();
+ list_for_each_entry(link, &tu->files, list)
+ uprobe_trace_print(tu, 0, regs, link->file);
+ rcu_read_unlock();
+
return 0;
}
static void uretprobe_trace_func(struct trace_uprobe *tu, unsigned long func,
struct pt_regs *regs)
{
- uprobe_trace_print(tu, func, regs);
+ struct event_file_link *link;
+
+ rcu_read_lock();
+ list_for_each_entry(link, &tu->files, list)
+ uprobe_trace_print(tu, func, regs, link->file);
+ rcu_read_unlock();
}
/* Event entry printers */
@@ -605,33 +629,84 @@ typedef bool (*filter_func_t)(struct uprobe_consumer *self,
struct mm_struct *mm);
static int
-probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
+probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
+ filter_func_t filter)
{
+ int enabled = 0;
int ret = 0;
+ /* we cannot call uprobe_register twice for same tu */
if (is_trace_uprobe_enabled(tu))
- return -EINTR;
+ enabled = 1;
+
+ if (file) {
+ struct event_file_link *link;
+
+ if (tu->flags & TP_FLAG_PROFILE)
+ return -EINTR;
+
+ link = kmalloc(sizeof(*link), GFP_KERNEL);
+ if (!link)
+ return -ENOMEM;
+
+ link->file = file;
+ list_add_rcu(&link->list, &tu->files);
+
+ tu->flags |= TP_FLAG_TRACE;
+ } else {
+ if (tu->flags & TP_FLAG_TRACE)
+ return -EINTR;
+
+ tu->flags |= TP_FLAG_PROFILE;
+ }
WARN_ON(!uprobe_filter_is_empty(&tu->filter));
- tu->flags |= flag;
- tu->consumer.filter = filter;
- ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
- if (ret)
- tu->flags &= ~flag;
+ if (!enabled) {
+ tu->consumer.filter = filter;
+ ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
+ if (ret)
+ tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
+ }
return ret;
}
-static void probe_event_disable(struct trace_uprobe *tu, int flag)
+static struct event_file_link *
+find_event_file_link(struct trace_uprobe *tu, struct ftrace_event_file *file)
{
- if (!is_trace_uprobe_enabled(tu))
- return;
+ struct event_file_link *link;
+
+ list_for_each_entry(link, &tu->files, list)
+ if (link->file == file)
+ return link;
+
+ return NULL;
+}
+
+static void
+probe_event_disable(struct trace_uprobe *tu, struct ftrace_event_file *file)
+{
+ if (file) {
+ struct event_file_link *link;
+
+ link = find_event_file_link(tu, file);
+ if (!link)
+ return;
+
+ list_del_rcu(&link->list);
+ /* synchronize with uprobe_trace_func/uretprobe_trace_func */
+ synchronize_sched();
+ kfree(link);
+
+ if (!list_empty(&tu->files))
+ return;
+ }
WARN_ON(!uprobe_filter_is_empty(&tu->filter));
uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
- tu->flags &= ~flag;
+ tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
}
static int uprobe_event_define_fields(struct ftrace_event_call *event_call)
@@ -867,21 +942,22 @@ static
int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type, void *data)
{
struct trace_uprobe *tu = event->data;
+ struct ftrace_event_file *file = data;
switch (type) {
case TRACE_REG_REGISTER:
- return probe_event_enable(tu, TP_FLAG_TRACE, NULL);
+ return probe_event_enable(tu, file, NULL);
case TRACE_REG_UNREGISTER:
- probe_event_disable(tu, TP_FLAG_TRACE);
+ probe_event_disable(tu, file);
return 0;
#ifdef CONFIG_PERF_EVENTS
case TRACE_REG_PERF_REGISTER:
- return probe_event_enable(tu, TP_FLAG_PROFILE, uprobe_perf_filter);
+ return probe_event_enable(tu, NULL, uprobe_perf_filter);
case TRACE_REG_PERF_UNREGISTER:
- probe_event_disable(tu, TP_FLAG_PROFILE);
+ probe_event_disable(tu, NULL);
return 0;
case TRACE_REG_PERF_OPEN:
--
1.7.9.7
(2013/06/25 12:30), zhangwei(Jovi) wrote:
> @@ -605,33 +629,84 @@ typedef bool (*filter_func_t)(struct uprobe_consumer *self,
> struct mm_struct *mm);
>
> static int
> -probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
> +probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
> + filter_func_t filter)
> {
> + int enabled = 0;
> int ret = 0;
>
> + /* we cannot call uprobe_register twice for same tu */
> if (is_trace_uprobe_enabled(tu))
> - return -EINTR;
> + enabled = 1;
> +
> + if (file) {
> + struct event_file_link *link;
> +
> + if (tu->flags & TP_FLAG_PROFILE)
> + return -EINTR;
> +
> + link = kmalloc(sizeof(*link), GFP_KERNEL);
> + if (!link)
> + return -ENOMEM;
> +
> + link->file = file;
> + list_add_rcu(&link->list, &tu->files);
Maybe, list_add_tail_rcu() ? :) (but this is a minor one)
Other parts looks good for me.
Reviewed-by: Masami Hiramatsu <[email protected]>
Thank you,
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
Sorry again, didn't have time to review, will try tomorrow.
Looks good but a couple of minor nits, and perhaps we should
fix the bugs with unregister_trace_uprobe first... in kprobes
too _if_ I am right. I'll return tomorrow.
On 06/25, zhangwei(Jovi) wrote:
>
> -probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
> +probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
> + filter_func_t filter)
> {
> + int enabled = 0;
> int ret = 0;
>
> + /* we cannot call uprobe_register twice for same tu */
> if (is_trace_uprobe_enabled(tu))
> - return -EINTR;
> + enabled = 1;
Cosmetic again, "int enabled = 0" and then "if (is_trace_uprobe_enabled)"
looks a bit strange,
enabled = is_trace_uprobe_enabled();
looks a bit more clean.
> + if (file) {
> + struct event_file_link *link;
> +
> + if (tu->flags & TP_FLAG_PROFILE)
> + return -EINTR;
> +
> + link = kmalloc(sizeof(*link), GFP_KERNEL);
> + if (!link)
> + return -ENOMEM;
> +
> + link->file = file;
> + list_add_rcu(&link->list, &tu->files);
I agree with Masami, list_add_rcu_tail() looks better even if this
doesn't really matter.
Oleg.
* zhangwei(Jovi) <[email protected]> [2013-06-25 11:30:20]:
> Support multi-buffer on uprobe-based dynamic events by
> using ftrace_event_file.
>
> This patch is based kprobe-based dynamic events multibuffer
> support work initially, commited by Masami(commit 41a7dd420c),
> but revised as below:
>
> Oleg changed the kprobe-based multibuffer design from
> array-pointers of ftrace_event_file into simple list,
> so this patch also change to the list degisn.
>
> rcu_read_lock/unlock added into uprobe_trace_func/uretprobe_trace_func,
> to synchronize with ftrace_event_file list add and delete.
>
> Even though we allow multi-uprobes instances now,
> but TP_FLAG_PROFILE/TP_FLAG_TRACE are still mutually exclusive
> in probe_event_enable currently, this means we cannot allow
> one user is using uprobe-tracer, and another user is using
> perf-probe on same uprobe concurrently.
> (Perhaps this will be fix in future, kprobe dont't have this
> limitation now)
>
> Signed-off-by: zhangwei(Jovi) <[email protected]>
> Cc: Masami Hiramatsu <[email protected]>
> Cc: Frederic Weisbecker <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Srikar Dronamraju <[email protected]>
> ---
> kernel/trace/trace_uprobe.c | 118 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 97 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 32494fb0..dbbb4a9 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -53,6 +53,7 @@ struct trace_uprobe {
> struct list_head list;
> struct ftrace_event_class class;
> struct ftrace_event_call call;
> + struct list_head files;
> struct trace_uprobe_filter filter;
> struct uprobe_consumer consumer;
> struct inode *inode;
> @@ -65,6 +66,11 @@ struct trace_uprobe {
> struct probe_arg args[];
> };
>
> +struct event_file_link {
> + struct ftrace_event_file *file;
> + struct list_head list;
> +};
> +
> #define SIZEOF_TRACE_UPROBE(n) \
> (offsetof(struct trace_uprobe, args) + \
> (sizeof(struct probe_arg) * (n)))
> @@ -124,6 +130,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
> goto error;
>
> INIT_LIST_HEAD(&tu->list);
> + INIT_LIST_HEAD(&tu->files);
> tu->consumer.handler = uprobe_dispatcher;
> if (is_ret)
> tu->consumer.ret_handler = uretprobe_dispatcher;
> @@ -511,7 +518,8 @@ static const struct file_operations uprobe_profile_ops = {
> };
>
> static void uprobe_trace_print(struct trace_uprobe *tu,
> - unsigned long func, struct pt_regs *regs)
> + unsigned long func, struct pt_regs *regs,
> + struct ftrace_event_file *ftrace_file)
> {
> struct uprobe_trace_entry_head *entry;
> struct ring_buffer_event *event;
> @@ -520,9 +528,12 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
> int size, i;
> struct ftrace_event_call *call = &tu->call;
>
> + WARN_ON(call != ftrace_file->event_call);
> +
> size = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));
> - event = trace_current_buffer_lock_reserve(&buffer, call->event.type,
> - size + tu->size, 0, 0);
> + event = trace_event_buffer_lock_reserve(&buffer, ftrace_file,
> + call->event.type,
> + size + tu->size, 0, 0);
> if (!event)
> return;
>
> @@ -546,15 +557,28 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
> /* uprobe handler */
> static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
> {
> - if (!is_ret_probe(tu))
> - uprobe_trace_print(tu, 0, regs);
> + struct event_file_link *link;
> +
> + if (is_ret_probe(tu))
> + return 0;
> +
> + rcu_read_lock();
> + list_for_each_entry(link, &tu->files, list)
> + uprobe_trace_print(tu, 0, regs, link->file);
> + rcu_read_unlock();
> +
> return 0;
> }
>
> static void uretprobe_trace_func(struct trace_uprobe *tu, unsigned long func,
> struct pt_regs *regs)
> {
> - uprobe_trace_print(tu, func, regs);
> + struct event_file_link *link;
> +
> + rcu_read_lock();
> + list_for_each_entry(link, &tu->files, list)
> + uprobe_trace_print(tu, func, regs, link->file);
> + rcu_read_unlock();
> }
>
> /* Event entry printers */
> @@ -605,33 +629,84 @@ typedef bool (*filter_func_t)(struct uprobe_consumer *self,
> struct mm_struct *mm);
>
> static int
> -probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
> +probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
> + filter_func_t filter)
> {
> + int enabled = 0;
> int ret = 0;
>
> + /* we cannot call uprobe_register twice for same tu */
> if (is_trace_uprobe_enabled(tu))
> - return -EINTR;
> + enabled = 1;
> +
> + if (file) {
> + struct event_file_link *link;
> +
> + if (tu->flags & TP_FLAG_PROFILE)
> + return -EINTR;
> +
> + link = kmalloc(sizeof(*link), GFP_KERNEL);
> + if (!link)
> + return -ENOMEM;
> +
> + link->file = file;
> + list_add_rcu(&link->list, &tu->files);
> +
> + tu->flags |= TP_FLAG_TRACE;
> + } else {
> + if (tu->flags & TP_FLAG_TRACE)
> + return -EINTR;
> +
> + tu->flags |= TP_FLAG_PROFILE;
> + }
>
> WARN_ON(!uprobe_filter_is_empty(&tu->filter));
>
> - tu->flags |= flag;
> - tu->consumer.filter = filter;
> - ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
> - if (ret)
> - tu->flags &= ~flag;
> + if (!enabled) {
> + tu->consumer.filter = filter;
> + ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
> + if (ret)
> + tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
Dont we need to free link here? or where does the link that got
allocated freed?
Think the list of files also needs to be cleaned up. No?
> + }
>
> return ret;
> }
>
> -static void probe_event_disable(struct trace_uprobe *tu, int flag)
> +static struct event_file_link *
> +find_event_file_link(struct trace_uprobe *tu, struct ftrace_event_file *file)
> {
> - if (!is_trace_uprobe_enabled(tu))
> - return;
> + struct event_file_link *link;
> +
> + list_for_each_entry(link, &tu->files, list)
> + if (link->file == file)
> + return link;
> +
> + return NULL;
> +}
> +
> +static void
> +probe_event_disable(struct trace_uprobe *tu, struct ftrace_event_file *file)
> +{
> + if (file) {
> + struct event_file_link *link;
> +
> + link = find_event_file_link(tu, file);
> + if (!link)
> + return;
> +
> + list_del_rcu(&link->list);
> + /* synchronize with uprobe_trace_func/uretprobe_trace_func */
> + synchronize_sched();
> + kfree(link);
> +
> + if (!list_empty(&tu->files))
> + return;
> + }
>
> WARN_ON(!uprobe_filter_is_empty(&tu->filter));
>
> uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
> - tu->flags &= ~flag;
> + tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
> }
>
> static int uprobe_event_define_fields(struct ftrace_event_call *event_call)
> @@ -867,21 +942,22 @@ static
> int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type, void *data)
> {
> struct trace_uprobe *tu = event->data;
> + struct ftrace_event_file *file = data;
>
> switch (type) {
> case TRACE_REG_REGISTER:
> - return probe_event_enable(tu, TP_FLAG_TRACE, NULL);
> + return probe_event_enable(tu, file, NULL);
>
> case TRACE_REG_UNREGISTER:
> - probe_event_disable(tu, TP_FLAG_TRACE);
> + probe_event_disable(tu, file);
> return 0;
>
> #ifdef CONFIG_PERF_EVENTS
> case TRACE_REG_PERF_REGISTER:
> - return probe_event_enable(tu, TP_FLAG_PROFILE, uprobe_perf_filter);
> + return probe_event_enable(tu, NULL, uprobe_perf_filter);
>
> case TRACE_REG_PERF_UNREGISTER:
> - probe_event_disable(tu, TP_FLAG_PROFILE);
> + probe_event_disable(tu, NULL);
> return 0;
>
> case TRACE_REG_PERF_OPEN:
> --
> 1.7.9.7
>
>
--
Thanks and Regards
Srikar Dronamraju
On 06/27, Srikar Dronamraju wrote:
>
> * zhangwei(Jovi) <[email protected]> [2013-06-25 11:30:20]:
> > + if (!enabled) {
> > + tu->consumer.filter = filter;
> > + ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
> > + if (ret)
> > + tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
>
> Dont we need to free link here? or where does the link that got
> allocated freed?
Agreed...
Masami, it seems that (just in case, with or without "Turn trace_probe->files
into list_head" I sent) trace_kpobes needs the similar fix too? Plus it should
clear TP_FLAG_* if enable_k.*probe() fails.
Or enable_trace_probe() assumes that enable_kprobe() must succeed? In this
case probably WARN_ON(ret) makes sense.
Oleg.
(2013/06/28 1:27), Oleg Nesterov wrote:
> On 06/27, Srikar Dronamraju wrote:
>>
>> * zhangwei(Jovi) <[email protected]> [2013-06-25 11:30:20]:
>>> + if (!enabled) {
>>> + tu->consumer.filter = filter;
>>> + ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
>>> + if (ret)
>>> + tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
>>
>> Dont we need to free link here? or where does the link that got
>> allocated freed?
>
> Agreed...
>
> Masami, it seems that (just in case, with or without "Turn trace_probe->files
> into list_head" I sent) trace_kpobes needs the similar fix too? Plus it should
> clear TP_FLAG_* if enable_k.*probe() fails.
Oops, right! this problem also happens on the latest kernel. I must fix that
before introducing list_head...
> Or enable_trace_probe() assumes that enable_kprobe() must succeed? In this
> case probably WARN_ON(ret) makes sense.
In the case of probing a module function, the event can be gone when the
module is unloaded. At that time, enable_trace_probe must fails.
BTW, Steven, could you control the traffic of recent tracing works? :)
There are several works on the ftrace we made, but each of them
sometimes conflicting. We'd better make a merge plan or a working
branch on your tracing tree which allows us to work together on it.
Thank you!
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
(2013/06/28 13:17), Masami Hiramatsu wrote:
> (2013/06/28 1:27), Oleg Nesterov wrote:
>> On 06/27, Srikar Dronamraju wrote:
>>>
>>> * zhangwei(Jovi) <[email protected]> [2013-06-25 11:30:20]:
>>>> + if (!enabled) {
>>>> + tu->consumer.filter = filter;
>>>> + ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
>>>> + if (ret)
>>>> + tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
>>>
>>> Dont we need to free link here? or where does the link that got
>>> allocated freed?
>>
>> Agreed...
>>
>> Masami, it seems that (just in case, with or without "Turn trace_probe->files
>> into list_head" I sent) trace_kpobes needs the similar fix too? Plus it should
>> clear TP_FLAG_* if enable_k.*probe() fails.
>
> Oops, right! this problem also happens on the latest kernel. I must fix that
> before introducing list_head...
>
>> Or enable_trace_probe() assumes that enable_kprobe() must succeed? In this
>> case probably WARN_ON(ret) makes sense.
>
> In the case of probing a module function, the event can be gone when the
> module is unloaded. At that time, enable_trace_probe must fails.
Hmm, I've looked into it carefully, and found that the enable_kprobe() must succeed
because the enable_trace_probe() invokes it after checking the failure conditions
(kprobe is registered and not gone). But anyway, that depends on the current
implementation. I think we need both of WARN_ON() and writeback.
Thank you,
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
On 2013/6/27 20:12, Srikar Dronamraju wrote:
> * zhangwei(Jovi) <[email protected]> [2013-06-25 11:30:20]:
>
>> Support multi-buffer on uprobe-based dynamic events by
>> using ftrace_event_file.
>>
>> This patch is based kprobe-based dynamic events multibuffer
>> support work initially, commited by Masami(commit 41a7dd420c),
>> but revised as below:
>>
>> Oleg changed the kprobe-based multibuffer design from
>> array-pointers of ftrace_event_file into simple list,
>> so this patch also change to the list degisn.
>>
>> rcu_read_lock/unlock added into uprobe_trace_func/uretprobe_trace_func,
>> to synchronize with ftrace_event_file list add and delete.
>>
>> Even though we allow multi-uprobes instances now,
>> but TP_FLAG_PROFILE/TP_FLAG_TRACE are still mutually exclusive
>> in probe_event_enable currently, this means we cannot allow
>> one user is using uprobe-tracer, and another user is using
>> perf-probe on same uprobe concurrently.
>> (Perhaps this will be fix in future, kprobe dont't have this
>> limitation now)
>>
>> Signed-off-by: zhangwei(Jovi) <[email protected]>
>> Cc: Masami Hiramatsu <[email protected]>
>> Cc: Frederic Weisbecker <[email protected]>
>> Cc: Oleg Nesterov <[email protected]>
>> Cc: Srikar Dronamraju <[email protected]>
>> ---
>> kernel/trace/trace_uprobe.c | 118 +++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 97 insertions(+), 21 deletions(-)
>>
>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
>> index 32494fb0..dbbb4a9 100644
>> --- a/kernel/trace/trace_uprobe.c
>> +++ b/kernel/trace/trace_uprobe.c
>> @@ -53,6 +53,7 @@ struct trace_uprobe {
>> struct list_head list;
>> struct ftrace_event_class class;
>> struct ftrace_event_call call;
>> + struct list_head files;
>> struct trace_uprobe_filter filter;
>> struct uprobe_consumer consumer;
>> struct inode *inode;
>> @@ -65,6 +66,11 @@ struct trace_uprobe {
>> struct probe_arg args[];
>> };
>>
>> +struct event_file_link {
>> + struct ftrace_event_file *file;
>> + struct list_head list;
>> +};
>> +
>> #define SIZEOF_TRACE_UPROBE(n) \
>> (offsetof(struct trace_uprobe, args) + \
>> (sizeof(struct probe_arg) * (n)))
>> @@ -124,6 +130,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
>> goto error;
>>
>> INIT_LIST_HEAD(&tu->list);
>> + INIT_LIST_HEAD(&tu->files);
>> tu->consumer.handler = uprobe_dispatcher;
>> if (is_ret)
>> tu->consumer.ret_handler = uretprobe_dispatcher;
>> @@ -511,7 +518,8 @@ static const struct file_operations uprobe_profile_ops = {
>> };
>>
>> static void uprobe_trace_print(struct trace_uprobe *tu,
>> - unsigned long func, struct pt_regs *regs)
>> + unsigned long func, struct pt_regs *regs,
>> + struct ftrace_event_file *ftrace_file)
>> {
>> struct uprobe_trace_entry_head *entry;
>> struct ring_buffer_event *event;
>> @@ -520,9 +528,12 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
>> int size, i;
>> struct ftrace_event_call *call = &tu->call;
>>
>> + WARN_ON(call != ftrace_file->event_call);
>> +
>> size = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));
>> - event = trace_current_buffer_lock_reserve(&buffer, call->event.type,
>> - size + tu->size, 0, 0);
>> + event = trace_event_buffer_lock_reserve(&buffer, ftrace_file,
>> + call->event.type,
>> + size + tu->size, 0, 0);
>> if (!event)
>> return;
>>
>> @@ -546,15 +557,28 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
>> /* uprobe handler */
>> static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
>> {
>> - if (!is_ret_probe(tu))
>> - uprobe_trace_print(tu, 0, regs);
>> + struct event_file_link *link;
>> +
>> + if (is_ret_probe(tu))
>> + return 0;
>> +
>> + rcu_read_lock();
>> + list_for_each_entry(link, &tu->files, list)
>> + uprobe_trace_print(tu, 0, regs, link->file);
>> + rcu_read_unlock();
>> +
>> return 0;
>> }
>>
>> static void uretprobe_trace_func(struct trace_uprobe *tu, unsigned long func,
>> struct pt_regs *regs)
>> {
>> - uprobe_trace_print(tu, func, regs);
>> + struct event_file_link *link;
>> +
>> + rcu_read_lock();
>> + list_for_each_entry(link, &tu->files, list)
>> + uprobe_trace_print(tu, func, regs, link->file);
>> + rcu_read_unlock();
>> }
>>
>> /* Event entry printers */
>> @@ -605,33 +629,84 @@ typedef bool (*filter_func_t)(struct uprobe_consumer *self,
>> struct mm_struct *mm);
>>
>> static int
>> -probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
>> +probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
>> + filter_func_t filter)
>> {
>> + int enabled = 0;
>> int ret = 0;
>>
>> + /* we cannot call uprobe_register twice for same tu */
>> if (is_trace_uprobe_enabled(tu))
>> - return -EINTR;
>> + enabled = 1;
>> +
>> + if (file) {
>> + struct event_file_link *link;
>> +
>> + if (tu->flags & TP_FLAG_PROFILE)
>> + return -EINTR;
>> +
>> + link = kmalloc(sizeof(*link), GFP_KERNEL);
>> + if (!link)
>> + return -ENOMEM;
>> +
>> + link->file = file;
>> + list_add_rcu(&link->list, &tu->files);
>> +
>> + tu->flags |= TP_FLAG_TRACE;
>> + } else {
>> + if (tu->flags & TP_FLAG_TRACE)
>> + return -EINTR;
>> +
>> + tu->flags |= TP_FLAG_PROFILE;
>> + }
>>
>> WARN_ON(!uprobe_filter_is_empty(&tu->filter));
>>
>> - tu->flags |= flag;
>> - tu->consumer.filter = filter;
>> - ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
>> - if (ret)
>> - tu->flags &= ~flag;
>> + if (!enabled) {
>> + tu->consumer.filter = filter;
>> + ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
>> + if (ret)
>> + tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
>
> Dont we need to free link here? or where does the link that got
> allocated freed?
>
> Think the list of files also needs to be cleaned up. No?
>
Thanks for review, I will update it in next series.
jovi
On Fri, 2013-06-28 at 13:17 +0900, Masami Hiramatsu wrote:
> BTW, Steven, could you control the traffic of recent tracing works? :)
> There are several works on the ftrace we made, but each of them
> sometimes conflicting. We'd better make a merge plan or a working
> branch on your tracing tree which allows us to work together on it.
After I pull in patches end test it, I push it up to my for-next branch.
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
-- Steve
(2013/06/28 20:51), Steven Rostedt wrote:
> On Fri, 2013-06-28 at 13:17 +0900, Masami Hiramatsu wrote:
>
>> BTW, Steven, could you control the traffic of recent tracing works? :)
>> There are several works on the ftrace we made, but each of them
>> sometimes conflicting. We'd better make a merge plan or a working
>> branch on your tracing tree which allows us to work together on it.
>
> After I pull in patches end test it, I push it up to my for-next branch.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
>
> -- Steve
Thanks!
So, I'll make a fix on the branch. :)
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
Make enable_trace_probe to recover (writeback) the old file array
and free new one if we fail to enable the kprobe.
However, this MUST NOT happen at this time except for unknown
bug or changing the implementation of enable_kprobe(), because
usual failure cases (not registered or gone) are already filtered.
Thus I just add a WARN_ON() on the error path.
Signed-off-by: Masami Hiramatsu <[email protected]>
Reported-by: Oleg Nesterov <[email protected]>
Reported-by: Srikar Dronamraju <[email protected]>
Cc: "zhangwei(Jovi)" <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
kernel/trace/trace_kprobe.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index f237417..d29773e 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -202,6 +202,20 @@ static int trace_probe_nr_files(struct trace_probe *tp)
static DEFINE_MUTEX(probe_enable_lock);
+static int __enable_trace_probe(struct trace_probe *tp)
+{
+ int ret = 0;
+
+ if (trace_probe_is_registered(tp) && !trace_probe_has_gone(tp)) {
+ if (trace_probe_is_return(tp))
+ ret = enable_kretprobe(&tp->rp);
+ else
+ ret = enable_kprobe(&tp->rp.kp);
+ WARN_ON(ret);/* This must succeed. */
+ }
+ return ret;
+}
+
/*
* Enable trace_probe
* if the file is NULL, enable "perf" handler, or enable "trace" handler.
@@ -232,19 +246,21 @@ enable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file)
rcu_assign_pointer(tp->files, new);
tp->flags |= TP_FLAG_TRACE;
+ ret = __enable_trace_probe(tp);
+ if (ret < 0) {
+ /* Write back the old list */
+ rcu_assign_pointer(tp->files, old);
+ old = new; /* "new" must be freed */
+ }
+
if (old) {
/* Make sure the probe is done with old files */
synchronize_sched();
kfree(old);
}
- } else
+ } else {
tp->flags |= TP_FLAG_PROFILE;
-
- if (trace_probe_is_registered(tp) && !trace_probe_has_gone(tp)) {
- if (trace_probe_is_return(tp))
- ret = enable_kretprobe(&tp->rp);
- else
- ret = enable_kprobe(&tp->rp.kp);
+ ret = __enable_trace_probe(tp);
}
out_unlock:
On 06/28, Masami Hiramatsu wrote:
>
> @@ -232,19 +246,21 @@ enable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file)
> rcu_assign_pointer(tp->files, new);
> tp->flags |= TP_FLAG_TRACE;
>
> + ret = __enable_trace_probe(tp);
> + if (ret < 0) {
> + /* Write back the old list */
> + rcu_assign_pointer(tp->files, old);
> + old = new; /* "new" must be freed */
> + }
> +
> if (old) {
> /* Make sure the probe is done with old files */
> synchronize_sched();
> kfree(old);
> }
Ah, but this conflicts with the other changes I sent. They have
your acks, and iiuc Steven is going to apply them.
Besides, this fix is not complete afaics, we should also clear
TP_FLAG_TRACE/PROFILE if __enable_trace_probe() fails.
Perhaps you can do this later, on top of the pending changes?
Or. Given that this patch assumes that enable_kprobe() must succed,
can't we make a minimal change for now?
-------------------------------------------------------------------------------
[PATCH] tracing/kprobe: WARN() if enable_kprobe() fails.
enable_trace_probe() doesn't recover tp->files/flags if enable_kprobe()
fails, this looks confusing.
However, enable_kprobe() must not fail at this time except for unknown
bug or changing the implementation of enable_kprobe(), because usual
failure cases (not registered or gone) are already filtered.
So this patch simply adds WARN_ON(ret) to document this fact, even if
it makes sense to cleanup the logic anyway later.
Reported-by: Srikar Dronamraju <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
---
kernel/trace/trace_kprobe.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 5c070db..bb608b5 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -220,6 +220,7 @@ enable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file)
ret = enable_kretprobe(&tp->rp);
else
ret = enable_kprobe(&tp->rp.kp);
+ WARN_ON(ret);
}
out:
return ret;
On Fri, 2013-06-28 at 16:27 +0200, Oleg Nesterov wrote:
> On 06/28, Masami Hiramatsu wrote:
> >
> > @@ -232,19 +246,21 @@ enable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file)
> > rcu_assign_pointer(tp->files, new);
> > tp->flags |= TP_FLAG_TRACE;
> >
> > + ret = __enable_trace_probe(tp);
> > + if (ret < 0) {
> > + /* Write back the old list */
> > + rcu_assign_pointer(tp->files, old);
> > + old = new; /* "new" must be freed */
> > + }
> > +
> > if (old) {
> > /* Make sure the probe is done with old files */
> > synchronize_sched();
> > kfree(old);
> > }
>
> Ah, but this conflicts with the other changes I sent. They have
> your acks, and iiuc Steven is going to apply them.
I'll see if I can solve any conflicts. I need to get my -rt versions out
and start on the new 3.6 stable today. Then after that, I plan on going
though and getting all the tracing patches settled.
Thanks,
-- Steve
>
> Besides, this fix is not complete afaics, we should also clear
> TP_FLAG_TRACE/PROFILE if __enable_trace_probe() fails.
>
> Perhaps you can do this later, on top of the pending changes?
>
> Or. Given that this patch assumes that enable_kprobe() must succed,
> can't we make a minimal change for now?
>
> -------------------------------------------------------------------------------
> [PATCH] tracing/kprobe: WARN() if enable_kprobe() fails.
>
> enable_trace_probe() doesn't recover tp->files/flags if enable_kprobe()
> fails, this looks confusing.
>
> However, enable_kprobe() must not fail at this time except for unknown
> bug or changing the implementation of enable_kprobe(), because usual
> failure cases (not registered or gone) are already filtered.
>
> So this patch simply adds WARN_ON(ret) to document this fact, even if
> it makes sense to cleanup the logic anyway later.
>
> Reported-by: Srikar Dronamraju <[email protected]>
> Signed-off-by: Oleg Nesterov <[email protected]>
> ---
> kernel/trace/trace_kprobe.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5c070db..bb608b5 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -220,6 +220,7 @@ enable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file)
> ret = enable_kretprobe(&tp->rp);
> else
> ret = enable_kprobe(&tp->rp.kp);
> + WARN_ON(ret);
> }
> out:
> return ret;
On 06/28, Steven Rostedt wrote:
>
> On Fri, 2013-06-28 at 16:27 +0200, Oleg Nesterov wrote:
> >
> > Ah, but this conflicts with the other changes I sent. They have
> > your acks, and iiuc Steven is going to apply them.
>
> I'll see if I can solve any conflicts. I need to get my -rt versions out
> and start on the new 3.6 stable today. Then after that, I plan on going
> though and getting all the tracing patches settled.
Thanks!
> > Besides, this fix is not complete afaics, we should also clear
> > TP_FLAG_TRACE/PROFILE if __enable_trace_probe() fails.
Yes.
And I forgot to mention, until we fix the races we discuss in another
thread, this WARN_ON() doesn't look right. So perhaps it would be
really better to delay this change a bit.
Oleg.
> > Perhaps you can do this later, on top of the pending changes?
> >
> > Or. Given that this patch assumes that enable_kprobe() must succed,
> > can't we make a minimal change for now?
> >
> > -------------------------------------------------------------------------------
> > [PATCH] tracing/kprobe: WARN() if enable_kprobe() fails.
> >
> > enable_trace_probe() doesn't recover tp->files/flags if enable_kprobe()
> > fails, this looks confusing.
> >
> > However, enable_kprobe() must not fail at this time except for unknown
> > bug or changing the implementation of enable_kprobe(), because usual
> > failure cases (not registered or gone) are already filtered.
> >
> > So this patch simply adds WARN_ON(ret) to document this fact, even if
> > it makes sense to cleanup the logic anyway later.
> >
> > Reported-by: Srikar Dronamraju <[email protected]>
> > Signed-off-by: Oleg Nesterov <[email protected]>
> > ---
> > kernel/trace/trace_kprobe.c | 1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 5c070db..bb608b5 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -220,6 +220,7 @@ enable_trace_probe(struct trace_probe *tp, struct ftrace_event_file *file)
> > ret = enable_kretprobe(&tp->rp);
> > else
> > ret = enable_kprobe(&tp->rp.kp);
> > + WARN_ON(ret);
> > }
> > out:
> > return ret;
>
>
(2013/06/29 3:43), Oleg Nesterov wrote:
> On 06/28, Steven Rostedt wrote:
>>
>> On Fri, 2013-06-28 at 16:27 +0200, Oleg Nesterov wrote:
>>>
>>> Ah, but this conflicts with the other changes I sent. They have
>>> your acks, and iiuc Steven is going to apply them.
>>
>> I'll see if I can solve any conflicts. I need to get my -rt versions out
>> and start on the new 3.6 stable today. Then after that, I plan on going
>> though and getting all the tracing patches settled.
>
> Thanks!
>
>>> Besides, this fix is not complete afaics, we should also clear
>>> TP_FLAG_TRACE/PROFILE if __enable_trace_probe() fails.
>
> Yes.
>
> And I forgot to mention, until we fix the races we discuss in another
> thread, this WARN_ON() doesn't look right. So perhaps it would be
> really better to delay this change a bit.
Agreed, this fix is not a critical one. The racing problem of
dynamic events is what we have to solve at first.
Thank you!
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]
On Sun, 2013-06-30 at 16:46 +0900, Masami Hiramatsu wrote:
> (2013/06/29 3:43), Oleg Nesterov wrote:
> > On 06/28, Steven Rostedt wrote:
> >>
> >> On Fri, 2013-06-28 at 16:27 +0200, Oleg Nesterov wrote:
> >>>
> >>> Ah, but this conflicts with the other changes I sent. They have
> >>> your acks, and iiuc Steven is going to apply them.
> >>
> >> I'll see if I can solve any conflicts. I need to get my -rt versions out
> >> and start on the new 3.6 stable today. Then after that, I plan on going
> >> though and getting all the tracing patches settled.
> >
> > Thanks!
> >
> >>> Besides, this fix is not complete afaics, we should also clear
> >>> TP_FLAG_TRACE/PROFILE if __enable_trace_probe() fails.
> >
> > Yes.
> >
> > And I forgot to mention, until we fix the races we discuss in another
> > thread, this WARN_ON() doesn't look right. So perhaps it would be
> > really better to delay this change a bit.
>
> Agreed, this fix is not a critical one. The racing problem of
> dynamic events is what we have to solve at first.
>
Do you want to reapply this on top of my current for-next branch? Or can
this wait?
Thanks,
-- Steve
(2013/07/03 8:02), Steven Rostedt wrote:
> On Sun, 2013-06-30 at 16:46 +0900, Masami Hiramatsu wrote:
>> (2013/06/29 3:43), Oleg Nesterov wrote:
>>> On 06/28, Steven Rostedt wrote:
>>>>
>>>> On Fri, 2013-06-28 at 16:27 +0200, Oleg Nesterov wrote:
>>>>>
>>>>> Ah, but this conflicts with the other changes I sent. They have
>>>>> your acks, and iiuc Steven is going to apply them.
>>>>
>>>> I'll see if I can solve any conflicts. I need to get my -rt versions out
>>>> and start on the new 3.6 stable today. Then after that, I plan on going
>>>> though and getting all the tracing patches settled.
>>>
>>> Thanks!
>>>
>>>>> Besides, this fix is not complete afaics, we should also clear
>>>>> TP_FLAG_TRACE/PROFILE if __enable_trace_probe() fails.
>>>
>>> Yes.
>>>
>>> And I forgot to mention, until we fix the races we discuss in another
>>> thread, this WARN_ON() doesn't look right. So perhaps it would be
>>> really better to delay this change a bit.
>>
>> Agreed, this fix is not a critical one. The racing problem of
>> dynamic events is what we have to solve at first.
>>
>
> Do you want to reapply this on top of my current for-next branch? Or can
> this wait?
This is a minor one (and must not happen), and AFAICS fixing all the problem
around this requires more works i.e. exporting event_mutex etc.(as I said)
I'll do that asap. :)
Thank you,
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]