tracing: Addition of set_event_pid

From: "Steven Rostedt (Red Hat)" <[email protected]>

In order to guarantee that a probe will be called before other probes that are attached to a tracepoint, there needs to be a mechanism to provide priority of one probe over the others.

Adding a prio field to the struct tracepoint_func, which lets the probes be sorted by the priority set in the structure. If no priority is specified, then a priority of 10 is given (this is a macro, and perhaps may be changed in the future).

Now probes may be added to affect other probes that are attached to a tracepoint with a guaranteed order.

One use case would be to allow tracing of tracepoints be able to filter by pid. A special (higher priority probe) may be added to the sched_switch tracepoint and set the necessary flags of the other tracepoints to notify them if they should be traced or not. In case a tracepoint is enabled at the sched_switch tracepoint too, the order of the two are not random.

Cc Mathieu Desnoyers <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
include/linux/tracepoint.h | 13 ++++++++++
kernel/tracepoint.c | 61 +++++++++++++++++++++++++++++++++++++---------

2 files changed, 63 insertions(+), 11 deletions(-)

include/linux/tracepoint.h +3 -0

index afada369c5b7..6b79537a42b1 100644
@@ -26,6 +26,7 @@ struct notifier_block;
 struct tracepoint_func {
 	void *func;
 	void *data;
+	int prio;
 };
 
 struct tracepoint {

@@ -42,9 +43,14 @@ struct trace_enum_map {
 	unsigned long		enum_value;
 };
 
+#define TRACEPOINT_DEFAULT_PRIO	10
+

extern int
tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data); extern int

include/linux/tracepoint.h +3 -0

+tracepoint_probe_register_prio(struct tracepoint *tp, void *probe, void *data,
+			       int prio);
+extern int

tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data); extern void
for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),

include/linux/tracepoint.h +7 -0

@@ -207,6 +213,13 @@ extern void syscall_unregfunc(void);
 						(void *)probe, data);	\
 	}								\
 	static inline int						\
+	register_trace_prio_##name(void (*probe)(data_proto), void *data,\
+				   int prio)				\
+	{								\
+		return tracepoint_probe_register_prio(&__tracepoint_##name, \
+					      (void *)probe, data, prio); \
+	}								\
+	static inline int						\
 	unregister_trace_##name(void (*probe)(data_proto), void *data)	\
 	{								\
 		return tracepoint_probe_unregister(&__tracepoint_##name,\

kernel/tracepoint.c +5 -3

index 3490407dc7b7..ecd536de603a 100644
@@ -91,11 +91,13 @@ static void debug_print_probes(struct tracepoint_func
 *funcs)
 		printk(KERN_DEBUG "Probe %d : %p\n", i, funcs[i].func);
 }
 
-static struct tracepoint_func *func_add(struct tracepoint_func **funcs,
-		struct tracepoint_func *tp_func)
+static struct tracepoint_func *
+func_add(struct tracepoint_func **funcs, struct tracepoint_func *tp_func,
+	 int prio)
 {
-	int nr_probes = 0;
 	struct tracepoint_func *old, *new;
+	int nr_probes = 0;
+	int pos = -1;
 
 	if (WARN_ON(!tp_func->func))

		return ERR_PTR(-EINVAL);

kernel/tracepoint.c +19 -4

@@ -104,18 +106,33 @@ static struct tracepoint_func *func_add(struct
 tracepoint_func **funcs,
 	old = *funcs;
 	if (old) {
 		/* (N -> N+1), (N != 0, 1) probes */
-		for (nr_probes = 0; old[nr_probes].func; nr_probes++)
+		for (nr_probes = 0; old[nr_probes].func; nr_probes++) {
+			/* Insert before probes of lower priority */
+			if (pos < 0 && old[nr_probes].prio < prio)
+				pos = nr_probes;
 			if (old[nr_probes].func == tp_func->func &&
 			    old[nr_probes].data == tp_func->data)
 				return ERR_PTR(-EEXIST);
+		}
 	}
 	/* + 2 : one for new probe, one for NULL func */
 	new = allocate_probes(nr_probes + 2);
 	if (new == NULL)
 		return ERR_PTR(-ENOMEM);
-	if (old)
-		memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
-	new[nr_probes] = *tp_func;
+	if (old) {
+		if (pos < 0) {
+			pos = nr_probes;
+			memcpy(new, old, nr_probes * sizeof(struct tracepoint_func));
+		} else {
+			/* Copy higher priority probes ahead of the new probe */
+			memcpy(new, old, pos * sizeof(struct tracepoint_func));
+			/* Copy the rest after it. */
+			memcpy(new + pos + 1, old + pos,
+			       (nr_probes - pos) * sizeof(struct tracepoint_func));
+		}
+	} else
+		pos = 0;
+	new[pos] = *tp_func;
 	new[nr_probes + 1].func = NULL;
 	*funcs = new;

	debug_print_probes(*funcs);

kernel/tracepoint.c +26 -4

@@ -174,7 +191,7 @@ static void *func_remove(struct tracepoint_func **funcs,
 * Add the probe function to a tracepoint.
 */
 static int tracepoint_add_func(struct tracepoint *tp,
-		struct tracepoint_func *func)
+			       struct tracepoint_func *func, int prio)
 {
 	struct tracepoint_func *old, *tp_funcs;
 

@@ -183,7 +200,7 @@ static int tracepoint_add_func(struct tracepoint *tp,
 
 	tp_funcs = rcu_dereference_protected(tp->funcs,
 			lockdep_is_held(&tracepoints_mutex));
-	old = func_add(&tp_funcs, func);
+	old = func_add(&tp_funcs, func, prio);
 	if (IS_ERR(old)) {
 		WARN_ON_ONCE(1);
 		return PTR_ERR(old);

@@ -240,6 +257,7 @@ static int tracepoint_remove_func(struct tracepoint *tp,
 * @tp: tracepoint
 * @probe: probe handler
 * @data: tracepoint data
+ * @prio: priority of this function over other registered functions
 *
 * Returns 0 if ok, error value on error.
 * Note: if @tp is within a module, the caller is responsible for

@@ -247,7 +265,8 @@ static int tracepoint_remove_func(struct tracepoint *tp,
 * performed either with a tracepoint module going notifier, or from
 * within module exit functions.
 */
-int tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data)
+int tracepoint_probe_register_prio(struct tracepoint *tp, void *probe,
+				   void *data, int prio)
 {
 	struct tracepoint_func tp_func;
 	int ret;

@@ -255,10 +274,30 @@ int tracepoint_probe_register(struct tracepoint *tp, void
 *probe, void *data)
 	mutex_lock(&tracepoints_mutex);
 	tp_func.func = probe;
 	tp_func.data = data;
-	ret = tracepoint_add_func(tp, &tp_func);
+	tp_func.prio = prio;
+	ret = tracepoint_add_func(tp, &tp_func, prio);
 	mutex_unlock(&tracepoints_mutex);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(tracepoint_probe_register_prio);
+
+/**
+ * tracepoint_probe_register -  Connect a probe to a tracepoint
+ * @tp: tracepoint
+ * @probe: probe handler
+ * @data: tracepoint data
+ * @prio: priority of this function over other registered functions
+ *
+ * Returns 0 if ok, error value on error.
+ * Note: if @tp is within a module, the caller is responsible for
+ * unregistering the probe before the module is gone. This can be
+ * performed either with a tracepoint module going notifier, or from
+ * within module exit functions.
+ */
+int tracepoint_probe_register(struct tracepoint *tp, void *probe, void *data)
+{
+	return tracepoint_probe_register_prio(tp, probe, data,
 TRACEPOINT_DEFAULT_PRIO);
+}
 EXPORT_SYMBOL_GPL(tracepoint_probe_register);

/**
--
2.6.1

Quoted 20 lines · ----- On Oct 29, 2015, at 3:07 AM, rostedt [email protected] wrote:

From: "Steven Rostedt (Red Hat)" <[email protected]>

In order to guarantee that a probe will be called before other probes that are attached to a tracepoint, there needs to be a mechanism to provide priority of one probe over the others.

Adding a prio field to the struct tracepoint_func, which lets the probes be sorted by the priority set in the structure. If no priority is specified, then a priority of 10 is given (this is a macro, and perhaps may be changed in the future).

Now probes may be added to affect other probes that are attached to a tracepoint with a guaranteed order.

One use case would be to allow tracing of tracepoints be able to filter by pid. A special (higher priority probe) may be added to the sched_switch tracepoint and set the necessary flags of the other tracepoints to notify them if they should be traced or not. In case a tracepoint is enabled at the sched_switch tracepoint too, the order of the two are not random.

Quoted 2 lines

Cc Mathieu Desnoyers <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

Sounds good to me,

Acked-by Mathieu Desnoyers <[email protected]>

Quoted 27 lines · On Thu, 29 Oct 2015 17:52:16 +0000 (UTC) Mathieu Desnoyers…

Quoted 14 lines · ----- On Oct 29, 2015, at 3:07 AM, rostedt [email protected] wrote:

From: "Steven Rostedt (Red Hat)" <[email protected]>

In order to guarantee that a probe will be called before other probes that are attached to a tracepoint, there needs to be a mechanism to provide priority of one probe over the others.

Adding a prio field to the struct tracepoint_func, which lets the probes be sorted by the priority set in the structure. If no priority is specified, then a priority of 10 is given (this is a macro, and perhaps may be changed in the future).

Now probes may be added to affect other probes that are attached to a tracepoint with a guaranteed order.

Quoted 8 lines

One use case would be to allow tracing of tracepoints be able to filter by pid. A special (higher priority probe) may be added to the sched_switch tracepoint and set the necessary flags of the other tracepoints to notify them if they should be traced or not. In case a tracepoint is enabled at the sched_switch tracepoint too, the order of the two are not random.

Cc Mathieu Desnoyers <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

Sounds good to me,

Quoted 2 lines

Acked-by Mathieu Desnoyers <[email protected]>

Thanks.

Unfortunately I already pushed it out to linux-next (had a deadline), otherwise I could have added your ack. I'm guessing you missed the RFC version I posted a week ago.

-- Steve

From: "Steven Rostedt (Red Hat)" <[email protected]>

Create a tracing directory called set_event_pid, which currently has no function, but will be used to filter all events for the tracing instance or the pids that are added to the file.

The reason no functionality is added with this commit is that this commit focuses on the creation and removal of the pids in a safe manner. And tests can be made against this change to make sure things are correct before hooking features to the list of pids.

Cc "Paul E. McKenney" <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/trace/trace.h        |   7 ++
 kernel/trace/trace_events.c | 287 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 294 insertions(+)

kernel/trace/trace.h +7 -0

index fb8a61c710ea..250481043bb5 100644
@@ -176,6 +176,12 @@ struct trace_options {
 	struct trace_option_dentry	*topts;
 };
 
+struct trace_pid_list {
+	unsigned int			nr_pids;
+	int				order;
+	pid_t				*pids;
+};
+
 /*
  * The trace array - an array of per-CPU trace arrays. This is the
  * highest level data structure that individual tracers deal with.

@@ -201,6 +207,7 @@ struct trace_array {
 	bool			allocated_snapshot;
 	unsigned long		max_latency;
 #endif
+	struct trace_pid_list	__rcu *filtered_pids;
 	/*
 	 * max_lock is used to protect the swapping of buffers
 	 * when taking a max snapshot. The buffers themselves are

kernel/trace/trace_events.c +42 -0

index d120cfe3cca7..2ad7014707ee 100644
@@ -15,8 +15,10 @@
 #include <linux/kthread.h>
 #include <linux/tracefs.h>
 #include <linux/uaccess.h>
+#include <linux/bsearch.h>
 #include <linux/module.h>
 #include <linux/ctype.h>
+#include <linux/sort.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
 

@@ -445,6 +447,43 @@ static void ftrace_clear_events(struct trace_array *tr)
 	mutex_unlock(&event_mutex);
 }
 
+static int cmp_pid(const void *key, const void *elt)
+{
+	const pid_t *search_pid = key;
+	const pid_t *pid = elt;
+
+	if (*search_pid == *pid)
+		return 0;
+	if (*search_pid < *pid)
+		return -1;
+	return 1;
+}
+
+static void __ftrace_clear_event_pids(struct trace_array *tr)
+{
+	struct trace_pid_list *pid_list;
+
+	pid_list = rcu_dereference_protected(tr->filtered_pids,
+					     lockdep_is_held(&event_mutex));
+	if (!pid_list)
+		return;
+
+	rcu_assign_pointer(tr->filtered_pids, NULL);
+
+	/* Wait till all users are no longer using pid filtering */
+	synchronize_sched();
+
+	free_pages((unsigned long)pid_list->pids, pid_list->order);
+	kfree(pid_list);
+}
+
+static void ftrace_clear_event_pids(struct trace_array *tr)
+{
+	mutex_lock(&event_mutex);
+	__ftrace_clear_event_pids(tr);
+	mutex_unlock(&event_mutex);
+}
+
 static void __put_system(struct event_subsystem *system)
 {
 	struct event_filter *filter = system->filter;

@@ -777,6 +816,56 @@ static void t_stop(struct seq_file *m, void *p)
 	mutex_unlock(&event_mutex);
 }
 
+static void *p_start(struct seq_file *m, loff_t *pos)
+{
+	struct trace_pid_list *pid_list;

kernel/trace/trace_events.c +36 -0

+
+	pid_list = rcu_dereference_sched(tr->filtered_pids);
+
+	if (!pid_list || *pos >= pid_list->nr_pids)
+		return NULL;
+
+	return (void *)&pid_list->pids[*pos];
+}
+
+static void p_stop(struct seq_file *m, void *p)
+{
+	rcu_read_unlock_sched();
+	mutex_unlock(&event_mutex);
+}
+
+static void *
+p_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	struct trace_array *tr = m->private;
+	struct trace_pid_list *pid_list = rcu_dereference_sched(tr->filtered_pids);
+
+	(*pos)++;
+
+	if (*pos >= pid_list->nr_pids)
+		return NULL;
+
+	return (void *)&pid_list->pids[*pos];
+}
+
+static int p_show(struct seq_file *m, void *v)
+{
+	pid_t *pid = v;
+
+	seq_printf(m, "%d\n", *pid);
+	return 0;
+}

Quoted 8 lines

kernel/trace/trace_events.c +8 -0

+static void *p_start(struct seq_file *m, loff_t *pos)
+{
+	struct trace_pid_list *pid_list;
+	struct trace_array *tr = m->private;
+
+	/*
+	 * Grab the mutex, to keep calls to p_next() having the same
+	 * tr->filtered_pids as p_start() has.

kernel/trace/trace_events.c static void t_stop(struct seq_file *m, void *p) · line 831

Quoted 5 lines

kernel/trace/trace_events.c +5 -0

+	 * If we just passed the tr->filtered_pids around, then RCU would
+	 * have been enough, but doing that makes things more complex.
+	 */
+	mutex_lock(&event_mutex);
+	rcu_read_lock_sched();  

This looks interesting... You hold the mutex, which I am guessing blocks changes. Then why the need for rcu_read_lock_sched()?

Thanx, Paul

Quoted 36 lines

kernel/trace/trace_events.c +36 -0

+
+	pid_list = rcu_dereference_sched(tr->filtered_pids);
+
+	if (!pid_list || *pos >= pid_list->nr_pids)
+		return NULL;
+
+	return (void *)&pid_list->pids[*pos];
+}
+
+static void p_stop(struct seq_file *m, void *p)
+{
+	rcu_read_unlock_sched();
+	mutex_unlock(&event_mutex);
+}
+
+static void *
+p_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	struct trace_array *tr = m->private;
+	struct trace_pid_list *pid_list = rcu_dereference_sched(tr->filtered_pids);
+
+	(*pos)++;
+
+	if (*pos >= pid_list->nr_pids)
+		return NULL;
+
+	return (void *)&pid_list->pids[*pos];
+}
+
+static int p_show(struct seq_file *m, void *v)
+{
+	pid_t *pid = v;
+
+	seq_printf(m, "%d\n", *pid);
+	return 0;
+}  

Quoted 8 lines · On Thu, 19 Nov 2015 15:24:27 -0800 "Paul E. McKenney"…

Quoted 8 lines

kernel/trace/trace_events.c +8 -0

+static void *p_start(struct seq_file *m, loff_t *pos)
+{
+	struct trace_pid_list *pid_list;
+	struct trace_array *tr = m->private;
+
+	/*
+	 * Grab the mutex, to keep calls to p_next() having the same
+	 * tr->filtered_pids as p_start() has.

Quoted 8 lines

kernel/trace/trace_events.c static void t_stop(struct seq_file *m, void *p) · line 831

Quoted 5 lines

kernel/trace/trace_events.c +5 -0

+	 * If we just passed the tr->filtered_pids around, then RCU would
+	 * have been enough, but doing that makes things more complex.
+	 */
+	mutex_lock(&event_mutex);
+	rcu_read_lock_sched();  

This looks interesting... You hold the mutex, which I am guessing blocks changes. Then why the need for rcu_read_lock_sched()?

Technically you are correct. It's not needed. But I added it more for documentation :-)

Ideally, we wouldn't need the mutex here. But then we need to pass around the pid_list which makes it a bit more complex in the seq_file code than to pass around the tr (where we get pid_list from tr->filtered_pids).

And we do multiple rcu_dereference_sched()s, and for this code to work properly (give consistent output), the result should be the same. Hence, we grab the mutex, to keep the tr->filtered_pids to be consistent between the rcu_dereference_sched() calls, but since we are not modifying tr->filtered_pids(), and if we changed this code to do a single rcu_dereference_sched() and pass around the result, then we wouldn't need to grab the mutex, and the rcu_read_lock_sched() would be enough.

I could remove it and change the code to do rcu_dereferenced_lock() but to me that makes it sound like this code is an update path, which it is not.

Does this make sense in a crazy way?

-- Steve

Quoted 39 lines

Thanx, Paul

Quoted 36 lines

kernel/trace/trace_events.c +36 -0

+
+	pid_list = rcu_dereference_sched(tr->filtered_pids);
+
+	if (!pid_list || *pos >= pid_list->nr_pids)
+		return NULL;
+
+	return (void *)&pid_list->pids[*pos];
+}
+
+static void p_stop(struct seq_file *m, void *p)
+{
+	rcu_read_unlock_sched();
+	mutex_unlock(&event_mutex);
+}
+
+static void *
+p_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	struct trace_array *tr = m->private;
+	struct trace_pid_list *pid_list = rcu_dereference_sched(tr->filtered_pids);
+
+	(*pos)++;
+
+	if (*pos >= pid_list->nr_pids)
+		return NULL;
+
+	return (void *)&pid_list->pids[*pos];
+}
+
+static int p_show(struct seq_file *m, void *v)
+{
+	pid_t *pid = v;
+
+	seq_printf(m, "%d\n", *pid);
+	return 0;
+}  

static ssize_t
ftrace_event_pid_write(struct file *filp, const char __user *ubuf,

kernel/trace/trace_events.c +0 -0

 		       size_t cnt, loff_t *ppos)

kernel/trace/trace_events.c +0 -0

 	mutex_unlock(&event_mutex);

Quoted 9 lines · Steven Rostedt

static ssize_t
ftrace_event_pid_write(struct file *filp, const char __user *ubuf,

kernel/trace/trace_events.c +2 -0

 		       size_t cnt, loff_t *ppos)

@@ -1711,6 +1727,12 @@ ftrace_event_pid_write(struct file *filp, const char __user *ubuf,
 						 tr, INT_MAX);
 		register_trace_prio_sched_wakeup(event_filter_pid_sched_wakeup_probe_post,
 						 tr, 0);
+
+		/*

kernel/trace/trace_events.c ftrace_event_pid_write(struct file *filp, const char __user *ubuf, · line 1736

Quoted 5 lines

kernel/trace/trace_events.c +4 -0

+		 * Ignoring of pids is done at task switch. But we have to
+		 * check for those tasks that are currently running.
+		 */
+		on_each_cpu(ignore_task_cpu, tr, 1);
 	}

on_each_cpu(ignore_task_cpu, tr, 1); can be put outside of the else block, then ignore_pid can be updated when appending pid, eg. echo 'xxx' >> set_event_pid, in case xxx is running on some cpu.

Quoted 2 lines

kernel/trace/trace_events.c +0 -0

 	mutex_unlock(&event_mutex);