uprobes/tracing: cleanups and minor fixes

PATCH In review Oleg Nesterov 3 participants 18 messages Jan 31, 2013 – Feb 11, 2013 Tracing

lore

Share & tools

lore

Oleg Nesterov

author

2013-01-31 19:19 UTC #0 lore raw

cover uprobes/tracing: cleanups and minor fixes

Hello.

Cleanups and minor fixes in trace_uprobe.c. This is also preparation for the next changes.

Please review, I am going to add this to "oleg/misc uprobes/core" tree unless someone objects.

Oleg.

 kernel/trace/trace_probe.h  |    1 -
 kernel/trace/trace_uprobe.c |   57 ++++++++++++++++---------------------------
 2 files changed, 21 insertions(+), 37 deletions(-)

Oleg Nesterov

author

2013-01-31 19:19 UTC #3 lore raw

PATCH 3/6 uprobes/tracing: Ensure inode != NULL in create_trace_uprobe()

probe_event_enable/disable() check tu->inode != NULL at the start. This is ugly, if igrab() can fail create_trace_uprobe() should not succeed and "postpone" the failure.

Note: alloc_uprobe() should probably check igrab() != NULL as well.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_uprobe.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

kernel/trace/trace_uprobe.c +4 -2

index 7b75949..f02bbec 100644
@@ -255,6 +255,8 @@ static int create_trace_uprobe(int argc, char **argv)
 
 	inode = igrab(path.dentry->d_inode);
 	path_put(&path);
+	if (!inode)
+		goto fail_address_parse;
 
 	ret = kstrtoul(arg, 0, &offset);
 	if (ret)

@@ -539,7 +541,7 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 	struct uprobe_trace_consumer *utc;
 	int ret = 0;
 
-	if (!tu->inode || tu->consumer)
+	if (tu->consumer)
 		return -EINTR;
 
 	utc = kzalloc(sizeof(struct uprobe_trace_consumer), GFP_KERNEL);

@@ -563,7 +565,7 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 
 static void probe_event_disable(struct trace_uprobe *tu, int flag)
 {
-	if (!tu->inode || !tu->consumer)
+	if (!tu->consumer)
 		return;
 
 	uprobe_unregister(tu->inode, tu->offset, &tu->consumer->cons);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-01-31 19:19 UTC #12 lore raw

PATCH 6/6 uprobes/perf: Always increment trace_uprobe->nhit

In reply to Oleg Nesterov

Move tu->nhit++ from uprobe_trace_func() to uprobe_dispatcher().

->nhit counts how many time we hit the breakpoint inserted by this uprobe, we do not want to loose this info if uprobe was enabled by sys_perf_event_open().

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_uprobe.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

kernel/trace/trace_uprobe.c +1 -2

index 55cdc14..0a9a8de 100644
@@ -473,8 +473,6 @@ static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 	unsigned long irq_flags;
 	struct ftrace_event_call *call = &tu->call;
 
-	tu->nhit++;
-
 	local_save_flags(irq_flags);
 	pc = preempt_count();

@@ -698,6 +696,7 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
 	struct trace_uprobe *tu;
 
 	tu = container_of(con, struct trace_uprobe, consumer);
+	tu->nhit++;
 
 	if (tu->flags & TP_FLAG_TRACE)
 		uprobe_trace_func(tu, regs);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-01-31 19:19 UTC #7 lore raw

PATCH 5/6 uprobes/tracing: Kill uprobe_trace_consumer, embed uprobe_consumer into trace_uprobe

In reply to Oleg Nesterov

trace_uprobe->consumer and "struct uprobe_trace_consumer" add the unnecessary indirection and complicate the code for no reason.

This patch simply embeds uprobe_consumer into "struct trace_uprobe", all other changes only fix the compilation errors.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_uprobe.c |   35 ++++++-----------------------------
 1 files changed, 6 insertions(+), 29 deletions(-)

kernel/trace/trace_uprobe.c +6 ~~-29~~

index 947379a..55cdc14 100644
@@ -31,17 +31,11 @@
 /*
  * uprobe event core functions
  */
-struct trace_uprobe;
-struct uprobe_trace_consumer {
-	struct uprobe_consumer		cons;
-	struct trace_uprobe		*tu;
-};
-
 struct trace_uprobe {
 	struct list_head		list;
 	struct ftrace_event_class	class;
 	struct ftrace_event_call	call;
-	struct uprobe_trace_consumer	*consumer;
+	struct uprobe_consumer		consumer;
 	struct inode			*inode;
 	char				*filename;
 	unsigned long			offset;

@@ -92,6 +86,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs)
 		goto error;
 
 	INIT_LIST_HEAD(&tu->list);
+	tu->consumer.handler = uprobe_dispatcher;
 	return tu;
 
 error:

@@ -543,27 +538,15 @@ static inline bool is_trace_uprobe_enabled(struct trace_uprobe *tu)
 
 static int probe_event_enable(struct trace_uprobe *tu, int flag)
 {
-	struct uprobe_trace_consumer *utc;
 	int ret = 0;
 
 	if (is_trace_uprobe_enabled(tu))
 		return -EINTR;
 
-	utc = kzalloc(sizeof(struct uprobe_trace_consumer), GFP_KERNEL);
-	if (!utc)
-		return -EINTR;
-
-	utc->cons.handler = uprobe_dispatcher;
-	utc->tu = tu;
-	tu->consumer = utc;
 	tu->flags |= flag;
-
-	ret = uprobe_register(tu->inode, tu->offset, &utc->cons);
-	if (ret) {
-		tu->consumer = NULL;
+	ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
+	if (ret)
 		tu->flags &= ~flag;
-		kfree(utc);
-	}
 
 	return ret;
 }

@@ -573,10 +556,8 @@ static void probe_event_disable(struct trace_uprobe *tu, int flag)
 	if (!is_trace_uprobe_enabled(tu))
 		return;
 
-	uprobe_unregister(tu->inode, tu->offset, &tu->consumer->cons);
+	uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
 	tu->flags &= ~flag;
-	kfree(tu->consumer);
-	tu->consumer = NULL;
 }
 
 static int uprobe_event_define_fields(struct ftrace_event_call *event_call)

