tracing/uprobes: Support ftrace_event_file base multibuffer

Support multi-buffer on uprobe-based dynamic events by using ftrace_event_file.

The code change is based on kprobe-based dynamic events multibuffer support work commited by Masami(commit 41a7dd420c)

Signed-off-by zhangwei(Jovi) <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Srikar Dronamraju <[email protected]>

---
 kernel/trace/trace_uprobe.c |  183 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 161 insertions(+), 22 deletions(-)

kernel/trace/trace_uprobe.c +161 -22

index d5d0cd3..31f08d6 100644
@@ -53,6 +53,7 @@ struct trace_uprobe {
 	struct list_head		list;
 	struct ftrace_event_class	class;
 	struct ftrace_event_call	call;
+	struct ftrace_event_file * __rcu *files;
 	struct trace_uprobe_filter	filter;
 	struct uprobe_consumer		consumer;
 	struct inode			*inode;

@@ -513,7 +514,8 @@ static const struct file_operations uprobe_profile_ops = {
 };
 
 static void uprobe_trace_print(struct trace_uprobe *tu,
-				unsigned long func, struct pt_regs *regs)
+				unsigned long func, struct pt_regs *regs,
+				struct ftrace_event_file *ftrace_file)
 {
 	struct uprobe_trace_entry_head *entry;
 	struct ring_buffer_event *event;

@@ -522,9 +524,15 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
 	int size, i;
 	struct ftrace_event_call *call = &tu->call;
 
+	WARN_ON(call != ftrace_file->event_call);
+
+	if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags))
+		return;
+
 	size = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));
-	event = trace_current_buffer_lock_reserve(&buffer, call->event.type,
-						  size + tu->size, 0, 0);
+	event = trace_event_buffer_lock_reserve(&buffer, ftrace_file,
+						call->event.type,
+						size + tu->size, 0, 0);
 	if (!event)
 		return;
 

@@ -548,15 +556,35 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
 /* uprobe handler */
 static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 {
-	if (!is_ret_probe(tu))
-		uprobe_trace_print(tu, 0, regs);
+	struct ftrace_event_file **file;
+
+	if (is_ret_probe(tu))
+		return 0;
+
+	file = rcu_dereference_raw(tu->files);
+	if (unlikely(!file))
+		return 0;
+
+	while (*file) {
+		uprobe_trace_print(tu, 0, regs, *file);
+		file++;
+	}
+
 	return 0;
 }
 
 static void uretprobe_trace_func(struct trace_uprobe *tu, unsigned long func,
 				struct pt_regs *regs)
 {
-	uprobe_trace_print(tu, func, regs);
+	struct ftrace_event_file **file = rcu_dereference_raw(tu->files);
+
+	if (unlikely(!file))
+		return;
+
+	while (*file) {
+		uprobe_trace_print(tu, func, regs, *file);
+		file++;
+	}
 }
 
 /* Event entry printers */

@@ -597,6 +625,26 @@ partial:
 	return TRACE_TYPE_PARTIAL_LINE;
 }
 
+
+static int trace_uprobe_nr_files(struct trace_uprobe *tu)
+{
+	struct ftrace_event_file **file;
+	int ret = 0;
+
+	/*
+	 * Since all tu->files updater is protected by uprobe_enable_lock,
+	 * we don't need to lock an rcu_read_lock.
+	 */
+	file = rcu_dereference_raw(tu->files);
+	if (file)
+		while (*(file++))
+			ret++;
+
+	return ret;
+}
+
+static DEFINE_MUTEX(uprobe_enable_lock);
+
 static inline bool is_trace_uprobe_enabled(struct trace_uprobe *tu)
 {
 	return tu->flags & (TP_FLAG_TRACE | TP_FLAG_PROFILE);

@@ -607,33 +655,123 @@ typedef bool (*filter_func_t)(struct uprobe_consumer *self,
 				struct mm_struct *mm);
 
 static int
-probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
+probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
+		   filter_func_t filter)
 {
+	int enabled = 0;
 	int ret = 0;
 
+	mutex_lock(&uprobe_enable_lock);
+
 	if (is_trace_uprobe_enabled(tu))
-		return -EINTR;
+		enabled = 1;
+
+	if (file) {
+		struct ftrace_event_file **new, **old;
+		int n = trace_uprobe_nr_files(tu);
+
+		old = rcu_dereference_raw(tu->files);
+		/* 1 is for new one and 1 is for stopper */
+		new = kzalloc((n + 2) * sizeof(struct ftrace_event_file *),
+			      GFP_KERNEL);
+		if (!new) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+		memcpy(new, old, n * sizeof(struct ftrace_event_file *));
+		new[n] = file;
+		/* The last one keeps a NULL */
+
+		rcu_assign_pointer(tu->files, new);
+		tu->flags |= TP_FLAG_TRACE;
+
+		if (old) {
+			/* Make sure the probe is done with old files */
+			synchronize_sched();
+			kfree(old);
+		}
+	} else
+		tu->flags |= TP_FLAG_PROFILE;
 
 	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
 
-	tu->flags |= flag;
-	tu->consumer.filter = filter;
-	ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
-	if (ret)
-		tu->flags &= ~flag;
+	if (!enabled) {
+		tu->consumer.filter = filter;
+		ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
+		if (ret)
+			tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
+	}
 
+ out_unlock:
+	mutex_unlock(&uprobe_enable_lock);
 	return ret;
 }
 
-static void probe_event_disable(struct trace_uprobe *tu, int flag)
+static int
+trace_uprobe_file_index(struct trace_uprobe *tu, struct ftrace_event_file *file)
 {
-	if (!is_trace_uprobe_enabled(tu))
-		return;
+	struct ftrace_event_file **files;
+	int i;
+
+	/*
+	 * Since all tu->files updater is protected by uprobe_enable_lock,
+	 * we don't need to lock an rcu_read_lock.
+	 */
+	files = rcu_dereference_raw(tu->files);
+	if (files) {
+		for (i = 0; files[i]; i++)
+			if (files[i] == file)
+				return i;
+	}
+
+	return -1;
+}
+
+static void
+probe_event_disable(struct trace_uprobe *tu, struct ftrace_event_file *file)
+{
+	mutex_lock(&uprobe_enable_lock);
+
+	if (file) {
+		struct ftrace_event_file **new, **old;
+		int n = trace_uprobe_nr_files(tu);
+		int i, j;
+
+		old = rcu_dereference_raw(tu->files);
+		if (n == 0 || trace_uprobe_file_index(tu, file) < 0)
+			goto out_unlock;
+
+		if (n == 1) {	/* Remove the last file */
+			tu->flags &= ~TP_FLAG_TRACE;
+			new = NULL;
+		} else {
+			new = kzalloc(n * sizeof(struct ftrace_event_file *),
+				      GFP_KERNEL);
+			if (!new)
+				goto out_unlock;
+
+			/* This copy & check loop copies the NULL stopper too */
+			for (i = 0, j = 0; j < n && i < n + 1; i++)
+				if (old[i] != file)
+					new[j++] = old[i];
+		}
+
+		rcu_assign_pointer(tu->files, new);
+
+		/* Make sure the probe is done with old files */
+		synchronize_sched();
+		kfree(old);
+	} else
+		tu->flags &= ~TP_FLAG_PROFILE;
+
 
 	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
 
-	uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
-	tu->flags &= ~flag;
+	if (!is_trace_uprobe_enabled(tu))
+		uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
+
+ out_unlock:
+	mutex_unlock(&uprobe_enable_lock);
 }
 
 static int uprobe_event_define_fields(struct ftrace_event_call *event_call)

@@ -869,21 +1007,22 @@ static
 int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type, void *data)
 {
 	struct trace_uprobe *tu = event->data;
+	struct ftrace_event_file *file = data;
 
 	switch (type) {
 	case TRACE_REG_REGISTER:
-		return probe_event_enable(tu, TP_FLAG_TRACE, NULL);
+		return probe_event_enable(tu, file, NULL);
 
 	case TRACE_REG_UNREGISTER:
-		probe_event_disable(tu, TP_FLAG_TRACE);
+		probe_event_disable(tu, file);
 		return 0;
 
 #ifdef CONFIG_PERF_EVENTS
 	case TRACE_REG_PERF_REGISTER:
-		return probe_event_enable(tu, TP_FLAG_PROFILE, uprobe_perf_filter);
+		return probe_event_enable(tu, NULL, uprobe_perf_filter);
 
 	case TRACE_REG_PERF_UNREGISTER:
-		probe_event_disable(tu, TP_FLAG_PROFILE);
+		probe_event_disable(tu, NULL);
 		return 0;
 
 	case TRACE_REG_PERF_OPEN:

Srikar, Oleg, could you check that?

Quoted 2 lines · Masami Hiramatsu

Srikar, Oleg, could you check that?

I'll definitely try to read this series on Monday.

Quoted 5 lines · zhangwei(Jovi)

Support multi-buffer on uprobe-based dynamic events by using ftrace_event_file.

The code change is based on kprobe-based dynamic events multibuffer support work commited by Masami(commit 41a7dd420c)

I'm not so sure that rcu_dereference_raw() can ensure the safety of accessing pointer in uprobe handler. ( since kprobes disables irq and preempt, it could use rcu.)

Srikar, Oleg, could you check that?

Thank you,

Quoted 50 lines

Signed-off-by zhangwei(Jovi) <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Srikar Dronamraju <[email protected]>

---
 kernel/trace/trace_uprobe.c |  183 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 161 insertions(+), 22 deletions(-)

kernel/trace/trace_uprobe.c +11 -3

index d5d0cd3..31f08d6 100644
@@ -53,6 +53,7 @@ struct trace_uprobe {
 	struct list_head		list;
 	struct ftrace_event_class	class;
 	struct ftrace_event_call	call;
+	struct ftrace_event_file * __rcu *files;
 	struct trace_uprobe_filter	filter;
 	struct uprobe_consumer		consumer;
 	struct inode			*inode;

@@ -513,7 +514,8 @@ static const struct file_operations uprobe_profile_ops = {
 };
 
 static void uprobe_trace_print(struct trace_uprobe *tu,
-				unsigned long func, struct pt_regs *regs)
+				unsigned long func, struct pt_regs *regs,
+				struct ftrace_event_file *ftrace_file)
 {
 	struct uprobe_trace_entry_head *entry;
 	struct ring_buffer_event *event;

@@ -522,9 +524,15 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
 	int size, i;
 	struct ftrace_event_call *call = &tu->call;
 
+	WARN_ON(call != ftrace_file->event_call);
+
+	if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags))
+		return;
+
 	size = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));
-	event = trace_current_buffer_lock_reserve(&buffer, call->event.type,
-						  size + tu->size, 0, 0);
+	event = trace_event_buffer_lock_reserve(&buffer, ftrace_file,
+						call->event.type,
+						size + tu->size, 0, 0);
 	if (!event)
 		return;
 

kernel/trace/trace_uprobe.c static void uprobe_trace_print(struct trace_uprobe *tu, · line 564

Quoted 12 lines

kernel/trace/trace_uprobe.c +6 -2

@@ -548,15 +556,35 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
 /* uprobe handler */
 static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 {
-	if (!is_ret_probe(tu))
-		uprobe_trace_print(tu, 0, regs);
+	struct ftrace_event_file **file;
+
+	if (is_ret_probe(tu))
+		return 0;
+
+	file = rcu_dereference_raw(tu->files);

Quoted 218 lines

kernel/trace/trace_uprobe.c +144 -17

+	if (unlikely(!file))
+		return 0;
+
+	while (*file) {
+		uprobe_trace_print(tu, 0, regs, *file);
+		file++;
+	}
+
 	return 0;
 }
 
 static void uretprobe_trace_func(struct trace_uprobe *tu, unsigned long func,
 				struct pt_regs *regs)
 {
-	uprobe_trace_print(tu, func, regs);
+	struct ftrace_event_file **file = rcu_dereference_raw(tu->files);
+
+	if (unlikely(!file))
+		return;
+
+	while (*file) {
+		uprobe_trace_print(tu, func, regs, *file);
+		file++;
+	}
 }
 
 /* Event entry printers */

@@ -597,6 +625,26 @@ partial:
 	return TRACE_TYPE_PARTIAL_LINE;
 }
 
+
+static int trace_uprobe_nr_files(struct trace_uprobe *tu)
+{
+	struct ftrace_event_file **file;
+	int ret = 0;
+
+	/*
+	 * Since all tu->files updater is protected by uprobe_enable_lock,
+	 * we don't need to lock an rcu_read_lock.
+	 */
+	file = rcu_dereference_raw(tu->files);
+	if (file)
+		while (*(file++))
+			ret++;
+
+	return ret;
+}
+
+static DEFINE_MUTEX(uprobe_enable_lock);
+
 static inline bool is_trace_uprobe_enabled(struct trace_uprobe *tu)
 {
 	return tu->flags & (TP_FLAG_TRACE | TP_FLAG_PROFILE);

@@ -607,33 +655,123 @@ typedef bool (*filter_func_t)(struct uprobe_consumer *self,
 				struct mm_struct *mm);
 
 static int
-probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
+probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
+		   filter_func_t filter)
 {
+	int enabled = 0;
 	int ret = 0;
 
+	mutex_lock(&uprobe_enable_lock);
+
 	if (is_trace_uprobe_enabled(tu))
-		return -EINTR;
+		enabled = 1;
+
+	if (file) {
+		struct ftrace_event_file **new, **old;
+		int n = trace_uprobe_nr_files(tu);
+
+		old = rcu_dereference_raw(tu->files);
+		/* 1 is for new one and 1 is for stopper */
+		new = kzalloc((n + 2) * sizeof(struct ftrace_event_file *),
+			      GFP_KERNEL);
+		if (!new) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+		memcpy(new, old, n * sizeof(struct ftrace_event_file *));
+		new[n] = file;
+		/* The last one keeps a NULL */
+
+		rcu_assign_pointer(tu->files, new);
+		tu->flags |= TP_FLAG_TRACE;
+
+		if (old) {
+			/* Make sure the probe is done with old files */
+			synchronize_sched();
+			kfree(old);
+		}
+	} else
+		tu->flags |= TP_FLAG_PROFILE;
 
 	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
 
-	tu->flags |= flag;
-	tu->consumer.filter = filter;
-	ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
-	if (ret)
-		tu->flags &= ~flag;
+	if (!enabled) {
+		tu->consumer.filter = filter;
+		ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
+		if (ret)
+			tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
+	}
 
+ out_unlock:
+	mutex_unlock(&uprobe_enable_lock);
 	return ret;
 }
 
-static void probe_event_disable(struct trace_uprobe *tu, int flag)
+static int
+trace_uprobe_file_index(struct trace_uprobe *tu, struct ftrace_event_file *file)
 {
-	if (!is_trace_uprobe_enabled(tu))
-		return;
+	struct ftrace_event_file **files;
+	int i;
+
+	/*
+	 * Since all tu->files updater is protected by uprobe_enable_lock,
+	 * we don't need to lock an rcu_read_lock.
+	 */
+	files = rcu_dereference_raw(tu->files);
+	if (files) {
+		for (i = 0; files[i]; i++)
+			if (files[i] == file)
+				return i;
+	}
+
+	return -1;
+}
+
+static void
+probe_event_disable(struct trace_uprobe *tu, struct ftrace_event_file *file)
+{
+	mutex_lock(&uprobe_enable_lock);
+
+	if (file) {
+		struct ftrace_event_file **new, **old;
+		int n = trace_uprobe_nr_files(tu);
+		int i, j;
+
+		old = rcu_dereference_raw(tu->files);
+		if (n == 0 || trace_uprobe_file_index(tu, file) < 0)
+			goto out_unlock;
+
+		if (n == 1) {	/* Remove the last file */
+			tu->flags &= ~TP_FLAG_TRACE;
+			new = NULL;
+		} else {
+			new = kzalloc(n * sizeof(struct ftrace_event_file *),
+				      GFP_KERNEL);
+			if (!new)
+				goto out_unlock;
+
+			/* This copy & check loop copies the NULL stopper too */
+			for (i = 0, j = 0; j < n && i < n + 1; i++)
+				if (old[i] != file)
+					new[j++] = old[i];
+		}
+
+		rcu_assign_pointer(tu->files, new);
+
+		/* Make sure the probe is done with old files */
+		synchronize_sched();
+		kfree(old);
+	} else
+		tu->flags &= ~TP_FLAG_PROFILE;
+
 
 	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
 
-	uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
-	tu->flags &= ~flag;
+	if (!is_trace_uprobe_enabled(tu))
+		uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
+
+ out_unlock:
+	mutex_unlock(&uprobe_enable_lock);
 }
 
 static int uprobe_event_define_fields(struct ftrace_event_call *event_call)

@@ -869,21 +1007,22 @@ static
 int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type, void *data)
 {
 	struct trace_uprobe *tu = event->data;
+	struct ftrace_event_file *file = data;
 
 	switch (type) {
 	case TRACE_REG_REGISTER:
-		return probe_event_enable(tu, TP_FLAG_TRACE, NULL);
+		return probe_event_enable(tu, file, NULL);
 
 	case TRACE_REG_UNREGISTER:
-		probe_event_disable(tu, TP_FLAG_TRACE);
+		probe_event_disable(tu, file);
 		return 0;
 
 #ifdef CONFIG_PERF_EVENTS
 	case TRACE_REG_PERF_REGISTER:
-		return probe_event_enable(tu, TP_FLAG_PROFILE, uprobe_perf_filter);
+		return probe_event_enable(tu, NULL, uprobe_perf_filter);
 
 	case TRACE_REG_PERF_UNREGISTER:
-		probe_event_disable(tu, TP_FLAG_PROFILE);
+		probe_event_disable(tu, NULL);
 		return 0;
 
 	case TRACE_REG_PERF_OPEN:

Quoted 10 lines · Masami Hiramatsu

Quoted 5 lines · zhangwei(Jovi)

Support multi-buffer on uprobe-based dynamic events by using ftrace_event_file.

The code change is based on kprobe-based dynamic events multibuffer support work commited by Masami(commit 41a7dd420c)

I'm not so sure that rcu_dereference_raw() can ensure the safety of accessing pointer in uprobe handler. ( since kprobes disables irq and preempt, it could use rcu.)

Correct.

Quoted 55 lines

Srikar, Oleg, could you check that?

Thank you,

Quoted 50 lines

Signed-off-by zhangwei(Jovi) <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Srikar Dronamraju <[email protected]>

---
 kernel/trace/trace_uprobe.c |  183 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 161 insertions(+), 22 deletions(-)

kernel/trace/trace_uprobe.c +11 -3

index d5d0cd3..31f08d6 100644
@@ -53,6 +53,7 @@ struct trace_uprobe {
 	struct list_head		list;
 	struct ftrace_event_class	class;
 	struct ftrace_event_call	call;
+	struct ftrace_event_file * __rcu *files;
 	struct trace_uprobe_filter	filter;
 	struct uprobe_consumer		consumer;
 	struct inode			*inode;

@@ -513,7 +514,8 @@ static const struct file_operations uprobe_profile_ops = {
 };
 
 static void uprobe_trace_print(struct trace_uprobe *tu,
-				unsigned long func, struct pt_regs *regs)
+				unsigned long func, struct pt_regs *regs,
+				struct ftrace_event_file *ftrace_file)
 {
 	struct uprobe_trace_entry_head *entry;
 	struct ring_buffer_event *event;

@@ -522,9 +524,15 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
 	int size, i;
 	struct ftrace_event_call *call = &tu->call;
 
+	WARN_ON(call != ftrace_file->event_call);
+
+	if (test_bit(FTRACE_EVENT_FL_SOFT_DISABLED_BIT, &ftrace_file->flags))
+		return;
+
 	size = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));
-	event = trace_current_buffer_lock_reserve(&buffer, call->event.type,
-						  size + tu->size, 0, 0);
+	event = trace_event_buffer_lock_reserve(&buffer, ftrace_file,
+						call->event.type,
+						size + tu->size, 0, 0);
 	if (!event)
 		return;
 

Quoted 12 lines

kernel/trace/trace_uprobe.c static void uprobe_trace_print(struct trace_uprobe *tu, · line 564

Quoted 12 lines

kernel/trace/trace_uprobe.c +6 -2

@@ -548,15 +556,35 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
 /* uprobe handler */
 static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 {
-	if (!is_ret_probe(tu))
-		uprobe_trace_print(tu, 0, regs);
+	struct ftrace_event_file **file;
+
+	if (is_ret_probe(tu))
+		return 0;
+
+	file = rcu_dereference_raw(tu->files);

Why are you using rcu_dereference_raw() and not rcu_dereference(). The _raw() is a bit special, and unless you know what you are doing with RCU here, don't use it.

As I see you using rcu_dereference_raw() all over this patch, along with mutexes, I believe that you are not using RCU correctly here.

-- Steve

Quoted 220 lines

Quoted 218 lines

kernel/trace/trace_uprobe.c +144 -17

+	if (unlikely(!file))
+		return 0;
+
+	while (*file) {
+		uprobe_trace_print(tu, 0, regs, *file);
+		file++;
+	}
+
 	return 0;
 }
 
 static void uretprobe_trace_func(struct trace_uprobe *tu, unsigned long func,
 				struct pt_regs *regs)
 {
-	uprobe_trace_print(tu, func, regs);
+	struct ftrace_event_file **file = rcu_dereference_raw(tu->files);
+
+	if (unlikely(!file))
+		return;
+
+	while (*file) {
+		uprobe_trace_print(tu, func, regs, *file);
+		file++;
+	}
 }
 
 /* Event entry printers */

@@ -597,6 +625,26 @@ partial:
 	return TRACE_TYPE_PARTIAL_LINE;
 }
 
+
+static int trace_uprobe_nr_files(struct trace_uprobe *tu)
+{
+	struct ftrace_event_file **file;
+	int ret = 0;
+
+	/*
+	 * Since all tu->files updater is protected by uprobe_enable_lock,
+	 * we don't need to lock an rcu_read_lock.
+	 */
+	file = rcu_dereference_raw(tu->files);
+	if (file)
+		while (*(file++))
+			ret++;
+
+	return ret;
+}
+
+static DEFINE_MUTEX(uprobe_enable_lock);
+
 static inline bool is_trace_uprobe_enabled(struct trace_uprobe *tu)
 {
 	return tu->flags & (TP_FLAG_TRACE | TP_FLAG_PROFILE);

@@ -607,33 +655,123 @@ typedef bool (*filter_func_t)(struct uprobe_consumer *self,
 				struct mm_struct *mm);
 
 static int
-probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
+probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
+		   filter_func_t filter)
 {
+	int enabled = 0;
 	int ret = 0;
 
+	mutex_lock(&uprobe_enable_lock);
+
 	if (is_trace_uprobe_enabled(tu))
-		return -EINTR;
+		enabled = 1;
+
+	if (file) {
+		struct ftrace_event_file **new, **old;
+		int n = trace_uprobe_nr_files(tu);
+
+		old = rcu_dereference_raw(tu->files);
+		/* 1 is for new one and 1 is for stopper */
+		new = kzalloc((n + 2) * sizeof(struct ftrace_event_file *),
+			      GFP_KERNEL);
+		if (!new) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+		memcpy(new, old, n * sizeof(struct ftrace_event_file *));
+		new[n] = file;
+		/* The last one keeps a NULL */
+
+		rcu_assign_pointer(tu->files, new);
+		tu->flags |= TP_FLAG_TRACE;
+
+		if (old) {
+			/* Make sure the probe is done with old files */
+			synchronize_sched();
+			kfree(old);
+		}
+	} else
+		tu->flags |= TP_FLAG_PROFILE;
 
 	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
 
-	tu->flags |= flag;
-	tu->consumer.filter = filter;
-	ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
-	if (ret)
-		tu->flags &= ~flag;
+	if (!enabled) {
+		tu->consumer.filter = filter;
+		ret = uprobe_register(tu->inode, tu->offset, &tu->consumer);
+		if (ret)
+			tu->flags &= file ? ~TP_FLAG_TRACE : ~TP_FLAG_PROFILE;
+	}
 
+ out_unlock:
+	mutex_unlock(&uprobe_enable_lock);
 	return ret;
 }
 
-static void probe_event_disable(struct trace_uprobe *tu, int flag)
+static int
+trace_uprobe_file_index(struct trace_uprobe *tu, struct ftrace_event_file *file)
 {
-	if (!is_trace_uprobe_enabled(tu))
-		return;
+	struct ftrace_event_file **files;
+	int i;
+
+	/*
+	 * Since all tu->files updater is protected by uprobe_enable_lock,
+	 * we don't need to lock an rcu_read_lock.
+	 */
+	files = rcu_dereference_raw(tu->files);
+	if (files) {
+		for (i = 0; files[i]; i++)
+			if (files[i] == file)
+				return i;
+	}
+
+	return -1;
+}
+
+static void
+probe_event_disable(struct trace_uprobe *tu, struct ftrace_event_file *file)
+{
+	mutex_lock(&uprobe_enable_lock);
+
+	if (file) {
+		struct ftrace_event_file **new, **old;
+		int n = trace_uprobe_nr_files(tu);
+		int i, j;
+
+		old = rcu_dereference_raw(tu->files);
+		if (n == 0 || trace_uprobe_file_index(tu, file) < 0)
+			goto out_unlock;
+
+		if (n == 1) {	/* Remove the last file */
+			tu->flags &= ~TP_FLAG_TRACE;
+			new = NULL;
+		} else {
+			new = kzalloc(n * sizeof(struct ftrace_event_file *),
+				      GFP_KERNEL);
+			if (!new)
+				goto out_unlock;
+
+			/* This copy & check loop copies the NULL stopper too */
+			for (i = 0, j = 0; j < n && i < n + 1; i++)
+				if (old[i] != file)
+					new[j++] = old[i];
+		}
+
+		rcu_assign_pointer(tu->files, new);
+
+		/* Make sure the probe is done with old files */
+		synchronize_sched();
+		kfree(old);
+	} else
+		tu->flags &= ~TP_FLAG_PROFILE;
+
 
 	WARN_ON(!uprobe_filter_is_empty(&tu->filter));
 
-	uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
-	tu->flags &= ~flag;
+	if (!is_trace_uprobe_enabled(tu))
+		uprobe_unregister(tu->inode, tu->offset, &tu->consumer);
+
+ out_unlock:
+	mutex_unlock(&uprobe_enable_lock);
 }
 
 static int uprobe_event_define_fields(struct ftrace_event_call *event_call)

@@ -869,21 +1007,22 @@ static
 int trace_uprobe_register(struct ftrace_event_call *event, enum trace_reg type, void *data)
 {
 	struct trace_uprobe *tu = event->data;
+	struct ftrace_event_file *file = data;
 
 	switch (type) {
 	case TRACE_REG_REGISTER:
-		return probe_event_enable(tu, TP_FLAG_TRACE, NULL);
+		return probe_event_enable(tu, file, NULL);
 
 	case TRACE_REG_UNREGISTER:
-		probe_event_disable(tu, TP_FLAG_TRACE);
+		probe_event_disable(tu, file);
 		return 0;
 
 #ifdef CONFIG_PERF_EVENTS
 	case TRACE_REG_PERF_REGISTER:
-		return probe_event_enable(tu, TP_FLAG_PROFILE, uprobe_perf_filter);
+		return probe_event_enable(tu, NULL, uprobe_perf_filter);
 
 	case TRACE_REG_PERF_UNREGISTER:
-		probe_event_disable(tu, TP_FLAG_PROFILE);
+		probe_event_disable(tu, NULL);
 		return 0;
 
 	case TRACE_REG_PERF_OPEN:

Quoted 17 lines

Quoted 4 lines

Quoted 4 lines

kernel/trace/trace_uprobe.c +0 -0

@@ -548,15 +556,35 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
 /* uprobe handler */
 static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 {

Quoted 8 lines

kernel/trace/trace_uprobe.c static void uprobe_trace_print(struct trace_uprobe *tu, · line 564

Quoted 8 lines

kernel/trace/trace_uprobe.c +6 -2

-	if (!is_ret_probe(tu))
-		uprobe_trace_print(tu, 0, regs);
+	struct ftrace_event_file **file;
+
+	if (is_ret_probe(tu))
+		return 0;
+
+	file = rcu_dereference_raw(tu->files);

Why are you using rcu_dereference_raw() and not rcu_dereference(). The _raw() is a bit special, and unless you know what you are doing with RCU here, don't use it.

Quoted 2 lines

As I see you using rcu_dereference_raw() all over this patch, along with mutexes, I believe that you are not using RCU correctly here.

If irqs and preempt are disabled, I suggest using rcu_dereference_sched(). That is what it is there for. ;-)

Quoted 20 lines · Paul E. McKenney

Quoted 19 lines

Quoted 4 lines

Quoted 4 lines

kernel/trace/trace_uprobe.c +0 -0

@@ -548,15 +556,35 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
 /* uprobe handler */
 static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 {

Quoted 8 lines

kernel/trace/trace_uprobe.c static void uprobe_trace_print(struct trace_uprobe *tu, · line 564

Quoted 8 lines

kernel/trace/trace_uprobe.c +6 -2

-	if (!is_ret_probe(tu))
-		uprobe_trace_print(tu, 0, regs);
+	struct ftrace_event_file **file;
+
+	if (is_ret_probe(tu))
+		return 0;
+
+	file = rcu_dereference_raw(tu->files);

Why are you using rcu_dereference_raw() and not rcu_dereference(). The _raw() is a bit special, and unless you know what you are doing with RCU here, don't use it.

As I see you using rcu_dereference_raw() all over this patch, along with mutexes, I believe that you are not using RCU correctly here.

Quoted 2 lines

If irqs and preempt are disabled, I suggest using rcu_dereference_sched(). That is what it is there for. ;-)

I believe this just copied what kprobes did, where irqs and preemption is disabled. I don't believe that these probes have that same luxury.

But that begs the question. Should we convert the rcu_dereference_raw() in the kprobe code to rcu_dereference_sched()?

Quoted 22 lines · Steven Rostedt

Quoted 20 lines · Paul E. McKenney

Quoted 19 lines

Quoted 4 lines

Quoted 4 lines

kernel/trace/trace_uprobe.c +0 -0

@@ -548,15 +556,35 @@ static void uprobe_trace_print(struct trace_uprobe *tu,
 /* uprobe handler */
 static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
 {

Quoted 8 lines

kernel/trace/trace_uprobe.c static void uprobe_trace_print(struct trace_uprobe *tu, · line 564

Quoted 8 lines

kernel/trace/trace_uprobe.c +6 -2

-	if (!is_ret_probe(tu))
-		uprobe_trace_print(tu, 0, regs);
+	struct ftrace_event_file **file;
+
+	if (is_ret_probe(tu))
+		return 0;
+
+	file = rcu_dereference_raw(tu->files);

Why are you using rcu_dereference_raw() and not rcu_dereference(). The _raw() is a bit special, and unless you know what you are doing with RCU here, don't use it.

As I see you using rcu_dereference_raw() all over this patch, along with mutexes, I believe that you are not using RCU correctly here.

Quoted 8 lines

Quoted 2 lines

If irqs and preempt are disabled, I suggest using rcu_dereference_sched(). That is what it is there for. ;-)

I believe this just copied what kprobes did, where irqs and preemption is disabled. I don't believe that these probes have that same luxury.

But that begs the question. Should we convert the rcu_dereference_raw() in the kprobe code to rcu_dereference_sched()?

It makes a lot of sense to me, at least assuming no issues with the interrupts being disabled, but the checks not spotting this. Here is the check:

	preempt_count() != 0 || irqs_disabled()

(With additional elaboration for if lockdep is enabled.)

Quoted 7 lines

It makes a lot of sense to me, at least assuming no issues with the interrupts being disabled, but the checks not spotting this. Here is the check:

	preempt_count() != 0 || irqs_disabled()

(With additional elaboration for if lockdep is enabled.)

OK, I see. So I'll convert all the rcu_dereference_raw() to rcu_dereference_sched() except kprobe handler, because in the kprobe handler above check always be true. :)

Quoted 11 lines · Masami Hiramatsu

Quoted 7 lines

It makes a lot of sense to me, at least assuming no issues with the interrupts being disabled, but the checks not spotting this. Here is the check:

	preempt_count() != 0 || irqs_disabled()

(With additional elaboration for if lockdep is enabled.)

OK, I see. So I'll convert all the rcu_dereference_raw() to rcu_dereference_sched() except kprobe handler, because in the kprobe handler above check always be true. :)

I would convert them all to rcu_dereference_sched(), the above check is only when CONFIG_DEBUG_LOCK_ALLOC is set. It also annotates what is protecting this variable. Please do not avoid a check because "it's always true here". You also get people copying that code (like for uprobes) and that will skip the check too.

The only reason ftrace function tracer uses the raw (and now raw_notrace) version is because it is extremely invasive, and these checks done at *every* function call can actually live lock the system. But other places in tracing use the appropriate rcu_dereference_*() functions. If they do not, then they need to be fixed too.

Quoted 5 lines · Steven Rostedt

The only reason ftrace function tracer uses the raw (and now raw_notrace) version is because it is extremely invasive, and these checks done at *every* function call can actually live lock the system. But other places in tracing use the appropriate rcu_dereference_*() functions. If they do not, then they need to be fixed too.

Oh, I see.
Anyway, it will be completely replaced by the Oleg's patch. Or should I fix that before his work?

Support multi-buffer on uprobe-based dynamic events by using ftrace_event_file.

The code change is based on kprobe-based dynamic events multibuffer support work commited by Masami(commit 41a7dd420c)

kernel/trace/trace_uprobe.c +2 -0

+static DEFINE_MUTEX(uprobe_enable_lock);
+
 static inline bool is_trace_uprobe_enabled(struct trace_uprobe *tu)
 {
 	return tu->flags & (TP_FLAG_TRACE | TP_FLAG_PROFILE);

@@ -607,33 +655,123 @@ typedef bool (*filter_func_t)(struct uprobe_consumer *self,
 				struct mm_struct *mm);

kernel/trace/trace_uprobe.c +18 -1

 	if (is_trace_uprobe_enabled(tu))
-		return -EINTR;
+		enabled = 1;
+
+	if (file) {
+		struct ftrace_event_file **new, **old;
+		int n = trace_uprobe_nr_files(tu);
+
+		old = rcu_dereference_raw(tu->files);
+		/* 1 is for new one and 1 is for stopper */
+		new = kzalloc((n + 2) * sizeof(struct ftrace_event_file *),
+			      GFP_KERNEL);
+		if (!new) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+		memcpy(new, old, n * sizeof(struct ftrace_event_file *));
+		new[n] = file;
+		/* The last one keeps a NULL */
+

Review

Quoted 9 lines

(quoted diff) +4 -1

-probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
+probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
+		   filter_func_t filter)
 {
+	int enabled = 0;
 	int ret = 0;
 
+	mutex_lock(&uprobe_enable_lock);

Do we really need this? Can't we really on mutex_event hold by the caller?

So perhaps something like below (untested) makes sense? With this patch we can trivially convert trace_kprobe.c to use list_add/del/each_rcu.

Quoted 11 lines · Oleg Nesterov

Review

Quoted 9 lines

(quoted diff) +4 -1

-probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter)
+probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file,
+		   filter_func_t filter)
 {
+	int enabled = 0;
 	int ret = 0;
 
+	mutex_lock(&uprobe_enable_lock);

Do we really need this? Can't we really on mutex_event hold by the caller?

Looks like, kprobes do not need probe_enable_lock too.

Steven, Masami, I just looked at this new multibuffer code. Not sure I really understand it, but it seems that ftrace_event_file should help its users.

Lets look at enable_trace_probe(). Firstly, "ftrace_event_file **files" and the add/remove code doesn't look very nice, list_head looks more convenient.

But the main problem is, synchronize_sched() is slow and it is called under the global event_mutex.

So perhaps something like below (untested) makes sense? With this patch we can trivially convert trace_kprobe.c to use list_add/del/each_rcu.

What do you think?

Oleg.

include/linux/ftrace_event.h +6 -0

 };
 
+struct event_file_link {
+	struct ftrace_event_file	*file;
+	struct list_head		list;
+	struct rcu_head			rcu;
+};
+

include/linux/ftrace_event.h +6 -0

+
+static inline void free_event_file_link(struct event_file_link *link)
+{
+	call_rcu(&link->rcu, rcu_free_event_file_link);
+}
+
 #define __TRACE_EVENT_FLAGS(name, value)				\
 	static int __init trace_init_flags_##name(void)			\
 	{								\

kernel/trace/trace_events.c +12 -0

@@ -1542,6 +1542,7 @@ trace_create_new_event(struct ftrace_event_call *call,
 	file->event_call = call;
 	file->tr = tr;
 	atomic_set(&file->sm_ref, 0);
+	atomic_set(&file->refcnt, 1);
 	list_add(&file->list, &tr->events);
 
 	return file;

@@ -2182,6 +2183,17 @@ __trace_early_add_events(struct trace_array *tr)
 	}
 }
 
+static void put_event_file(struct ftrace_event_file *file)
+{
+	if (atomic_dec_and_test(&file->refcnt))
+		kmem_cache_free(file_cachep, file);
+}
+
+static void delayed_put_event_file(struct rcu_head *rcu)
+{
+	put_event_file(container_of(rcu, struct ftrace_event_file, rcu));
+}
+
 /* Remove the event directory structure for a trace directory. */
 static void
 __trace_remove_event_dirs(struct trace_array *tr)

kernel/trace/trace_events.c +8 -0

 	}
 }
 
+void rcu_free_event_file_link(struct rcu_head *rcu)
+{
+	struct event_file_link *link =
+			container_of(rcu, struct event_file_link, rcu);
+	put_event_file(link->file);
+	kfree(link);
+}
+
 static void
 __add_event_to_tracers(struct ftrace_event_call *call,
 		       struct ftrace_module_file_ops *file_ops)

Quoted 3 lines · Oleg Nesterov

But the main problem is, synchronize_sched() is slow and it is called under the global event_mutex.

But is that really an issue? event_mutex is used to add or remove events, and this happens only when we load or unload a module, or add or remove a probe. These are mostly user operations that take a relatively long time to complete (but not relative to humans).

kernel/trace/trace_uprobe.c typedef bool (*filter_func_t)(struct uprobe_consumer *self, · line 694

Quoted 11 lines · zhangwei(Jovi)

kernel/trace/trace_uprobe.c +10 -0

+		rcu_assign_pointer(tu->files, new);
+		tu->flags |= TP_FLAG_TRACE;
+
+		if (old) {
+			/* Make sure the probe is done with old files */
+			synchronize_sched();
+			kfree(old);
+		}
+	} else
+		tu->flags |= TP_FLAG_PROFILE;

So it can set both TP_FLAG_TRACE and TP_FLAG_PROFILE, yes?

If yes, this is not right. Until we change the pre-filtering at least. Currently TP_FLAG_TRACE/TP_FLAG_PROFILE are mutually exclusive.

Hi,

Now that we finished the discussion of the similar code in kprobes, let me summarize.

Quoted 16 lines · Oleg Nesterov

kernel/trace/trace_uprobe.c typedef bool (*filter_func_t)(struct uprobe_consumer *self, · line 694

Quoted 11 lines · zhangwei(Jovi)

kernel/trace/trace_uprobe.c +10 -0

+		rcu_assign_pointer(tu->files, new);
+		tu->flags |= TP_FLAG_TRACE;
+
+		if (old) {
+			/* Make sure the probe is done with old files */
+			synchronize_sched();
+			kfree(old);
+		}
+	} else
+		tu->flags |= TP_FLAG_PROFILE;

So it can set both TP_FLAG_TRACE and TP_FLAG_PROFILE, yes?

Quoted 2 lines

If yes, this is not right. Until we change the pre-filtering at least. Currently TP_FLAG_TRACE/TP_FLAG_PROFILE are mutually exclusive.

Yes. So please update this patch so that TP_FLAG_PROFILE can't be set if TP_FLAG_TRACE is set and vice versa.

Once again, this limitation is artificial. But it was always here, and we need more changes to remove it. I'll try to do this later. (but if you want to play with this code - welcome ;)

Don't add the mutex, and do not use array-of-pointers (I hope you noticed the recent discussion).

Locking. Oh, OK. Let it be rcu for now (but please do not forget that you need rcu_read_lock, uprobe handlers run in the sleepable context). This is sub-optimal, but this is just another indication that uprobes API is not perfect, we can't use uprobe->register_sem.

Also. When I was reading trace_kprobes.c I notice the new (and imho useful) feature, soft disable/enable. Perhaps you can make a 2nd patch which adds the FTRACE_EVENT_FL_SOFT_DISABLED_BIT check?

Oleg.