[PATCH 0/7] uprobes/perf: pre-filtering

[PATCH 0/7] uprobes/perf: pre-filtering

[Oleg Nesterov]

Hello.

Based on git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core,
on top of "[PATCH 0/5] uprobes: kill uprobe_trace_consumer and other cleanups"
series.

With this series 'perf record -e uprobe ...' does not install the breakpoints
into the processes it doesn't want to probe.

Probably this all needs more testing (the last patch for sure) but please
review. I'll try to do more testing tomorrow.

A special note about 1/7. It was actually needed for initial implementation,
this version doesn't need it. Still I am sending it at least for review, it
looks "natural" and potentially useful, and I would like to know if it is
correct or not.

Oleg.

 include/linux/perf_event.h  |    9 ++-
 include/linux/uprobes.h     |    6 ++
 kernel/events/core.c        |   29 +++++----
 kernel/events/uprobes.c     |   39 ++++++++++-
 kernel/trace/trace_uprobe.c |  152 ++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 206 insertions(+), 29 deletions(-)

[PATCH 1/7] perf: Ensure we do not free event->parent before event

[Oleg Nesterov]

__perf_event_exit_task() does:

	sync_child_event(event)
	free_event(event)

sync_child_event() does put_event(event->parent) which can actually
free ->parent. This means that event->destroy(event) is called with
->parent pointing to nowhere.

perf_free_event() does put_parent(parent) before free_event(child)
too.

Afaics, currently this is fine. But the tracing "subclasses" (like
trace_uprobe) may want to track the events and their parents, and even
the fact that parent->destroy() is called before child->destroy() can
complicate this.

Move this put_event() from sync_child_event() to its single caller,
__perf_event_exit_task(). Change perf_free_event() the same way.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/core.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 301079d..1b2e516 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6787,12 +6787,6 @@ static void sync_child_event(struct perf_event *child_event,
 	mutex_lock(&parent_event->child_mutex);
 	list_del_init(&child_event->child_list);
 	mutex_unlock(&parent_event->child_mutex);
-
-	/*
-	 * Release the parent event, if this was the last
-	 * reference to it.
-	 */
-	put_event(parent_event);
 }
 
 static void
@@ -6800,7 +6794,9 @@ __perf_event_exit_task(struct perf_event *child_event,
 			 struct perf_event_context *child_ctx,
 			 struct task_struct *child)
 {
-	if (child_event->parent) {
+	struct perf_event *parent_event = child_event->parent;
+
+	if (parent_event) {
 		raw_spin_lock_irq(&child_ctx->lock);
 		perf_group_detach(child_event);
 		raw_spin_unlock_irq(&child_ctx->lock);
@@ -6813,9 +6809,14 @@ __perf_event_exit_task(struct perf_event *child_event,
 	 * that are still around due to the child reference. These
 	 * events need to be zapped.
 	 */
-	if (child_event->parent) {
+	if (parent_event) {
 		sync_child_event(child_event, child);
 		free_event(child_event);
+		/*
+		 * Release the parent event, if this was the last
+		 * reference to it.
+		 */
+		put_event(parent_event);
 	}
 }
 
@@ -6867,8 +6868,7 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
 	 * We can recurse on the same lock type through:
 	 *
 	 *   __perf_event_exit_task()
-	 *     sync_child_event()
-	 *       put_event()
+	 *       put_event(parent_event)
 	 *         mutex_lock(&ctx->mutex)
 	 *
 	 * But since its the parent context it won't be the same instance.
@@ -6937,11 +6937,11 @@ static void perf_free_event(struct perf_event *event,
 	list_del_init(&event->child_list);
 	mutex_unlock(&parent->child_mutex);
 
-	put_event(parent);
-
 	perf_group_detach(event);
 	list_del_event(event, ctx);
 	free_event(event);
+
+	put_event(parent);
 }
 
 /*
-- 
1.5.5.1

[PATCH 2/7] perf: Introduce hw_perf_event->tp_target and ->tp_list

[Oleg Nesterov]

sys_perf_event_open()->perf_init_event(event) is called before
find_get_context(event), this means that event->ctx == NULL when
class->reg(TRACE_REG_PERF_REGISTER/OPEN) is called and thus it
can't know if this event is per-task or system-wide.

This patch adds hw_perf_event->tp_target for PERF_TYPE_TRACEPOINT,
this is analogous to PERF_TYPE_BREAKPOINT/bp_target we already have.
The patch also moves ->bp_target up so that it can overlap with the
new member, this can help the compiler to generate the better code.

trace_uprobe_register() will use it for prefiltering to avoid the
unnecessary breakpoints in mm's we do not want to trace.

->tp_target doesn't have its own reference, but we can rely on the
fact that either sys_perf_event_open() holds a reference, or it is
equal to event->ctx->task. So this pointer is always valid until
free_event().

Also add the "struct list_head tp_list" into this union. It is not
strictly necessary, but it can simplify the next changes and we can
add it for free.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/perf_event.h |    9 +++++++--
 kernel/events/core.c       |    5 ++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 6bfb2fa..c9775e9 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -135,16 +135,21 @@ struct hw_perf_event {
 		struct { /* software */
 			struct hrtimer	hrtimer;
 		};
+		struct { /* tracepoint */
+			struct task_struct	*tp_target;
+			/* for tp_event->class */
+			struct list_head	tp_list;
+		};
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 		struct { /* breakpoint */
-			struct arch_hw_breakpoint	info;
-			struct list_head		bp_list;
 			/*
 			 * Crufty hack to avoid the chicken and egg
 			 * problem hw_breakpoint has with context
 			 * creation and event initalization.
 			 */
 			struct task_struct		*bp_target;
+			struct arch_hw_breakpoint	info;
+			struct list_head		bp_list;
 		};
 #endif
 	};
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1b2e516..340fb53 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6162,11 +6162,14 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 
 	if (task) {
 		event->attach_state = PERF_ATTACH_TASK;
+
+		if (attr->type == PERF_TYPE_TRACEPOINT)
+			event->hw.tp_target = task;
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 		/*
 		 * hw_breakpoint is a bit difficult here..
 		 */
-		if (attr->type == PERF_TYPE_BREAKPOINT)
+		else if (attr->type == PERF_TYPE_BREAKPOINT)
 			event->hw.bp_target = task;
 #endif
 	}
-- 
1.5.5.1

[PATCH 3/7] uprobes: Introduce uprobe_apply()

[Oleg Nesterov]

Currently it is not possible to change the filtering constraints after
uprobe_register(), so a consumer can not, say, start to trace a task/mm
which was previously filtered out, or remove the no longer needed bp's.

Introduce uprobe_apply() which simply does register_for_each_vma() again
to consult uprobe_consumer->filter() and install/remove the breakpoints.
The only complication is that register_for_each_vma() can no longer
assume that uprobe->consumers should be consulter if is_register == T,
so we change it to accept "struct uprobe_consumer *new" instead.

Unlike uprobe_register(), uprobe_apply(true) doesn't do "unregister" if
register_for_each_vma() fails, it is up to caller to handle the error.

Note: we probably need to cleanup the current interface, it is strange
that uprobe_apply/unregister need inode/offset. We should either change
uprobe_register() to return "struct uprobe *", or add a private ->uprobe
member in uprobe_consumer. And in the long term uprobe_apply() should
take a single argument, uprobe or consumer, even "bool add" should go
away.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/uprobes.h |    6 ++++++
 kernel/events/uprobes.c |   39 +++++++++++++++++++++++++++++++++++----
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 95d0002..02b83db 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -101,6 +101,7 @@ extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsign
 extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
 extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
 extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
+extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
 extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
 extern int uprobe_mmap(struct vm_area_struct *vma);
 extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
@@ -124,6 +125,11 @@ uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
 {
 	return -ENOSYS;
 }
+static inline int
+uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool add)
+{
+	return -ENOSYS;
+}
 static inline void
 uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
 {
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 23340a7..2bcd08e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -733,8 +733,10 @@ build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
 	return curr;
 }
 
-static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
+static int
+register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
 {
+	bool is_register = !!new;
 	struct map_info *info;
 	int err = 0;
 
@@ -765,7 +767,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 
 		if (is_register) {
 			/* consult only the "caller", new consumer. */
-			if (consumer_filter(uprobe->consumers,
+			if (consumer_filter(new,
 					UPROBE_FILTER_REGISTER, mm))
 				err = install_breakpoint(uprobe, mm, vma, info->vaddr);
 		} else if (test_bit(MMF_HAS_UPROBES, &mm->flags)) {
@@ -788,7 +790,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 static int __uprobe_register(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
 	consumer_add(uprobe, uc);
-	return register_for_each_vma(uprobe, true);
+	return register_for_each_vma(uprobe, uc);
 }
 
 static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
@@ -798,7 +800,7 @@ static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *u
 	if (!consumer_del(uprobe, uc))	/* WARN? */
 		return;
 
-	err = register_for_each_vma(uprobe, false);
+	err = register_for_each_vma(uprobe, NULL);
 	/* TODO : cant unregister? schedule a worker thread */
 	if (!uprobe->consumers && !err)
 		delete_uprobe(uprobe);
@@ -855,6 +857,35 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
 EXPORT_SYMBOL_GPL(uprobe_register);
 
 /*
+ * uprobe_apply - unregister a already registered probe.
+ * @inode: the file in which the probe has to be removed.
+ * @offset: offset from the start of the file.
+ * @uc: consumer which wants to add more or remove some breakpoints
+ * @add: add or remove the breakpoints
+ */
+int uprobe_apply(struct inode *inode, loff_t offset,
+			struct uprobe_consumer *uc, bool add)
+{
+	struct uprobe *uprobe;
+	struct uprobe_consumer *con;
+	int ret = -ENOENT;
+
+	uprobe = find_uprobe(inode, offset);
+	if (!uprobe)
+		return ret;
+
+	down_write(&uprobe->register_rwsem);
+	for (con = uprobe->consumers; con && con != uc ; con = con->next)
+		;
+	if (con)
+		ret = register_for_each_vma(uprobe, add ? uc : NULL);
+	up_write(&uprobe->register_rwsem);
+	put_uprobe(uprobe);
+
+	return ret;
+}
+
+/*
  * uprobe_unregister - unregister a already registered probe.
  * @inode: the file in which the probe has to be removed.
  * @offset: offset from the start of the file.
-- 
1.5.5.1

[PATCH 4/7] uprobes/perf: Teach trace_uprobe/perf code to track the active perf_event's

[Oleg Nesterov]

Introduce "struct trace_uprobe_filter" which records the "active"
perf_event's attached to ftrace_event_call. For the start we simply
use list_head, we can optimize this later if needed. For example, we
do not really need to record an event with ->parent != NULL, we can
rely on parent->child_list. And we can certainly do some optimizations
for the case when 2 events have the same ->tp_target or tp_target->mm.

Change trace_uprobe_register() to process TRACE_REG_PERF_OPEN/CLOSE
and add/del this perf_event to the list.

We can probably avoid any locking, but lets start with the "obvioulsy
correct" trace_uprobe_filter->rwlock which protects everything.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace_uprobe.c |   55 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 0a9a8de..f05ec32 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -28,6 +28,12 @@
 
 #define UPROBE_EVENT_SYSTEM	"uprobes"
 
+struct trace_uprobe_filter {
+	rwlock_t		rwlock;
+	int			nr_systemwide;
+	struct list_head	perf_events;
+};
+
 /*
  * uprobe event core functions
  */
@@ -35,6 +41,7 @@ struct trace_uprobe {
 	struct list_head		list;
 	struct ftrace_event_class	class;
 	struct ftrace_event_call	call;
+	struct trace_uprobe_filter	filter;
 	struct uprobe_consumer		consumer;
 	struct inode			*inode;
 	char				*filename;
@@ -58,6 +65,18 @@ static LIST_HEAD(uprobe_list);
 
 static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs);
 
+static inline void init_trace_uprobe_filter(struct trace_uprobe_filter *filter)
+{
+	rwlock_init(&filter->rwlock);
+	filter->nr_systemwide = 0;
+	INIT_LIST_HEAD(&filter->perf_events);
+}
+
+static inline bool uprobe_filter_is_empty(struct trace_uprobe_filter *filter)
+{
+	return !filter->nr_systemwide && list_empty(&filter->perf_events);
+}
+
 /*
  * Allocate new trace_uprobe and initialize it (including uprobes).
  */
@@ -87,6 +106,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs)
 
 	INIT_LIST_HEAD(&tu->list);
 	tu->consumer.handler = uprobe_dispatcher;
+	init_trace_uprobe_filter(&tu->filter);
 	return tu;
 
 error:
@@ -541,6 +561,8 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 	if (is_trace_uprobe_enabled(tu))
 		return -EINTR;
 
+	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
+
 	tu->flags |= flag;
 	ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
 	if (ret)
@@ -554,6 +576,8 @@ static void probe_event_disable(struct trace_uprobe *tu, int flag)
 	if (!is_trace_uprobe_enabled(tu))
 		return;
 
+	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
+
 	uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
 	tu->flags &= ~flag;
 }
@@ -629,6 +653,30 @@ static int set_print_fmt(struct trace_uprobe *tu)
 }
 
 #ifdef CONFIG_PERF_EVENTS
+static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
+{
+	write_lock(&tu->filter.rwlock);
+	if (event->hw.tp_target)
+		list_add(&event->hw.tp_list, &tu->filter.perf_events);
+	else
+		tu->filter.nr_systemwide++;
+	write_unlock(&tu->filter.rwlock);
+
+	return 0;
+}
+
+static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
+{
+	write_lock(&tu->filter.rwlock);
+	if (event->hw.tp_target)
+		list_del(&event->hw.tp_list);
+	else
+		tu->filter.nr_systemwide--;
+	write_unlock(&tu->filter.rwlock);
+
+	return 0;
+}
+
 /* uprobe profile handler */
 static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
 {
@@ -684,6 +732,13 @@ int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type,
 	case TRACE_REG_PERF_UNREGISTER:
 		probe_event_disable(tu, TP_FLAG_PROFILE);
 		return 0;
+
+	case TRACE_REG_PERF_OPEN:
+		return uprobe_perf_open(tu, data);
+
+	case TRACE_REG_PERF_CLOSE:
+		return uprobe_perf_close(tu, data);
+
 #endif
 	default:
 		return 0;
-- 
1.5.5.1

[PATCH 5/7] uprobes/perf: Teach trace_uprobe/perf code to pre-filter

[Oleg Nesterov]

Finally implement uprobe_perf_filter() which checks ->nr_systemwide or
->perf_events to figure out whether we need to insert the breakpoint.

uprobe_perf_open/close are changed to do uprobe_apply(true/false) when
the new perf event comes or goes away.

Note that currently this is very suboptimal:

	- uprobe_register() called by TRACE_REG_PERF_REGISTER becomes a
	  heavy nop, consumer->filter() always returns F at this stage.

	  As it was already discussed we need uprobe_register_only() to
	  avoid the costly register_for_each_vma() when possible.

	- uprobe_apply() is oftenly overkill. Unless "nr_systemwide != 0"
	  changes we need uprobe_apply_mm(), unapply_uprobe() is almost
	  what we need.

	- uprobe_apply() can be simply avoided sometimes, see the next
	  changes.

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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace_uprobe.c |   46 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index f05ec32..5d5a261 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -554,7 +554,12 @@ 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)
+typedef bool (*filter_func_t)(struct uprobe_consumer *self,
+				enum uprobe_filter_ctx ctx,
+				struct mm_struct *mm);
+
+static int
+probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
 {
 	int ret = 0;
 
@@ -564,6 +569,7 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
 	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;
@@ -653,6 +659,22 @@ static int set_print_fmt(struct trace_uprobe *tu)
 }
 
 #ifdef CONFIG_PERF_EVENTS
+static bool
+__uprobe_perf_filter(struct trace_uprobe_filter *filter, struct mm_struct *mm)
+{
+	struct perf_event *event;
+
+	if (filter->nr_systemwide)
+		return true;
+
+	list_for_each_entry(event, &filter->perf_events, hw.tp_list) {
+		if (event->hw.tp_target->mm == mm)
+			return true;
+	}
+
+	return false;
+}
+
 static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
 {
 	write_lock(&tu->filter.rwlock);
@@ -662,6 +684,8 @@ static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
 		tu->filter.nr_systemwide++;
 	write_unlock(&tu->filter.rwlock);
 
+	uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
+
 	return 0;
 }
 
@@ -674,9 +698,25 @@ static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
 		tu->filter.nr_systemwide--;
 	write_unlock(&tu->filter.rwlock);
 
+	uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
+
 	return 0;
 }
 
+static bool uprobe_perf_filter(struct uprobe_consumer *uc,
+				enum uprobe_filter_ctx ctx, struct mm_struct *mm)
+{
+	struct trace_uprobe *tu;
+	int ret;
+
+	tu = container_of(uc, struct trace_uprobe, consumer);
+	read_lock(&tu->filter.rwlock);
+	ret = __uprobe_perf_filter(&tu->filter, mm);
+	read_unlock(&tu->filter.rwlock);
+
+	return ret;
+}
+
 /* uprobe profile handler */
 static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
 {
@@ -719,7 +759,7 @@ int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type,
 
 	switch (type) {
 	case TRACE_REG_REGISTER:
-		return probe_event_enable(tu, TP_FLAG_TRACE);
+		return probe_event_enable(tu, TP_FLAG_TRACE, NULL);
 
 	case TRACE_REG_UNREGISTER:
 		probe_event_disable(tu, TP_FLAG_TRACE);
@@ -727,7 +767,7 @@ int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type,
 
 #ifdef CONFIG_PERF_EVENTS
 	case TRACE_REG_PERF_REGISTER:
-		return probe_event_enable(tu, TP_FLAG_PROFILE);
+		return probe_event_enable(tu, TP_FLAG_PROFILE, uprobe_perf_filter);
 
 	case TRACE_REG_PERF_UNREGISTER:
 		probe_event_disable(tu, TP_FLAG_PROFILE);
-- 
1.5.5.1

[PATCH 6/7] uprobes/perf: Teach trace_uprobe/perf code to use UPROBE_HANDLER_REMOVE

[Oleg Nesterov]

Change uprobe_trace_func() and uprobe_perf_func() to return "int". Change
Change uprobe_dispatcher() to return "trace_ret | perf_ret" although this
is not really needed, currently TP_FLAG_TRACE/TP_FLAG_PROFILE are mutually
exclusive.

The only functional change is that uprobe_perf_func() checks the filtering
too and returns UPROBE_HANDLER_REMOVE if nobody wants to trace current.

Testing:

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

	# perf record -e probe_libc:syscall -i perl -e 'fork; syscall -1 for 1..10; wait'

	# 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				20

A child process doesn't have a counter, but still it hits this breakoint
"copied" by dup_mmap().

After the patch:

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

The child process hits this int3 only once and does unapply_uprobe().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace_uprobe.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 5d5a261..1114619 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -483,7 +483,7 @@ static const struct file_operations uprobe_profile_ops = {
 };
 
 /* uprobe handler */
-static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
+static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 {
 	struct uprobe_trace_entry_head *entry;
 	struct ring_buffer_event *event;
@@ -501,7 +501,7 @@ static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 	event = trace_current_buffer_lock_reserve(&buffer, call->event.type,
 						  size, irq_flags, pc);
 	if (!event)
-		return;
+		return 0;
 
 	entry = ring_buffer_event_data(event);
 	entry->ip = instruction_pointer(task_pt_regs(current));
@@ -511,6 +511,8 @@ static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 
 	if (!filter_current_check_discard(buffer, call, entry, event))
 		trace_buffer_unlock_commit(buffer, event, irq_flags, pc);
+
+	return 0;
 }
 
 /* Event entry printers */
@@ -718,7 +720,7 @@ static bool uprobe_perf_filter(struct uprobe_consumer *uc,
 }
 
 /* uprobe profile handler */
-static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
+static int uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
 {
 	struct ftrace_event_call *call = &tu->call;
 	struct uprobe_trace_entry_head *entry;
@@ -727,11 +729,14 @@ static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
 	int size, __size, i;
 	int rctx;
 
+	if (!uprobe_perf_filter(&tu->consumer, 0, current->mm))
+		return UPROBE_HANDLER_REMOVE;
+
 	__size = sizeof(*entry) + tu->size;
 	size = ALIGN(__size + sizeof(u32), sizeof(u64));
 	size -= sizeof(u32);
 	if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE, "profile buffer not large enough"))
-		return;
+		return 0;
 
 	preempt_disable();
 
@@ -749,6 +754,7 @@ static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
 
  out:
 	preempt_enable();
+	return 0;
 }
 #endif	/* CONFIG_PERF_EVENTS */
 
@@ -789,18 +795,19 @@ 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 trace_uprobe *tu;
+	int ret = 0;
 
 	tu = container_of(con, struct trace_uprobe, consumer);
 	tu->nhit++;
 
 	if (tu->flags & TP_FLAG_TRACE)
-		uprobe_trace_func(tu, regs);
+		ret |= uprobe_trace_func(tu, regs);
 
 #ifdef CONFIG_PERF_EVENTS
 	if (tu->flags & TP_FLAG_PROFILE)
-		uprobe_perf_func(tu, regs);
+		ret |= uprobe_perf_func(tu, regs);
 #endif
-	return 0;
+	return ret;
 }
 
 static struct trace_event_functions uprobe_funcs = {
-- 
1.5.5.1

[PATCH 7/7] uprobes/perf: Avoid uprobe_apply() whenever possible

[Oleg Nesterov]

uprobe_perf_open/close call the costly uprobe_apply() every time,
we can avoid it if:

	- "nr_systemwide != 0" is not changed.

	- There is another process/thread with the same ->mm.

	- copy_proccess() does inherit_event(). dup_mmap() preserves the
	  inserted breakpoints.

	- event->attr.enable_on_exec == T, we can rely on uprobe_mmap()
	  called by exec/mmap paths.

	- tp_target is exiting. Only _close() checks PF_EXITING, I don't
	  think TRACE_REG_PERF_OPEN can hit the dying task too often.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/trace/trace_uprobe.c |   42 ++++++++++++++++++++++++++++++++++++------
 1 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 1114619..e4aab34 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -677,30 +677,60 @@ __uprobe_perf_filter(struct trace_uprobe_filter *filter, struct mm_struct *mm)
 	return false;
 }
 
+static inline bool
+uprobe_filter_event(struct trace_uprobe *tu, struct perf_event *event)
+{
+	return __uprobe_perf_filter(&tu->filter, event->hw.tp_target->mm);
+}
+
 static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
 {
+	bool done;
+
 	write_lock(&tu->filter.rwlock);
-	if (event->hw.tp_target)
+	if (event->hw.tp_target) {
+		/*
+		 * event->parent != NULL means copy_process(), we can avoid
+		 * uprobe_apply(). current->mm must be probed and we can rely
+		 * on dup_mmap() which preserves the already installed bp's.
+		 *
+		 * attr.enable_on_exec means that exec/mmap will install the
+		 * breakpoints we need.
+		 */
+		done = tu->filter.nr_systemwide ||
+			event->parent || event->attr.enable_on_exec ||
+			uprobe_filter_event(tu, event);
 		list_add(&event->hw.tp_list, &tu->filter.perf_events);
-	else
+	} else {
+		done = tu->filter.nr_systemwide;
 		tu->filter.nr_systemwide++;
+	}
 	write_unlock(&tu->filter.rwlock);
 
-	uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
+	if (!done)
+		uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
 
 	return 0;
 }
 
 static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
 {
+	bool done;
+
 	write_lock(&tu->filter.rwlock);
-	if (event->hw.tp_target)
+	if (event->hw.tp_target) {
 		list_del(&event->hw.tp_list);
-	else
+		done = tu->filter.nr_systemwide ||
+			(event->hw.tp_target->flags & PF_EXITING) ||
+			uprobe_filter_event(tu, event);
+	} else {
 		tu->filter.nr_systemwide--;
+		done = tu->filter.nr_systemwide;
+	}
 	write_unlock(&tu->filter.rwlock);
 
-	uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
+	if (!done)
+		uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
 
 	return 0;
 }
-- 
1.5.5.1

Re: [PATCH 0/7] uprobes/perf: pre-filtering

[Oleg Nesterov]

On 02/04, Oleg Nesterov wrote:
>
> Based on git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core,
> on top of "[PATCH 0/5] uprobes: kill uprobe_trace_consumer and other cleanups"
> series.
>
> With this series 'perf record -e uprobe ...' does not install the breakpoints
> into the processes it doesn't want to probe.
>
> Probably this all needs more testing (the last patch for sure) but please
> review. I'll try to do more testing tomorrow.

Well, everything seems to work....

And nobody cares^Wobjects. I am going to add this to "oleg/misc uprobes/core"
and ask Ingo to pull.

> A special note about 1/7. It was actually needed for initial implementation,
> this version doesn't need it. Still I am sending it at least for review, it
> looks "natural" and potentially useful, and I would like to know if it is
> correct or not.

But since 1/7 wasn't reviewed, I'll drop it.




Btw, during the testing I noticed a minor problem (or problems) in tools/perf/,
I'll try to investigate...

Oleg.

[PATCH 0/1] (Was uprobes/perf: pre-filtering)

[Oleg Nesterov]

On 02/06, Oleg Nesterov wrote:
>
> Btw, during the testing I noticed a minor problem (or problems) in tools/perf/,
> I'll try to investigate...

To avoid the confusion, this has nothing to do with perf/uprobe/prefiltering
changes.



Well. I am very new to this code, and I didn't even use perf before,
it is quite possible I missed something/everything.

But I simply can't understand the logic behind perf_evlist__create_maps/etc.

OK, builtin-record.c:record sets .target->uses_mmap == T, this is correct,
we are going to use perf_mmap().

But why do we need it? It is for perf_evlist__create_maps() to ensure we
do not use cpu_map__dummy_new().

But. Even if we use perf_mmap(), "event->cpu == -1 && !event->attr.inherit"
is fine. And indeed, "perf record -e whatever -p1" creates a single counter
with "cpu = -1" and this works. Because, damn, perf_evlist__config_attrs()
notices "evlist->cpus->map[0] < 0" and sets "opts->no_inherit = true".

But at the same time, this does not allow to do, say,
"perf record -e whatever -C0 -p1". -C0 is simply ignored because when
perf_evlist__create_maps() is called perf_target__has_task() == T due to "-p1".

Not to mention, there is no way to trace init and the children it will
fork...

And otoh. "perf record -e whatever -i true" creates a counter for each
cpu due to ->uses_mmap. This is suboptimal, but of course the hack like

	--- a/tools/perf/builtin-record.c
	+++ b/tools/perf/builtin-record.c
	@@ -1081,6 +1078,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
		if (!argc && perf_target__none(&rec->opts.target))
			usage_with_options(record_usage, record_options);
	 
	+	rec->opts.target.uses_mmap = !rec->opts.no_inherit;
	+
		if (rec->force && rec->append_file) {
			ui__error("Can't overwrite and append at the same time."
				  " You need to choose between -f and -A");

should not be even discussed.

And why opts->target.system_wide is only set by OPT_BOOLEAN("all-cpus") ?
I meant, why I can't do "perf record -e whatever -C0" to create a "global"
counter on CPU_0? This doesn't work because __cmd_record() sees !.system_wide
and assumes we need perf_event__synthesize_thread_map() which silently fail.

So I am sending a single patch to fix the problem which complicated my
testing. It is trivial but I am not sure it correct, please review.

Oleg.

[PATCH 1/1] perf/tools: Fix "perf record -C... workload" behaviour

[Oleg Nesterov]

Test-case:

	# perf record -e probe_libc:syscall -C0 true
	true: Terminated

The child process is killed by perf_record__sig_exit(). This is because
perf_evlist__prepare_workload() doesn't initialize threads->map[] and
then perf_event__synthesize_mmap_events() (silently!) fails. This patch
seems to fix the problem, but perhaps we should also move
'pr_err("Couldn't run the workload!\n")' down to 'out_delete_session:',
so that the problems like this would be more understandable.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 tools/perf/util/evlist.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 7052934..8b3f6ef 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -783,7 +783,7 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist,
 		exit(-1);
 	}
 
-	if (perf_target__none(&opts->target))
+	if (!perf_target__has_task(&opts->target))
 		evlist->threads->map[0] = evlist->workload.pid;
 
 	close(child_ready_pipe[1]);
-- 
1.5.5.1

Re: [PATCH 0/1] (Was uprobes/perf: pre-filtering)

[Namhyung Kim]

Hi Oleg,

On Wed, 6 Feb 2013 20:42:18 +0100, Oleg Nesterov wrote:
> OK, builtin-record.c:record sets .target->uses_mmap == T, this is correct,
> we are going to use perf_mmap().
>
> But why do we need it? It is for perf_evlist__create_maps() to ensure we
> do not use cpu_map__dummy_new().
>
> But. Even if we use perf_mmap(), "event->cpu == -1 && !event->attr.inherit"
> is fine. And indeed, "perf record -e whatever -p1" creates a single counter
> with "cpu = -1" and this works. Because, damn, perf_evlist__config_attrs()
> notices "evlist->cpus->map[0] < 0" and sets "opts->no_inherit = true".
>
> But at the same time, this does not allow to do, say,
> "perf record -e whatever -C0 -p1". -C0 is simply ignored because when
> perf_evlist__create_maps() is called perf_target__has_task() == T due to "-p1".
>
> Not to mention, there is no way to trace init and the children it will
> fork...
>
> And otoh. "perf record -e whatever -i true" creates a counter for each
> cpu due to ->uses_mmap. This is suboptimal, but of course the hack like
>
> 	--- a/tools/perf/builtin-record.c
> 	+++ b/tools/perf/builtin-record.c
> 	@@ -1081,6 +1078,8 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
> 		if (!argc && perf_target__none(&rec->opts.target))
> 			usage_with_options(record_usage, record_options);
> 	 
> 	+	rec->opts.target.uses_mmap = !rec->opts.no_inherit;
> 	+
> 		if (rec->force && rec->append_file) {
> 			ui__error("Can't overwrite and append at the same time."
> 				  " You need to choose between -f and -A");
>
> should not be even discussed.
>
> And why opts->target.system_wide is only set by OPT_BOOLEAN("all-cpus") ?
> I meant, why I can't do "perf record -e whatever -C0" to create a "global"
> counter on CPU_0? This doesn't work because __cmd_record() sees !.system_wide
> and assumes we need perf_event__synthesize_thread_map() which silently fail.
>
> So I am sending a single patch to fix the problem which complicated my
> testing. It is trivial but I am not sure it correct, please review.

Yes, it's not clear how it handles above (-C0) case.  I think it should
be treated as a system_wide mode like --all-cpus (-a).  So we could set
->system_wide to true if -C is given and/or test perf_target__has_cpu()
for perf_event__synthesize_thread_map() or both.

Thanks,
Namhyung

Re: [PATCH 0/1] (Was uprobes/perf: pre-filtering)

[Oleg Nesterov]

Hi,

On 02/07, Namhyung Kim wrote:
>
> On Wed, 6 Feb 2013 20:42:18 +0100, Oleg Nesterov wrote:
> >
> > And why opts->target.system_wide is only set by OPT_BOOLEAN("all-cpus") ?
> > I meant, why I can't do "perf record -e whatever -C0" to create a "global"
> > counter on CPU_0? This doesn't work because __cmd_record() sees !.system_wide
> > and assumes we need perf_event__synthesize_thread_map() which silently fail.
> >
> > So I am sending a single patch to fix the problem which complicated my
> > testing. It is trivial but I am not sure it correct, please review.
>
> Yes, it's not clear how it handles above (-C0) case.  I think it should
> be treated as a system_wide mode like --all-cpus (-a).  So we could set
> ->system_wide to true if -C is given and/or test perf_target__has_cpu()
> for perf_event__synthesize_thread_map() or both.

Yes, thanks... but to be honest I do not understand opts->systemwide as
well.

OK, both 'perf record ... sleep 1' and 'perf record ... -a sleep 1' attach
the counter(s) to the child process, but opts->systemwide differs. In the
latter case run_command() does perf_event__synthesize_threads(). Not sure
this is right.

Btw, I am just curious. You can override the target with "-p" and run the
command, but it seems that it is not possible to create a global counter
and run the command. Not that important, but could be handy.

Oleg.

Re: [PATCH 3/7] uprobes: Introduce uprobe_apply()

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-02-04 20:02:50]:

> Currently it is not possible to change the filtering constraints after
> uprobe_register(), so a consumer can not, say, start to trace a task/mm
> which was previously filtered out, or remove the no longer needed bp's.
> 
> Introduce uprobe_apply() which simply does register_for_each_vma() again
> to consult uprobe_consumer->filter() and install/remove the breakpoints.
> The only complication is that register_for_each_vma() can no longer
> assume that uprobe->consumers should be consulter if is_register == T,
> so we change it to accept "struct uprobe_consumer *new" instead.
> 
> Unlike uprobe_register(), uprobe_apply(true) doesn't do "unregister" if
> register_for_each_vma() fails, it is up to caller to handle the error.
> 
> Note: we probably need to cleanup the current interface, it is strange
> that uprobe_apply/unregister need inode/offset. We should either change
> uprobe_register() to return "struct uprobe *", or add a private ->uprobe
> member in uprobe_consumer. And in the long term uprobe_apply() should
> take a single argument, uprobe or consumer, even "bool add" should go
> away.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  include/linux/uprobes.h |    6 ++++++
>  kernel/events/uprobes.c |   39 +++++++++++++++++++++++++++++++++++----
>  2 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 95d0002..02b83db 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -101,6 +101,7 @@ extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsign
>  extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
>  extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
>  extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
> +extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
>  extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
>  extern int uprobe_mmap(struct vm_area_struct *vma);
>  extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned long end);
> @@ -124,6 +125,11 @@ uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
>  {
>  	return -ENOSYS;
>  }
> +static inline int
> +uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool add)
> +{
> +	return -ENOSYS;
> +}
>  static inline void
>  uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *uc)
>  {
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 23340a7..2bcd08e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -733,8 +733,10 @@ build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
>  	return curr;
>  }
> 
> -static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
> +static int
> +register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
>  {
> +	bool is_register = !!new;
>  	struct map_info *info;
>  	int err = 0;
> 
> @@ -765,7 +767,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
> 
>  		if (is_register) {
>  			/* consult only the "caller", new consumer. */
> -			if (consumer_filter(uprobe->consumers,
> +			if (consumer_filter(new,
>  					UPROBE_FILTER_REGISTER, mm))
>  				err = install_breakpoint(uprobe, mm, vma, info->vaddr);
>  		} else if (test_bit(MMF_HAS_UPROBES, &mm->flags)) {
> @@ -788,7 +790,7 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
>  static int __uprobe_register(struct uprobe *uprobe, struct uprobe_consumer *uc)
>  {
>  	consumer_add(uprobe, uc);
> -	return register_for_each_vma(uprobe, true);
> +	return register_for_each_vma(uprobe, uc);
>  }
> 
>  static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> @@ -798,7 +800,7 @@ static void __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *u
>  	if (!consumer_del(uprobe, uc))	/* WARN? */
>  		return;
> 
> -	err = register_for_each_vma(uprobe, false);
> +	err = register_for_each_vma(uprobe, NULL);
>  	/* TODO : cant unregister? schedule a worker thread */
>  	if (!uprobe->consumers && !err)
>  		delete_uprobe(uprobe);
> @@ -855,6 +857,35 @@ int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *
>  EXPORT_SYMBOL_GPL(uprobe_register);
> 
>  /*
> + * uprobe_apply - unregister a already registered probe.
> + * @inode: the file in which the probe has to be removed.
> + * @offset: offset from the start of the file.
> + * @uc: consumer which wants to add more or remove some breakpoints
> + * @add: add or remove the breakpoints
> + */
> +int uprobe_apply(struct inode *inode, loff_t offset,
> +			struct uprobe_consumer *uc, bool add)
> +{
> +	struct uprobe *uprobe;
> +	struct uprobe_consumer *con;
> +	int ret = -ENOENT;
> +
> +	uprobe = find_uprobe(inode, offset);
> +	if (!uprobe)
> +		return ret;
> +
> +	down_write(&uprobe->register_rwsem);
> +	for (con = uprobe->consumers; con && con != uc ; con = con->next)
> +		;
> +	if (con)
> +		ret = register_for_each_vma(uprobe, add ? uc : NULL);
> +	up_write(&uprobe->register_rwsem);
> +	put_uprobe(uprobe);
> +
> +	return ret;
> +}
> +
> +/*
>   * uprobe_unregister - unregister a already registered probe.
>   * @inode: the file in which the probe has to be removed.
>   * @offset: offset from the start of the file.
> -- 
> 1.5.5.1
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH 2/7] perf: Introduce hw_perf_event->tp_target and ->tp_list

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-02-04 20:02:46]:

> sys_perf_event_open()->perf_init_event(event) is called before
> find_get_context(event), this means that event->ctx == NULL when
> class->reg(TRACE_REG_PERF_REGISTER/OPEN) is called and thus it
> can't know if this event is per-task or system-wide.
> 
> This patch adds hw_perf_event->tp_target for PERF_TYPE_TRACEPOINT,
> this is analogous to PERF_TYPE_BREAKPOINT/bp_target we already have.
> The patch also moves ->bp_target up so that it can overlap with the
> new member, this can help the compiler to generate the better code.
> 
> trace_uprobe_register() will use it for prefiltering to avoid the
> unnecessary breakpoints in mm's we do not want to trace.
> 
> ->tp_target doesn't have its own reference, but we can rely on the
> fact that either sys_perf_event_open() holds a reference, or it is
> equal to event->ctx->task. So this pointer is always valid until
> free_event().
> 
> Also add the "struct list_head tp_list" into this union. It is not
> strictly necessary, but it can simplify the next changes and we can
> add it for free.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  include/linux/perf_event.h |    9 +++++++--
>  kernel/events/core.c       |    5 ++++-
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 6bfb2fa..c9775e9 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -135,16 +135,21 @@ struct hw_perf_event {
>  		struct { /* software */
>  			struct hrtimer	hrtimer;
>  		};
> +		struct { /* tracepoint */
> +			struct task_struct	*tp_target;
> +			/* for tp_event->class */
> +			struct list_head	tp_list;
> +		};
>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>  		struct { /* breakpoint */
> -			struct arch_hw_breakpoint	info;
> -			struct list_head		bp_list;
>  			/*
>  			 * Crufty hack to avoid the chicken and egg
>  			 * problem hw_breakpoint has with context
>  			 * creation and event initalization.
>  			 */
>  			struct task_struct		*bp_target;
> +			struct arch_hw_breakpoint	info;
> +			struct list_head		bp_list;
>  		};
>  #endif
>  	};
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 1b2e516..340fb53 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6162,11 +6162,14 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
> 
>  	if (task) {
>  		event->attach_state = PERF_ATTACH_TASK;
> +
> +		if (attr->type == PERF_TYPE_TRACEPOINT)
> +			event->hw.tp_target = task;
>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>  		/*
>  		 * hw_breakpoint is a bit difficult here..
>  		 */
> -		if (attr->type == PERF_TYPE_BREAKPOINT)
> +		else if (attr->type == PERF_TYPE_BREAKPOINT)
>  			event->hw.bp_target = task;
>  #endif
>  	}
> -- 
> 1.5.5.1
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH 4/7] uprobes/perf: Teach trace_uprobe/perf code to track the active perf_event's

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-02-04 20:02:54]:

> Introduce "struct trace_uprobe_filter" which records the "active"
> perf_event's attached to ftrace_event_call. For the start we simply
> use list_head, we can optimize this later if needed. For example, we
> do not really need to record an event with ->parent != NULL, we can
> rely on parent->child_list. And we can certainly do some optimizations
> for the case when 2 events have the same ->tp_target or tp_target->mm.
> 
> Change trace_uprobe_register() to process TRACE_REG_PERF_OPEN/CLOSE
> and add/del this perf_event to the list.
> 
> We can probably avoid any locking, but lets start with the "obvioulsy
> correct" trace_uprobe_filter->rwlock which protects everything.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>


Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/trace/trace_uprobe.c |   55 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 55 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 0a9a8de..f05ec32 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -28,6 +28,12 @@
> 
>  #define UPROBE_EVENT_SYSTEM	"uprobes"
> 
> +struct trace_uprobe_filter {
> +	rwlock_t		rwlock;
> +	int			nr_systemwide;
> +	struct list_head	perf_events;
> +};
> +
>  /*
>   * uprobe event core functions
>   */
> @@ -35,6 +41,7 @@ struct trace_uprobe {
>  	struct list_head		list;
>  	struct ftrace_event_class	class;
>  	struct ftrace_event_call	call;
> +	struct trace_uprobe_filter	filter;
>  	struct uprobe_consumer		consumer;
>  	struct inode			*inode;
>  	char				*filename;
> @@ -58,6 +65,18 @@ static LIST_HEAD(uprobe_list);
> 
>  static int uprobe_dispatcher(struct uprobe_consumer *con, struct pt_regs *regs);
> 
> +static inline void init_trace_uprobe_filter(struct trace_uprobe_filter *filter)
> +{
> +	rwlock_init(&filter->rwlock);
> +	filter->nr_systemwide = 0;
> +	INIT_LIST_HEAD(&filter->perf_events);
> +}
> +
> +static inline bool uprobe_filter_is_empty(struct trace_uprobe_filter *filter)
> +{
> +	return !filter->nr_systemwide && list_empty(&filter->perf_events);
> +}
> +
>  /*
>   * Allocate new trace_uprobe and initialize it (including uprobes).
>   */
> @@ -87,6 +106,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs)
> 
>  	INIT_LIST_HEAD(&tu->list);
>  	tu->consumer.handler = uprobe_dispatcher;
> +	init_trace_uprobe_filter(&tu->filter);
>  	return tu;
> 
>  error:
> @@ -541,6 +561,8 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
>  	if (is_trace_uprobe_enabled(tu))
>  		return -EINTR;
> 
> +	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
> +
>  	tu->flags |= flag;
>  	ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
>  	if (ret)
> @@ -554,6 +576,8 @@ static void probe_event_disable(struct trace_uprobe *tu, int flag)
>  	if (!is_trace_uprobe_enabled(tu))
>  		return;
> 
> +	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
> +
>  	uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
>  	tu->flags &= ~flag;
>  }
> @@ -629,6 +653,30 @@ static int set_print_fmt(struct trace_uprobe *tu)
>  }
> 
>  #ifdef CONFIG_PERF_EVENTS
> +static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
> +{
> +	write_lock(&tu->filter.rwlock);
> +	if (event->hw.tp_target)
> +		list_add(&event->hw.tp_list, &tu->filter.perf_events);
> +	else
> +		tu->filter.nr_systemwide++;
> +	write_unlock(&tu->filter.rwlock);
> +
> +	return 0;
> +}
> +
> +static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
> +{
> +	write_lock(&tu->filter.rwlock);
> +	if (event->hw.tp_target)
> +		list_del(&event->hw.tp_list);
> +	else
> +		tu->filter.nr_systemwide--;
> +	write_unlock(&tu->filter.rwlock);
> +
> +	return 0;
> +}
> +
>  /* uprobe profile handler */
>  static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
>  {
> @@ -684,6 +732,13 @@ int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type,
>  	case TRACE_REG_PERF_UNREGISTER:
>  		probe_event_disable(tu, TP_FLAG_PROFILE);
>  		return 0;
> +
> +	case TRACE_REG_PERF_OPEN:
> +		return uprobe_perf_open(tu, data);
> +
> +	case TRACE_REG_PERF_CLOSE:
> +		return uprobe_perf_close(tu, data);
> +
>  #endif
>  	default:
>  		return 0;
> -- 
> 1.5.5.1
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH 5/7] uprobes/perf: Teach trace_uprobe/perf code to pre-filter

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-02-04 20:02:58]:

> Finally implement uprobe_perf_filter() which checks ->nr_systemwide or
> ->perf_events to figure out whether we need to insert the breakpoint.
> 
> uprobe_perf_open/close are changed to do uprobe_apply(true/false) when
> the new perf event comes or goes away.
> 
> Note that currently this is very suboptimal:
> 
> 	- uprobe_register() called by TRACE_REG_PERF_REGISTER becomes a
> 	  heavy nop, consumer->filter() always returns F at this stage.
> 
> 	  As it was already discussed we need uprobe_register_only() to
> 	  avoid the costly register_for_each_vma() when possible.
> 
> 	- uprobe_apply() is oftenly overkill. Unless "nr_systemwide != 0"
> 	  changes we need uprobe_apply_mm(), unapply_uprobe() is almost
> 	  what we need.
> 
> 	- uprobe_apply() can be simply avoided sometimes, see the next
> 	  changes.
> 
> 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.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/trace/trace_uprobe.c |   46 ++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index f05ec32..5d5a261 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -554,7 +554,12 @@ 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)
> +typedef bool (*filter_func_t)(struct uprobe_consumer *self,
> +				enum uprobe_filter_ctx ctx,
> +				struct mm_struct *mm);
> +
> +static int
> +probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
>  {
>  	int ret = 0;
> 
> @@ -564,6 +569,7 @@ static int probe_event_enable(struct trace_uprobe *tu, int flag)
>  	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;
> @@ -653,6 +659,22 @@ static int set_print_fmt(struct trace_uprobe *tu)
>  }
> 
>  #ifdef CONFIG_PERF_EVENTS
> +static bool
> +__uprobe_perf_filter(struct trace_uprobe_filter *filter, struct mm_struct *mm)
> +{
> +	struct perf_event *event;
> +
> +	if (filter->nr_systemwide)
> +		return true;
> +
> +	list_for_each_entry(event, &filter->perf_events, hw.tp_list) {
> +		if (event->hw.tp_target->mm == mm)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
>  {
>  	write_lock(&tu->filter.rwlock);
> @@ -662,6 +684,8 @@ static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
>  		tu->filter.nr_systemwide++;
>  	write_unlock(&tu->filter.rwlock);
> 
> +	uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
> +
>  	return 0;
>  }
> 
> @@ -674,9 +698,25 @@ static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
>  		tu->filter.nr_systemwide--;
>  	write_unlock(&tu->filter.rwlock);
> 
> +	uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
> +
>  	return 0;
>  }
> 
> +static bool uprobe_perf_filter(struct uprobe_consumer *uc,
> +				enum uprobe_filter_ctx ctx, struct mm_struct *mm)
> +{
> +	struct trace_uprobe *tu;
> +	int ret;
> +
> +	tu = container_of(uc, struct trace_uprobe, consumer);
> +	read_lock(&tu->filter.rwlock);
> +	ret = __uprobe_perf_filter(&tu->filter, mm);
> +	read_unlock(&tu->filter.rwlock);
> +
> +	return ret;
> +}
> +
>  /* uprobe profile handler */
>  static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
>  {
> @@ -719,7 +759,7 @@ int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type,
> 
>  	switch (type) {
>  	case TRACE_REG_REGISTER:
> -		return probe_event_enable(tu, TP_FLAG_TRACE);
> +		return probe_event_enable(tu, TP_FLAG_TRACE, NULL);
> 
>  	case TRACE_REG_UNREGISTER:
>  		probe_event_disable(tu, TP_FLAG_TRACE);
> @@ -727,7 +767,7 @@ int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type,
> 
>  #ifdef CONFIG_PERF_EVENTS
>  	case TRACE_REG_PERF_REGISTER:
> -		return probe_event_enable(tu, TP_FLAG_PROFILE);
> +		return probe_event_enable(tu, TP_FLAG_PROFILE, uprobe_perf_filter);
> 
>  	case TRACE_REG_PERF_UNREGISTER:
>  		probe_event_disable(tu, TP_FLAG_PROFILE);
> -- 
> 1.5.5.1
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH 6/7] uprobes/perf: Teach trace_uprobe/perf code to use UPROBE_HANDLER_REMOVE

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-02-04 20:03:01]:

> Change uprobe_trace_func() and uprobe_perf_func() to return "int". Change
> Change uprobe_dispatcher() to return "trace_ret | perf_ret" although this
> is not really needed, currently TP_FLAG_TRACE/TP_FLAG_PROFILE are mutually
> exclusive.
> 
> The only functional change is that uprobe_perf_func() checks the filtering
> too and returns UPROBE_HANDLER_REMOVE if nobody wants to trace current.
> 
> Testing:
> 
> 	# perf probe -x /lib/libc.so.6 syscall
> 
> 	# perf record -e probe_libc:syscall -i perl -e 'fork; syscall -1 for 1..10; wait'
> 
> 	# 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				20
> 
> A child process doesn't have a counter, but still it hits this breakoint
> "copied" by dup_mmap().
> 
> After the patch:
> 
> 	# cat /sys/kernel/debug/tracing/uprobe_profile
> 		/lib/libc.so.6 syscall				11
> 
> The child process hits this int3 only once and does unapply_uprobe().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/trace/trace_uprobe.c |   21 ++++++++++++++-------
>  1 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 5d5a261..1114619 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -483,7 +483,7 @@ static const struct file_operations uprobe_profile_ops = {
>  };
> 
>  /* uprobe handler */
> -static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
> +static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
>  {
>  	struct uprobe_trace_entry_head *entry;
>  	struct ring_buffer_event *event;
> @@ -501,7 +501,7 @@ static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
>  	event = trace_current_buffer_lock_reserve(&buffer, call->event.type,
>  						  size, irq_flags, pc);
>  	if (!event)
> -		return;
> +		return 0;
> 
>  	entry = ring_buffer_event_data(event);
>  	entry->ip = instruction_pointer(task_pt_regs(current));
> @@ -511,6 +511,8 @@ static void uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
> 
>  	if (!filter_current_check_discard(buffer, call, entry, event))
>  		trace_buffer_unlock_commit(buffer, event, irq_flags, pc);
> +
> +	return 0;
>  }
> 
>  /* Event entry printers */
> @@ -718,7 +720,7 @@ static bool uprobe_perf_filter(struct uprobe_consumer *uc,
>  }
> 
>  /* uprobe profile handler */
> -static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
> +static int uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
>  {
>  	struct ftrace_event_call *call = &tu->call;
>  	struct uprobe_trace_entry_head *entry;
> @@ -727,11 +729,14 @@ static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
>  	int size, __size, i;
>  	int rctx;
> 
> +	if (!uprobe_perf_filter(&tu->consumer, 0, current->mm))
> +		return UPROBE_HANDLER_REMOVE;
> +
>  	__size = sizeof(*entry) + tu->size;
>  	size = ALIGN(__size + sizeof(u32), sizeof(u64));
>  	size -= sizeof(u32);
>  	if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE, "profile buffer not large enough"))
> -		return;
> +		return 0;
> 
>  	preempt_disable();
> 
> @@ -749,6 +754,7 @@ static void uprobe_perf_func(struct trace_uprobe *tu, struct pt_regs *regs)
> 
>   out:
>  	preempt_enable();
> +	return 0;
>  }
>  #endif	/* CONFIG_PERF_EVENTS */
> 
> @@ -789,18 +795,19 @@ 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 trace_uprobe *tu;
> +	int ret = 0;
> 
>  	tu = container_of(con, struct trace_uprobe, consumer);
>  	tu->nhit++;
> 
>  	if (tu->flags & TP_FLAG_TRACE)
> -		uprobe_trace_func(tu, regs);
> +		ret |= uprobe_trace_func(tu, regs);
> 
>  #ifdef CONFIG_PERF_EVENTS
>  	if (tu->flags & TP_FLAG_PROFILE)
> -		uprobe_perf_func(tu, regs);
> +		ret |= uprobe_perf_func(tu, regs);
>  #endif
> -	return 0;
> +	return ret;
>  }
> 
>  static struct trace_event_functions uprobe_funcs = {
> -- 
> 1.5.5.1
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH 7/7] uprobes/perf: Avoid uprobe_apply() whenever possible

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-02-04 20:03:05]:

> uprobe_perf_open/close call the costly uprobe_apply() every time,
> we can avoid it if:
> 
> 	- "nr_systemwide != 0" is not changed.
> 
> 	- There is another process/thread with the same ->mm.
> 
> 	- copy_proccess() does inherit_event(). dup_mmap() preserves the
> 	  inserted breakpoints.
> 
> 	- event->attr.enable_on_exec == T, we can rely on uprobe_mmap()
> 	  called by exec/mmap paths.
> 
> 	- tp_target is exiting. Only _close() checks PF_EXITING, I don't
> 	  think TRACE_REG_PERF_OPEN can hit the dying task too often.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  kernel/trace/trace_uprobe.c |   42 ++++++++++++++++++++++++++++++++++++------
>  1 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 1114619..e4aab34 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -677,30 +677,60 @@ __uprobe_perf_filter(struct trace_uprobe_filter *filter, struct mm_struct *mm)
>  	return false;
>  }
> 
> +static inline bool
> +uprobe_filter_event(struct trace_uprobe *tu, struct perf_event *event)
> +{
> +	return __uprobe_perf_filter(&tu->filter, event->hw.tp_target->mm);
> +}
> +
>  static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event *event)
>  {
> +	bool done;
> +
>  	write_lock(&tu->filter.rwlock);
> -	if (event->hw.tp_target)
> +	if (event->hw.tp_target) {
> +		/*
> +		 * event->parent != NULL means copy_process(), we can avoid
> +		 * uprobe_apply(). current->mm must be probed and we can rely
> +		 * on dup_mmap() which preserves the already installed bp's.
> +		 *
> +		 * attr.enable_on_exec means that exec/mmap will install the
> +		 * breakpoints we need.
> +		 */
> +		done = tu->filter.nr_systemwide ||
> +			event->parent || event->attr.enable_on_exec ||
> +			uprobe_filter_event(tu, event);
>  		list_add(&event->hw.tp_list, &tu->filter.perf_events);
> -	else
> +	} else {
> +		done = tu->filter.nr_systemwide;
>  		tu->filter.nr_systemwide++;
> +	}
>  	write_unlock(&tu->filter.rwlock);
> 
> -	uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
> +	if (!done)
> +		uprobe_apply(tu->inode, tu->offset, &tu->consumer, true);
> 
>  	return 0;
>  }
> 
>  static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event *event)
>  {
> +	bool done;
> +
>  	write_lock(&tu->filter.rwlock);
> -	if (event->hw.tp_target)
> +	if (event->hw.tp_target) {
>  		list_del(&event->hw.tp_list);
> -	else
> +		done = tu->filter.nr_systemwide ||
> +			(event->hw.tp_target->flags & PF_EXITING) ||
> +			uprobe_filter_event(tu, event);
> +	} else {
>  		tu->filter.nr_systemwide--;
> +		done = tu->filter.nr_systemwide;
> +	}
>  	write_unlock(&tu->filter.rwlock);
> 
> -	uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
> +	if (!done)
> +		uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
> 
>  	return 0;
>  }
> -- 
> 1.5.5.1
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH 1/1] perf/tools: Fix "perf record -C... workload" behaviour

[Jiri Olsa]

On Wed, Feb 06, 2013 at 08:42:58PM +0100, Oleg Nesterov wrote:
> Test-case:
> 
> 	# perf record -e probe_libc:syscall -C0 true
> 	true: Terminated
> 
> The child process is killed by perf_record__sig_exit(). This is because
> perf_evlist__prepare_workload() doesn't initialize threads->map[] and
> then perf_event__synthesize_mmap_events() (silently!) fails. This patch

hi,
sorry for late reply..

I think the reason it fails is that it actually got inside
perf_event__synthesize_mmap_events while there's cpu list
specified.

I sent possible fix:
http://marc.info/?l=linux-kernel&m=136178604718144&w=2


> seems to fix the problem, but perhaps we should also move
> 'pr_err("Couldn't run the workload!\n")' down to 'out_delete_session:',

I'll check ;-)

thanks,
jirka

Re: [PATCH 1/7] perf: Ensure we do not free event->parent before event

[Jiri Olsa]

On Mon, Feb 04, 2013 at 08:02:43PM +0100, Oleg Nesterov wrote:
> __perf_event_exit_task() does:
> 
> 	sync_child_event(event)
> 	free_event(event)
> 
> sync_child_event() does put_event(event->parent) which can actually
> free ->parent. This means that event->destroy(event) is called with
> ->parent pointing to nowhere.
> 
> perf_free_event() does put_parent(parent) before free_event(child)
> too.
> 
> Afaics, currently this is fine. But the tracing "subclasses" (like
> trace_uprobe) may want to track the events and their parents, and even
> the fact that parent->destroy() is called before child->destroy() can
> complicate this.
> 
> Move this put_event() from sync_child_event() to its single caller,
> __perf_event_exit_task(). Change perf_free_event() the same way.

looks ok to me, could prevent future headaches ;-)

jirka



> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/core.c |   24 ++++++++++++------------
>  1 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 301079d..1b2e516 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6787,12 +6787,6 @@ static void sync_child_event(struct perf_event *child_event,
>  	mutex_lock(&parent_event->child_mutex);
>  	list_del_init(&child_event->child_list);
>  	mutex_unlock(&parent_event->child_mutex);
> -
> -	/*
> -	 * Release the parent event, if this was the last
> -	 * reference to it.
> -	 */
> -	put_event(parent_event);
>  }
>  
>  static void
> @@ -6800,7 +6794,9 @@ __perf_event_exit_task(struct perf_event *child_event,
>  			 struct perf_event_context *child_ctx,
>  			 struct task_struct *child)
>  {
> -	if (child_event->parent) {
> +	struct perf_event *parent_event = child_event->parent;
> +
> +	if (parent_event) {
>  		raw_spin_lock_irq(&child_ctx->lock);
>  		perf_group_detach(child_event);
>  		raw_spin_unlock_irq(&child_ctx->lock);
> @@ -6813,9 +6809,14 @@ __perf_event_exit_task(struct perf_event *child_event,
>  	 * that are still around due to the child reference. These
>  	 * events need to be zapped.
>  	 */
> -	if (child_event->parent) {
> +	if (parent_event) {
>  		sync_child_event(child_event, child);
>  		free_event(child_event);
> +		/*
> +		 * Release the parent event, if this was the last
> +		 * reference to it.
> +		 */
> +		put_event(parent_event);
>  	}
>  }
>  
> @@ -6867,8 +6868,7 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
>  	 * We can recurse on the same lock type through:
>  	 *
>  	 *   __perf_event_exit_task()
> -	 *     sync_child_event()
> -	 *       put_event()
> +	 *       put_event(parent_event)
>  	 *         mutex_lock(&ctx->mutex)
>  	 *
>  	 * But since its the parent context it won't be the same instance.
> @@ -6937,11 +6937,11 @@ static void perf_free_event(struct perf_event *event,
>  	list_del_init(&event->child_list);
>  	mutex_unlock(&parent->child_mutex);
>  
> -	put_event(parent);
> -
>  	perf_group_detach(event);
>  	list_del_event(event, ctx);
>  	free_event(event);
> +
> +	put_event(parent);
>  }
>  
>  /*
> -- 
> 1.5.5.1
>