@@ -714,13 +695,9 @@ int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type,
 
 static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
 {
-	struct uprobe_trace_consumer *utc;
 	struct trace_uprobe *tu;
 
-	utc = container_of(con, struct uprobe_trace_consumer, cons);
-	tu = utc->tu;
-	if (!tu || tu->consumer != utc)
-		return 0;
+	tu = container_of(con, struct trace_uprobe, consumer);
 
 	if (tu->flags & TP_FLAG_TRACE)
 		uprobe_trace_func(tu, regs);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-01-31 19:19 UTC #1 lore raw

PATCH 1/6 uprobes/tracing: Fix dentry/mount leak in create_trace_uprobe()

In reply to Oleg Nesterov

create_trace_uprobe() does kern_path() to find ->d_inode, but forgets to do path_put(). We can do this right after igrab().

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

---
 kernel/trace/trace_uprobe.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

kernel/trace/trace_uprobe.c +3 -2

index 7d5b407..6563f00 100644
@@ -253,12 +253,13 @@ static int create_trace_uprobe(int argc, char **argv)
 	if (ret)
 		goto fail_address_parse;
 
+	inode = igrab(path.dentry->d_inode);
+	path_put(&path);
+
 	ret = kstrtoul(arg, 0, &offset);
 	if (ret)
 		goto fail_address_parse;
 
-	inode = igrab(path.dentry->d_inode);
-
 	argc -= 2;
 	argv += 2;

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-01-31 19:19 UTC #5 lore raw

PATCH 4/6 uprobes/tracing: Introduce is_trace_uprobe_enabled()

In reply to Oleg Nesterov

probe_event_enable/disable() check tu->consumer != NULL to avoid the wrong uprobe_register/unregister().

We are going to kill this pointer and "struct uprobe_trace_consumer", so we add the new helper, is_trace_uprobe_enabled(), which can rely on TP_FLAG_TRACE/TP_FLAG_PROFILE instead.

Note: the current logic doesn't look optimal, it is not clear why TP_FLAG_TRACE/TP_FLAG_PROFILE are mutually exclusive, we will probably change this later.

Also kill the unused TP_FLAG_UPROBE.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_probe.h  |    1 -
 kernel/trace/trace_uprobe.c |    9 +++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

kernel/trace/trace_probe.h +0 -1

index 9337086..5c7e09d 100644
@@ -66,7 +66,6 @@
 #define TP_FLAG_TRACE		1
 #define TP_FLAG_PROFILE		2
 #define TP_FLAG_REGISTERED	4
-#define TP_FLAG_UPROBE		8
 
 
 /* data_rloc: data relative location, compatible with u32 */

kernel/trace/trace_uprobe.c +7 -2

index f02bbec..947379a 100644
@@ -536,12 +536,17 @@ partial:
 	return TRACE_TYPE_PARTIAL_LINE;
 }
 
+static inline bool is_trace_uprobe_enabled(struct trace_uprobe *tu)
+{
+	return tu->flags & (TP_FLAG_TRACE | TP_FLAG_PROFILE);
+}
+
 static int probe_event_enable(struct trace_uprobe *tu, int flag)
 {
 	struct uprobe_trace_consumer *utc;
 	int ret = 0;
 
-	if (tu->consumer)
+	if (is_trace_uprobe_enabled(tu))
 		return -EINTR;
 
 	utc = kzalloc(sizeof(struct uprobe_trace_consumer), GFP_KERNEL);

@@ -565,7 +570,7 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 
 static void probe_event_disable(struct trace_uprobe *tu, int flag)
 {
-	if (!tu->consumer)
+	if (!is_trace_uprobe_enabled(tu))
 		return;
 
 	uprobe_unregister(tu->inode, tu->offset, &tu->consumer->cons);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2013-01-31 19:22 UTC #2 lore raw

PATCH 2/6 uprobes/tracing: Fully initialize uprobe_trace_consumer before uprobe_register()

In reply to Oleg Nesterov

probe_event_enable() does uprobe_register() and only after that sets utc->tu and tu->consumer/flags. This can race with uprobe_dispatcher() which can miss these assignments or see them out of order. Nothing really bad can happen, but this doesn't look clean/safe.

And this does not allow to use uprobe_consumer->filter() we are going to add, it is called by uprobe_register() and it needs utc->tu.

Change this code to initialize everything before uprobe_register(), and reset tu->consumer/flags if it fails. We can't race with event_disable(), the caller holds event_mutex, and if we could the code would be wrong anyway.

In fact I think uprobe_trace_consumer should die, it buys nothing but complicates the code. We can simply add uprobe_consumer into trace_uprobe.

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

---
 kernel/trace/trace_uprobe.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

kernel/trace/trace_uprobe.c +7 -6

index 6563f00..7b75949 100644
@@ -547,17 +547,18 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 		return -EINTR;
 
 	utc->cons.handler = uprobe_dispatcher;
+	utc->tu = tu;
+	tu->consumer = utc;
+	tu->flags |= flag;
+
 	ret = uprobe_register(tu->inode, tu->offset, &utc->cons);
 	if (ret) {
+		tu->consumer = NULL;
+		tu->flags &= ~flag;
 		kfree(utc);
-		return ret;
 	}
 
-	tu->flags |= flag;
-	utc->tu = tu;
-	tu->consumer = utc;
-
-	return 0;
+	return ret;
 }
 
 static void probe_event_disable(struct trace_uprobe *tu, int flag)

Signature · 1 line

1.5.5.1

Srikar Dronamraju

Acked-by on v1 3/6

2013-02-04 10:51 UTC #4 lore raw

Re: [PATCH 3/6] uprobes/tracing: Ensure inode != NULL in create_trace_uprobe()

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

probe_event_enable/disable() check tu->inode != NULL at the start. This is ugly, if igrab() can fail create_trace_uprobe() should not succeed and "postpone" the failure.
Note: alloc_uprobe() should probably check igrab() != NULL as well.
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

Quoted 38 lines

---
 kernel/trace/trace_uprobe.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

kernel/trace/trace_uprobe.c +4 -2

index 7b75949..f02bbec 100644
@@ -255,6 +255,8 @@ static int create_trace_uprobe(int argc, char **argv)
 
 	inode = igrab(path.dentry->d_inode);
 	path_put(&path);
+	if (!inode)
+		goto fail_address_parse;
 
 	ret = kstrtoul(arg, 0, &offset);
 	if (ret)

@@ -539,7 +541,7 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 	struct uprobe_trace_consumer *utc;
 	int ret = 0;
 
-	if (!tu->inode || tu->consumer)
+	if (tu->consumer)
 		return -EINTR;
 
 	utc = kzalloc(sizeof(struct uprobe_trace_consumer), GFP_KERNEL);

@@ -563,7 +565,7 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 
 static void probe_event_disable(struct trace_uprobe *tu, int flag)
 {
-	if (!tu->inode || !tu->consumer)
+	if (!tu->consumer)
 		return;
 
 	uprobe_unregister(tu->inode, tu->offset, &tu->consumer->cons);

Signature · 1 line

1.5.5.1

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Srikar Dronamraju

2013-02-04 11:19 UTC #13 lore raw

Re: [PATCH 6/6] uprobes/perf: Always increment trace_uprobe->nhit

In reply to Oleg Nesterov

Quoted 6 lines · Oleg Nesterov

Move tu->nhit++ from uprobe_trace_func() to uprobe_dispatcher().
->nhit counts how many time we hit the breakpoint inserted by this uprobe, we do not want to loose this info if uprobe was enabled by sys_perf_event_open().

Though I dont see a problem with this change, It seems unnecessary for me.

Info from nhits is mostly for /sys/kernel/debug/tracing/uprobe_profile

I am not sure how sys_perf_event_open() is making use of this?

Quoted 29 lines

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_uprobe.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

kernel/trace/trace_uprobe.c +1 -2

index 55cdc14..0a9a8de 100644
@@ -473,8 +473,6 @@ static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 	unsigned long irq_flags;
 	struct ftrace_event_call *call = &tu->call;
 
-	tu->nhit++;
-
 	local_save_flags(irq_flags);
 	pc = preempt_count();

@@ -698,6 +696,7 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
 	struct trace_uprobe *tu;
 
 	tu = container_of(con, struct trace_uprobe, consumer);
+	tu->nhit++;
 
 	if (tu->flags & TP_FLAG_TRACE)
 		uprobe_trace_func(tu, regs);

Signature · 1 line

1.5.5.1

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Oleg Nesterov

author

2013-02-04 15:20 UTC #14 lore raw

Re: [PATCH 6/6] uprobes/perf: Always increment trace_uprobe->nhit

Quoted 10 lines · Srikar Dronamraju

Quoted 6 lines · Oleg Nesterov
Move tu->nhit++ from uprobe_trace_func() to uprobe_dispatcher().
->nhit counts how many time we hit the breakpoint inserted by this uprobe, we do not want to loose this info if uprobe was enabled by sys_perf_event_open().

Quoted 4 lines

Though I dont see a problem with this change, It seems unnecessary for me.
Info from nhits is mostly for /sys/kernel/debug/tracing/uprobe_profile

It is only for uprobe_profile, yes, and it is useful. Why should we hide this info if this uprobe is used by perf?

Quoted 1 line

I am not sure how sys_perf_event_open() is making use of this?

I hope I'll send the final series today. From the changelog of the patch which actually turns the filtering on:

Testing:

# perf probe -x /lib/libc.so.6 syscall

# perl -e 'syscall -1 while 1' &
[1] 530

# perf record -e probe_libc:syscall perl -e 'syscall -1 for 1..10; sleep 1'

# perf report --show-total-period
100.00% 10 perl libc-2.8.so [.] syscall

Before this patch:

# cat /sys/kernel/debug/tracing/uprobe_profile
/lib/libc.so.6 syscall 79291

A huge ->nrhit == 79291 reflects the fact that the background process 530 constantly hits this breakpoint too, even if doesn't contribute to the output.

After the patch:

# cat /sys/kernel/debug/tracing/uprobe_profile
/lib/libc.so.6 syscall 10

This shows that only the target process was punished by int3.

Oleg.

Srikar Dronamraju

Acked-by on v1 4/6

2013-02-04 16:14 UTC #6 lore raw

Re: [PATCH 4/6] uprobes/tracing: Introduce is_trace_uprobe_enabled()

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

probe_event_enable/disable() check tu->consumer != NULL to avoid the wrong uprobe_register/unregister().
We are going to kill this pointer and "struct uprobe_trace_consumer", so we add the new helper, is_trace_uprobe_enabled(), which can rely on TP_FLAG_TRACE/TP_FLAG_PROFILE instead.

Quoted 7 lines

Note: the current logic doesn't look optimal, it is not clear why TP_FLAG_TRACE/TP_FLAG_PROFILE are mutually exclusive, we will probably change this later.
Also kill the unused TP_FLAG_UPROBE.
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

Quoted 52 lines

---
 kernel/trace/trace_probe.h  |    1 -
 kernel/trace/trace_uprobe.c |    9 +++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

kernel/trace/trace_probe.h +0 -1

index 9337086..5c7e09d 100644
@@ -66,7 +66,6 @@
 #define TP_FLAG_TRACE		1
 #define TP_FLAG_PROFILE		2
 #define TP_FLAG_REGISTERED	4
-#define TP_FLAG_UPROBE		8
 
 
 /* data_rloc: data relative location, compatible with u32 */

kernel/trace/trace_uprobe.c +7 -2

index f02bbec..947379a 100644
@@ -536,12 +536,17 @@ partial:
 	return TRACE_TYPE_PARTIAL_LINE;
 }
 
+static inline bool is_trace_uprobe_enabled(struct trace_uprobe *tu)
+{
+	return tu->flags & (TP_FLAG_TRACE | TP_FLAG_PROFILE);
+}
+
 static int probe_event_enable(struct trace_uprobe *tu, int flag)
 {
 	struct uprobe_trace_consumer *utc;
 	int ret = 0;
 
-	if (tu->consumer)
+	if (is_trace_uprobe_enabled(tu))
 		return -EINTR;
 
 	utc = kzalloc(sizeof(struct uprobe_trace_consumer), GFP_KERNEL);

@@ -565,7 +570,7 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 
 static void probe_event_disable(struct trace_uprobe *tu, int flag)
 {
-	if (!tu->consumer)
+	if (!is_trace_uprobe_enabled(tu))
 		return;
 
 	uprobe_unregister(tu->inode, tu->offset, &tu->consumer->cons);

Signature · 1 line

1.5.5.1

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Srikar Dronamraju

2013-02-04 16:28 UTC #15 lore raw

Re: [PATCH 6/6] uprobes/perf: Always increment trace_uprobe->nhit

In reply to Oleg Nesterov

Quoted 11 lines · Oleg Nesterov

Quoted 10 lines · Srikar Dronamraju
Quoted 6 lines · Oleg Nesterov
Move tu->nhit++ from uprobe_trace_func() to uprobe_dispatcher().
->nhit counts how many time we hit the breakpoint inserted by this uprobe, we do not want to loose this info if uprobe was enabled by sys_perf_event_open().

Quoted 7 lines

Quoted 4 lines
Though I dont see a problem with this change, It seems unnecessary for me.
Info from nhits is mostly for /sys/kernel/debug/tracing/uprobe_profile
It is only for uprobe_profile, yes, and it is useful. Why should we hide this info if this uprobe is used by perf?

Fine with me.

Steve, Masami, Do you have comments/suggestions on this. (Since kprobe_profile just accounts for kprobetracer and doesnt account for perf record.)
May we should make a similar change in kprobetracer to keep things similar.

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Quoted 41 lines

Quoted 2 lines
I am not sure how sys_perf_event_open() is making use of this?
I hope I'll send the final series today. From the changelog of the patch which actually turns the filtering on:
Testing:
# perf probe -x /lib/libc.so.6 syscall
# perl -e 'syscall -1 while 1' &
[1] 530
# perf record -e probe_libc:syscall perl -e 'syscall -1 for 1..10; sleep 1'
# perf report --show-total-period
100.00% 10 perl libc-2.8.so [.] syscall
Before this patch:
# cat /sys/kernel/debug/tracing/uprobe_profile
/lib/libc.so.6 syscall 79291
A huge ->nrhit == 79291 reflects the fact that the background process 530 constantly hits this breakpoint too, even if doesn't contribute to the output.
After the patch:
# cat /sys/kernel/debug/tracing/uprobe_profile
/lib/libc.so.6 syscall 10
This shows that only the target process was punished by int3.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Steven Rostedt

maintainer

2013-02-04 16:34 UTC #16 lore raw

Re: [PATCH 6/6] uprobes/perf: Always increment trace_uprobe->nhit

Quoted 16 lines · Srikar Dronamraju

Quoted 14 lines · Oleg Nesterov
Quoted 13 lines · Srikar Dronamraju
Quoted 6 lines · Oleg Nesterov
Move tu->nhit++ from uprobe_trace_func() to uprobe_dispatcher().
->nhit counts how many time we hit the breakpoint inserted by this uprobe, we do not want to loose this info if uprobe was enabled by sys_perf_event_open().
Though I dont see a problem with this change, It seems unnecessary for me.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Info from nhits is mostly for /sys/kernel/debug/tracing/uprobe_profile
It is only for uprobe_profile, yes, and it is useful. Why should we hide this info if this uprobe is used by perf?
Fine with me.
Steve, Masami, Do you have comments/suggestions on this.

I have no problem with the change.

Quoted 4 lines

(Since kprobe_profile just accounts for kprobetracer and doesnt account for perf record.)
May we should make a similar change in kprobetracer to keep things similar.

I'm fine either way.

-- Steve

Srikar Dronamraju

2013-02-04 17:01 UTC #8 lore raw

Re: [PATCH 5/6] uprobes/tracing: Kill uprobe_trace_consumer, embed uprobe_consumer into trace_uprobe

In reply to Oleg Nesterov

Quoted 6 lines · Oleg Nesterov

trace_uprobe->consumer and "struct uprobe_trace_consumer" add the unnecessary indirection and complicate the code for no reason.
This patch simply embeds uprobe_consumer into "struct trace_uprobe", all other changes only fix the compilation errors.

I know this patch doesnt change the current behaviour.

We dont handle two concurrent perf record sessions for the same user space probe. Since both sessons share the same trace_uprobe and hence share the same consumer. Initially I had thought of having a chain in uprobe_trace_consumer. However we dont get have enough information at the probe_event_disable() time to detect which consumer to delete Hence I dropped the idea of having a list of consumers attached to the trace_uprobe.

However with allowing prefiltering, we need to have ability to distinguish consumers. The idea of embedding uprobe_consumer within trace_uprobe, may make the problem even more tougher to solve.

Should we document this as a TODO?

Quoted 96 lines

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/trace/trace_uprobe.c |   35 ++++++-----------------------------
 1 files changed, 6 insertions(+), 29 deletions(-)

kernel/trace/trace_uprobe.c +6 ~~-29~~

index 947379a..55cdc14 100644
@@ -31,17 +31,11 @@
 /*
  * uprobe event core functions
  */
-struct trace_uprobe;
-struct uprobe_trace_consumer {
-	struct uprobe_consumer		cons;
-	struct trace_uprobe		*tu;
-};
-
 struct trace_uprobe {
 	struct list_head		list;
 	struct ftrace_event_class	class;
 	struct ftrace_event_call	call;
-	struct uprobe_trace_consumer	*consumer;
+	struct uprobe_consumer		consumer;
 	struct inode			*inode;
 	char				*filename;
 	unsigned long			offset;

@@ -92,6 +86,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs)
 		goto error;
 
 	INIT_LIST_HEAD(&tu->list);
+	tu->consumer.handler = uprobe_dispatcher;
 	return tu;
 
 error:

@@ -543,27 +538,15 @@ static inline bool is_trace_uprobe_enabled(struct trace_uprobe *tu)
 
 static int probe_event_enable(struct trace_uprobe *tu, int flag)
 {
-	struct uprobe_trace_consumer *utc;
 	int ret = 0;
 
 	if (is_trace_uprobe_enabled(tu))
 		return -EINTR;
 
-	utc = kzalloc(sizeof(struct uprobe_trace_consumer), GFP_KERNEL);
-	if (!utc)
-		return -EINTR;
-
-	utc->cons.handler = uprobe_dispatcher;
-	utc->tu = tu;
-	tu->consumer = utc;
 	tu->flags |= flag;
-
-	ret = uprobe_register(tu->inode, tu->offset, &utc->cons);
-	if (ret) {
-		tu->consumer = NULL;
+	ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
+	if (ret)
 		tu->flags &= ~flag;
-		kfree(utc);
-	}
 
 	return ret;
 }

@@ -573,10 +556,8 @@ static void probe_event_disable(struct trace_uprobe *tu, int flag)
 	if (!is_trace_uprobe_enabled(tu))
 		return;
 
-	uprobe_unregister(tu->inode, tu->offset, &tu->consumer->cons);
+	uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
 	tu->flags &= ~flag;
-	kfree(tu->consumer);
-	tu->consumer = NULL;
 }
 
 static int uprobe_event_define_fields(struct ftrace_event_call *event_call)

@@ -714,13 +695,9 @@ int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type,
 
 static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
 {
-	struct uprobe_trace_consumer *utc;
 	struct trace_uprobe *tu;
 
-	utc = container_of(con, struct uprobe_trace_consumer, cons);
-	tu = utc->tu;
-	if (!tu || tu->consumer != utc)
-		return 0;
+	tu = container_of(con, struct trace_uprobe, consumer);
 
 	if (tu->flags & TP_FLAG_TRACE)
 		uprobe_trace_func(tu, regs);

Signature · 1 line

1.5.5.1

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Oleg Nesterov

author

2013-02-04 17:21 UTC #9 lore raw

Re: [PATCH 5/6] uprobes/tracing: Kill uprobe_trace_consumer, embed uprobe_consumer into trace_uprobe

Quoted 10 lines · Srikar Dronamraju

Quoted 5 lines · Oleg Nesterov
trace_uprobe->consumer and "struct uprobe_trace_consumer" add the unnecessary indirection and complicate the code for no reason.
This patch simply embeds uprobe_consumer into "struct trace_uprobe", all other changes only fix the compilation errors.
I know this patch doesnt change the current behaviour.

Yes, and it makes the code simpler.

Quoted 3 lines

We dont handle two concurrent perf record sessions for the same user space probe. Since both sessons share the same trace_uprobe and hence share the same consumer.

We do? I am testing the patches I am going to send, and I specially tried to verify that 2 concurent sessions with different/same filtering constraints work fine.

Or I misunderstood what you meant...

Quoted 5 lines

Initially I had thought of having a chain in uprobe_trace_consumer. However we dont get have enough information at the probe_event_disable() time to detect which consumer to delete Hence I dropped the idea of having a list of consumers attached to the trace_uprobe.

You know, until recently I knew absolutely nothing about kernel/events/ and kernel/trace/. Not that I really understand this code now, I can be easily wrong.

But so far I think that a chain of multiple consumers makes no sense. Each consumer->handler() will use the same call->perf_events list, this will only complicate the code for no reason.

Quoted 2 lines

However with allowing prefiltering, we need to have ability to distinguish consumers.

Certainly not. Please see the patches I am going to send.

Oleg.

Srikar Dronamraju

2013-02-11 09:21 UTC #10 lore raw

Re: [PATCH 5/6] uprobes/tracing: Kill uprobe_trace_consumer, embed uprobe_consumer into trace_uprobe

Quoted 10 lines · Oleg Nesterov

But so far I think that a chain of multiple consumers makes no sense. Each consumer->handler() will use the same call->perf_events list, this will only complicate the code for no reason.
Quoted 2 lines
However with allowing prefiltering, we need to have ability to distinguish consumers.
Certainly not. Please see the patches I am going to send.

Yeah, Looking at the next patchset made me realize this.

thanks

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Srikar Dronamraju

Acked-by on v1 5/6

2013-02-11 09:21 UTC #11 lore raw

Re: [PATCH 5/6] uprobes/tracing: Kill uprobe_trace_consumer, embed uprobe_consumer into trace_uprobe

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

trace_uprobe->consumer and "struct uprobe_trace_consumer" add the unnecessary indirection and complicate the code for no reason.
This patch simply embeds uprobe_consumer into "struct trace_uprobe", all other changes only fix the compilation errors.
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

Quoted 95 lines

---
 kernel/trace/trace_uprobe.c |   35 ++++++-----------------------------
 1 files changed, 6 insertions(+), 29 deletions(-)

kernel/trace/trace_uprobe.c +6 ~~-29~~

index 947379a..55cdc14 100644
@@ -31,17 +31,11 @@
 /*
  * uprobe event core functions
  */
-struct trace_uprobe;
-struct uprobe_trace_consumer {
-	struct uprobe_consumer		cons;
-	struct trace_uprobe		*tu;
-};
-
 struct trace_uprobe {
 	struct list_head		list;
 	struct ftrace_event_class	class;
 	struct ftrace_event_call	call;
-	struct uprobe_trace_consumer	*consumer;
+	struct uprobe_consumer		consumer;
 	struct inode			*inode;
 	char				*filename;
 	unsigned long			offset;

@@ -92,6 +86,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs)
 		goto error;
 
 	INIT_LIST_HEAD(&tu->list);
+	tu->consumer.handler = uprobe_dispatcher;
 	return tu;
 
 error:

@@ -543,27 +538,15 @@ static inline bool is_trace_uprobe_enabled(struct trace_uprobe *tu)
 
 static int probe_event_enable(struct trace_uprobe *tu, int flag)
 {
-	struct uprobe_trace_consumer *utc;
 	int ret = 0;
 
 	if (is_trace_uprobe_enabled(tu))
 		return -EINTR;
 
-	utc = kzalloc(sizeof(struct uprobe_trace_consumer), GFP_KERNEL);
-	if (!utc)
-		return -EINTR;
-
-	utc->cons.handler = uprobe_dispatcher;
-	utc->tu = tu;
-	tu->consumer = utc;
 	tu->flags |= flag;
-
-	ret = uprobe_register(tu->inode, tu->offset, &utc->cons);
-	if (ret) {
-		tu->consumer = NULL;
+	ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
+	if (ret)
 		tu->flags &= ~flag;
-		kfree(utc);
-	}
 
 	return ret;
 }

@@ -573,10 +556,8 @@ static void probe_event_disable(struct trace_uprobe *tu, int flag)
 	if (!is_trace_uprobe_enabled(tu))
 		return;
 
-	uprobe_unregister(tu->inode, tu->offset, &tu->consumer->cons);
+	uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
 	tu->flags &= ~flag;
-	kfree(tu->consumer);
-	tu->consumer = NULL;
 }
 
 static int uprobe_event_define_fields(struct ftrace_event_call *event_call)

@@ -714,13 +695,9 @@ int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type,
 
 static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
 {
-	struct uprobe_trace_consumer *utc;
 	struct trace_uprobe *tu;
 
-	utc = container_of(con, struct uprobe_trace_consumer, cons);
-	tu = utc->tu;
-	if (!tu || tu->consumer != utc)
-		return 0;
+	tu = container_of(con, struct trace_uprobe, consumer);
 
 	if (tu->flags & TP_FLAG_TRACE)
 		uprobe_trace_func(tu, regs);

Signature · 1 line

1.5.5.1

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Srikar Dronamraju

Acked-by on v1 6/6

2013-02-11 09:22 UTC #17 lore raw

Re: [PATCH 6/6] uprobes/perf: Always increment trace_uprobe->nhit

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

Move tu->nhit++ from uprobe_trace_func() to uprobe_dispatcher().
->nhit counts how many time we hit the breakpoint inserted by this uprobe, we do not want to loose this info if uprobe was enabled by sys_perf_event_open().
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Srikar Dronamraju <[email protected]>

Quoted 28 lines

---
 kernel/trace/trace_uprobe.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

kernel/trace/trace_uprobe.c +1 -2

index 55cdc14..0a9a8de 100644
@@ -473,8 +473,6 @@ static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 	unsigned long irq_flags;
 	struct ftrace_event_call *call = &tu->call;
 
-	tu->nhit++;
-
 	local_save_flags(irq_flags);
 	pc = preempt_count();

@@ -698,6 +696,7 @@ static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs)
 	struct trace_uprobe *tu;
 
 	tu = container_of(con, struct trace_uprobe, consumer);
+	tu->nhit++;
 
 	if (tu->flags & TP_FLAG_TRACE)
 		uprobe_trace_func(tu, regs);

Signature · 1 line

1.5.5.1

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju