Skip to contentSkip to conversation

ftrace: clean ups for tip

PATCH In review Steven Rostedt 8 participants 45 messages Dec 4, 2008 – Dec 5, 2008

Subsystems (2)
Tracing ftrace
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover ftrace: clean ups for tip

Ingo,

The following patches are to have ftrace use the pid struct for filtering on pids.

The following patches are in:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

branch: tip/devel

Steven Rostedt (3):
fix the do_each_pid_task macro
ftrace: use struct pid
ftrace: add ability to only trace swapper tasks

----

 include/linux/pid.h   |    4 +-
 kernel/trace/ftrace.c |  128 ++++++++++++++++++++++++++++++++++++------------
 kernel/trace/trace.h  |    4 +-
 3 files changed, 100 insertions(+), 36 deletions(-)
author maintainer
#1 lore raw

PATCH 1/3 fix the do_each_pid_task macro

From: Steven Rostedt <[email protected]>

Impact: fix

This patch adds parenthesis around 'pid' in the do_each_pid_task macro to allow callers to pass in more complex parameters.

e.g. do_each_pid_task(*pid, type, task)

Signed-off-by Steven Rostedt <[email protected]>
Cc Eric W. Biederman <[email protected]>
---
 include/linux/pid.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
include/linux/pid.h +2 -2
index d7e98ff..bb206c5 100644
@@ -147,9 +147,9 @@ pid_t pid_vnr(struct pid *pid);
 #define do_each_pid_task(pid, type, task)				\
 	do {								\
 		struct hlist_node *pos___;				\
-		if (pid != NULL)					\
+		if ((pid) != NULL)					\
 			hlist_for_each_entry_rcu((task), pos___,	\
-				&pid->tasks[type], pids[type].node) {
+				&(pid)->tasks[type], pids[type].node) {
 
 			/*
 			 * Both old and new leaders may be attached to
Signature · 3 lines
1.5.6.5

--
author maintainer
#2 lore raw

PATCH 2/3 ftrace: use struct pid

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: clean up

Eric Biederman suggested using the struct pid for filtering on pids in the kernel. This patch is based off of a demonstration of an implementation that Eric sent me in an email.

Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/ftrace.c |   76 ++++++++++++++++++++++++++++--------------------
 kernel/trace/trace.h  |    4 +-
 2 files changed, 46 insertions(+), 34 deletions(-)
kernel/trace/ftrace.c +44 -32
index 57592a9..10b1d7c 100644
@@ -48,7 +48,7 @@ int ftrace_enabled __read_mostly;
 static int last_ftrace_enabled;
 
 /* set when tracing only a pid */
-int ftrace_pid_trace = -1;
+struct pid *ftrace_pid_trace;
 
 /* Quick disabling of function tracer. */
 int function_trace_stop;

@@ -153,7 +153,7 @@ static int __register_ftrace_function(struct ftrace_ops *ops)
 		else
 			func = ftrace_list_func;
 
-		if (ftrace_pid_trace >= 0) {
+		if (ftrace_pid_trace) {
 			set_ftrace_pid_function(func);
 			func = ftrace_pid_func;
 		}

@@ -209,7 +209,7 @@ static int __unregister_ftrace_function(struct ftrace_ops *ops)
 		if (ftrace_list->next == &ftrace_list_end) {
 			ftrace_func_t func = ftrace_list->func;
 
-			if (ftrace_pid_trace >= 0) {
+			if (ftrace_pid_trace) {
 				set_ftrace_pid_function(func);
 				func = ftrace_pid_func;
 			}

@@ -239,7 +239,7 @@ static void ftrace_update_pid_func(void)
 
 	func = ftrace_trace_function;
 
-	if (ftrace_pid_trace >= 0) {
+	if (ftrace_pid_trace) {
 		set_ftrace_pid_function(func);
 		func = ftrace_pid_func;
 	} else {

@@ -1678,18 +1678,40 @@ ftrace_pid_read(struct file *file, char __user *ubuf,
 	char buf[64];
 	int r;
 
-	if (ftrace_pid_trace >= 0)
-		r = sprintf(buf, "%u\n", ftrace_pid_trace);
+	if (ftrace_pid_trace)
+		r = sprintf(buf, "%u\n", pid_nr(ftrace_pid_trace));
 	else
 		r = sprintf(buf, "no pid\n");
 
 	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
 }
 
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+	struct task_struct *p;
+
+	do_each_pid_task(*pid, PIDTYPE_PID, p) {
+		clear_tsk_trace_trace(p);
+	} while_each_pid_task(*pid, PIDTYPE_PID, p);
+	put_pid(*pid);
+
+	*pid = NULL;
+}
+
+static void set_ftrace_pid_task(struct pid *pid)
+{
+	struct task_struct *p;
+
+	do_each_pid_task(pid, PIDTYPE_PID, p) {
+		set_tsk_trace_trace(p);
+	} while_each_pid_task(pid, PIDTYPE_PID, p);
+}
+
 static ssize_t
 ftrace_pid_write(struct file *filp, const char __user *ubuf,
 		   size_t cnt, loff_t *ppos)
 {
+	struct pid *pid;
 	char buf[64];
 	long val;
 	int ret;

@@ -1707,40 +1729,30 @@ ftrace_pid_write(struct file *filp, const char __user *ubuf,
 		return ret;
 
 	mutex_lock(&ftrace_start_lock);
-	if (ret < 0) {
+	if (val < 0) {
 		/* disable pid tracing */
-		if (ftrace_pid_trace < 0)
+		if (!ftrace_pid_trace)
 			goto out;
-		ftrace_pid_trace = -1;
+
+		clear_ftrace_pid_task(&ftrace_pid_trace);
 
 	} else {
-		struct task_struct *p;
-		int found = 0;
+		pid = find_get_pid(val);
 
-		if (ftrace_pid_trace == val)
+		if (pid == ftrace_pid_trace) {
+			put_pid(pid);
 			goto out;
-
-		/*
-		 * Find the task that matches this pid.
-		 * TODO: use pid namespaces instead.
-		 */
-		rcu_read_lock();
-		for_each_process(p) {
-			if (p->pid == val) {
-				found = 1;
-				set_tsk_trace_trace(p);
-			} else if (test_tsk_trace_trace(p))
-				clear_tsk_trace_trace(p);
 		}
-		rcu_read_unlock();
 
-		if (found)
-			ftrace_pid_trace = val;
-		else {
-			if (ftrace_pid_trace < 0)
-				goto out;
-			ftrace_pid_trace = -1;
-		}
+		if (ftrace_pid_trace)
+			clear_ftrace_pid_task(&ftrace_pid_trace);
+
+		if (!pid)
+			goto out;
+
+		ftrace_pid_trace = pid;
+
+		set_ftrace_pid_task(ftrace_pid_trace);
 	}
 
 	/* update the function call */
kernel/trace/trace.h +2 -2
index 95fff37..8b81b4d 100644
@@ -541,11 +541,11 @@ print_graph_function(struct trace_iterator *iter)
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
-extern int ftrace_pid_trace;
+extern struct pid *ftrace_pid_trace;
 
 static inline int ftrace_trace_task(struct task_struct *task)
 {
-	if (ftrace_pid_trace < 0)
+	if (ftrace_pid_trace)
 		return 1;
 
 	return test_tsk_trace_trace(task);
Signature · 3 lines
1.5.6.5

--
#3 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

Quoted 7 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: clean up

Eric Biederman suggested using the struct pid for filtering on pids in the kernel. This patch is based off of a demonstration of an implementation that Eric sent me in an email.

A little nit. I forgot to mention rcu_read_lock() is still needed in that email.

Quoted 56 lines
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/ftrace.c |   76 ++++++++++++++++++++++++++++--------------------
 kernel/trace/trace.h  |    4 +-
 2 files changed, 46 insertions(+), 34 deletions(-)
kernel/trace/ftrace.c +2 -2
index 57592a9..10b1d7c 100644
@@ -48,7 +48,7 @@ int ftrace_enabled __read_mostly;
 static int last_ftrace_enabled;
 
 /* set when tracing only a pid */
-int ftrace_pid_trace = -1;
+struct pid *ftrace_pid_trace;
 
 /* Quick disabling of function tracer. */
 int function_trace_stop;

@@ -153,7 +153,7 @@ static int __register_ftrace_function(struct ftrace_ops
 *ops)
 		else
 			func = ftrace_list_func;
 
-		if (ftrace_pid_trace >= 0) {
+		if (ftrace_pid_trace) {
 			set_ftrace_pid_function(func);
 			func = ftrace_pid_func;
kernel/trace/ftrace.c +1 -1
 		}

@@ -209,7 +209,7 @@ static int __unregister_ftrace_function(struct ftrace_ops
 *ops)
 		if (ftrace_list->next == &ftrace_list_end) {
 			ftrace_func_t func = ftrace_list->func;
 
-			if (ftrace_pid_trace >= 0) {
+			if (ftrace_pid_trace) {
 				set_ftrace_pid_function(func);
 				func = ftrace_pid_func;
kernel/trace/ftrace.c +3 -3
 			}

@@ -239,7 +239,7 @@ static void ftrace_update_pid_func(void)
 
 	func = ftrace_trace_function;
 
-	if (ftrace_pid_trace >= 0) {
+	if (ftrace_pid_trace) {
 		set_ftrace_pid_function(func);
 		func = ftrace_pid_func;
 	} else {

@@ -1678,18 +1678,40 @@ ftrace_pid_read(struct file *file, char __user *ubuf,
 	char buf[64];
 	int r;
 
-	if (ftrace_pid_trace >= 0)
-		r = sprintf(buf, "%u\n", ftrace_pid_trace);
+	if (ftrace_pid_trace)
+		r = sprintf(buf, "%u\n", pid_nr(ftrace_pid_trace));

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1692

Quoted 10 lines
kernel/trace/ftrace.c +4 -0
 	else
 		r = sprintf(buf, "no pid\n");
 
 	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
 }
 
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+	struct task_struct *p;
+

	rcu_read_lock();

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1695

Quoted 3 lines
kernel/trace/ftrace.c +3 -0
+	do_each_pid_task(*pid, PIDTYPE_PID, p) {
+		clear_tsk_trace_trace(p);
+	} while_each_pid_task(*pid, PIDTYPE_PID, p);

rcu_read_unlock()

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1704

Quoted 9 lines
kernel/trace/ftrace.c +9 -0
+	put_pid(*pid);
+
+	*pid = NULL;
+}
+
+static void set_ftrace_pid_task(struct pid *pid)
+{
+	struct task_struct *p;
+

	rcu_read_lock();

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1707

Quoted 3 lines
kernel/trace/ftrace.c +3 -0
+	do_each_pid_task(pid, PIDTYPE_PID, p) {
+		set_tsk_trace_trace(p);
+	} while_each_pid_task(pid, PIDTYPE_PID, p);

	rcu_read_unlock();
Quoted 90 lines
kernel/trace/ftrace.c +19 -26
+}
+
 static ssize_t
 ftrace_pid_write(struct file *filp, const char __user *ubuf,
 		   size_t cnt, loff_t *ppos)
 {
+	struct pid *pid;
 	char buf[64];
 	long val;
 	int ret;

@@ -1707,40 +1729,30 @@ ftrace_pid_write(struct file *filp, const char __user
 *ubuf,
 		return ret;
 
 	mutex_lock(&ftrace_start_lock);
-	if (ret < 0) {
+	if (val < 0) {
 		/* disable pid tracing */
-		if (ftrace_pid_trace < 0)
+		if (!ftrace_pid_trace)
 			goto out;
-		ftrace_pid_trace = -1;
+
+		clear_ftrace_pid_task(&ftrace_pid_trace);
 
 	} else {
-		struct task_struct *p;
-		int found = 0;
+		pid = find_get_pid(val);
 
-		if (ftrace_pid_trace == val)
+		if (pid == ftrace_pid_trace) {
+			put_pid(pid);
 			goto out;
-
-		/*
-		 * Find the task that matches this pid.
-		 * TODO: use pid namespaces instead.
-		 */
-		rcu_read_lock();
-		for_each_process(p) {
-			if (p->pid == val) {
-				found = 1;
-				set_tsk_trace_trace(p);
-			} else if (test_tsk_trace_trace(p))
-				clear_tsk_trace_trace(p);
 		}
-		rcu_read_unlock();
 
-		if (found)
-			ftrace_pid_trace = val;
-		else {
-			if (ftrace_pid_trace < 0)
-				goto out;
-			ftrace_pid_trace = -1;
-		}
+		if (ftrace_pid_trace)
+			clear_ftrace_pid_task(&ftrace_pid_trace);
+
+		if (!pid)
+			goto out;
+
+		ftrace_pid_trace = pid;
+
+		set_ftrace_pid_task(ftrace_pid_trace);
 	}
kernel/trace/ftrace.c +0 -0
 	/* update the function call */
kernel/trace/trace.h +2 -2
index 95fff37..8b81b4d 100644
@@ -541,11 +541,11 @@ print_graph_function(struct trace_iterator *iter)
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
-extern int ftrace_pid_trace;
+extern struct pid *ftrace_pid_trace;
 
 static inline int ftrace_trace_task(struct task_struct *task)
 {
-	if (ftrace_pid_trace < 0)
+	if (ftrace_pid_trace)
 		return 1;
 
 	return test_tsk_trace_trace(task);
Signature · 3 lines
1.5.6.5

--
#4 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

Quoted 10 lines · Eric W. Biederman

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1692

Quoted 5 lines
kernel/trace/ftrace.c +4 -0
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+     struct task_struct *p;
+

        rcu_read_lock();

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1695

Quoted 3 lines
kernel/trace/ftrace.c +3 -0
+     do_each_pid_task(*pid, PIDTYPE_PID, p) {
+             clear_tsk_trace_trace(p);
+     } while_each_pid_task(*pid, PIDTYPE_PID, p);
Quoted 6 lines

rcu_read_unlock()

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1699

Quoted 4 lines
kernel/trace/ftrace.c +4 -0
+     put_pid(*pid);
+
+     *pid = NULL;
+}

Could we get away with sticking the rcu_read_{un}lock() inside those macros? Those are going to get used in pretty high level code and we're allowed to nest rcu_read_lock(). No danger of deadlocks or lock inversions.

-- Dave

#5 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

Quoted 13 lines · Dave Hansen
Quoted 12 lines · Eric W. Biederman

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1692

Quoted 5 lines
kernel/trace/ftrace.c +4 -0
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+     struct task_struct *p;
+

        rcu_read_lock();

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1695

Quoted 3 lines
kernel/trace/ftrace.c +3 -0
+     do_each_pid_task(*pid, PIDTYPE_PID, p) {
+             clear_tsk_trace_trace(p);
+     } while_each_pid_task(*pid, PIDTYPE_PID, p);

rcu_read_unlock()

Quoted 9 lines
Quoted 4 lines

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1699

Quoted 4 lines
kernel/trace/ftrace.c +4 -0
+     put_pid(*pid);
+
+     *pid = NULL;
+}

Could we get away with sticking the rcu_read_{un}lock() inside those macros? Those are going to get used in pretty high level code and we're allowed to nest rcu_read_lock(). No danger of deadlocks or lock inversions.

Why don't any of the other users of do_each_pid_task() use rcu_read_lock()? They all seem to be under read_lock(&tasklist_lock) (except one is under a write lock of the same).

-- Dave

#6 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

Quoted 24 lines · Dave Hansen
Quoted 13 lines · Dave Hansen
Quoted 12 lines · Eric W. Biederman

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1692

Quoted 5 lines
kernel/trace/ftrace.c +4 -0
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+     struct task_struct *p;
+

        rcu_read_lock();

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1695

Quoted 3 lines
kernel/trace/ftrace.c +3 -0
+     do_each_pid_task(*pid, PIDTYPE_PID, p) {
+             clear_tsk_trace_trace(p);
+     } while_each_pid_task(*pid, PIDTYPE_PID, p);

rcu_read_unlock()

Quoted 9 lines
Quoted 4 lines

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1699

Quoted 4 lines
kernel/trace/ftrace.c +4 -0
+     put_pid(*pid);
+
+     *pid = NULL;
+}

Could we get away with sticking the rcu_read_{un}lock() inside those macros? Those are going to get used in pretty high level code and we're allowed to nest rcu_read_lock(). No danger of deadlocks or lock inversions.

Quoted 3 lines

Why don't any of the other users of do_each_pid_task() use rcu_read_lock()? They all seem to be under read_lock(&tasklist_lock) (except one is under a write lock of the same).

We probably should. Historically read_lock(&tasklist_lock) implies rcu_read_lock(). And the tasklist lock is what we hold when it is safe.

But if you look at find_vpid we should be holding just the rcu lock there.

Eric

#7 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

Quoted 25 lines · Eric W. Biederman
Quoted 24 lines · Dave Hansen
Quoted 22 lines · Dave Hansen
Quoted 10 lines · Eric W. Biederman

Review

Quoted 5 lines
(quoted diff) +4 -0
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+     struct task_struct *p;
+

        rcu_read_lock();

Review

Quoted 3 lines
(quoted diff) +3 -0
+     do_each_pid_task(*pid, PIDTYPE_PID, p) {
+             clear_tsk_trace_trace(p);
+     } while_each_pid_task(*pid, PIDTYPE_PID, p);
Quoted 6 lines

rcu_read_unlock()

Review

Quoted 4 lines
(quoted diff) +4 -0
+     put_pid(*pid);
+
+     *pid = NULL;
+}

Could we get away with sticking the rcu_read_{un}lock() inside those macros? Those are going to get used in pretty high level code and we're allowed to nest rcu_read_lock(). No danger of deadlocks or lock inversions.

Quoted 6 lines
Quoted 3 lines

Why don't any of the other users of do_each_pid_task() use rcu_read_lock()? They all seem to be under read_lock(&tasklist_lock) (except one is under a write lock of the same).

We probably should. Historically read_lock(&tasklist_lock) implies rcu_read_lock().

You mean because the current task can't go through a quiescent period until it hits userspace, and we can't go to userspace while holding read_lock()? Nah, that's not subtle. ;)

Quoted 3 lines

And the tasklist lock is what we hold when it is safe.

But if you look at find_vpid we should be holding just the rcu lock there.

Yup, I see it there.

So, any reason not to do this? Brown-bag compile tested.

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/include/linux/pid.h |    2 ++
 1 file changed, 2 insertions(+)
include/linux/pid.h renamed from include/linux/pid.h~put-rcu-ops-in-do_each_pid_task +2 -0
diff -puN include/linux/pid.h~put-rcu-ops-in-do_each_pid_task include/linux/pid.h
@@ -147,6 +147,7 @@ pid_t pid_vnr(struct pid *pid);
 #define do_each_pid_task(pid, type, task)				\
 	do {								\
 		struct hlist_node *pos___;				\
+		rcu_read_lock();					\
 		if (pid != NULL)					\
 			hlist_for_each_entry_rcu((task), pos___,	\
 				&pid->tasks[type], pids[type].node) {

@@ -159,6 +160,7 @@ pid_t pid_vnr(struct pid *pid);
 				if (type == PIDTYPE_PID)		\
 					break;				\
 			}						\
+		rcu_read_unlock();					\
 	} while (0)
 
 #define do_each_pid_thread(pid, type, task)				\

_

-- Dave

author maintainer
#8 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

Quoted 10 lines · Dave Hansen
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines

Could we get away with sticking the rcu_read_{un}lock() inside those macros? Those are going to get used in pretty high level code and we're allowed to nest rcu_read_lock(). No danger of deadlocks or lock inversions.

Why don't any of the other users of do_each_pid_task() use rcu_read_lock()? They all seem to be under read_lock(&tasklist_lock) (except one is under a write lock of the same).

Quoted 6 lines
Quoted 2 lines

We probably should. Historically read_lock(&tasklist_lock) implies rcu_read_lock().

You mean because the current task can't go through a quiescent period until it hits userspace, and we can't go to userspace while holding read_lock()? Nah, that's not subtle. ;)

Has nothing to do with userspace. We can not go through a quiescent period while holding a rcu_read_lock, or if preemption is disabled. read_lock prevents preemption, as does spin_locks.

Quoted 28 lines
Quoted 4 lines

And the tasklist lock is what we hold when it is safe.

But if you look at find_vpid we should be holding just the rcu lock there.

Yup, I see it there.

So, any reason not to do this? Brown-bag compile tested.

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/include/linux/pid.h |    2 ++
 1 file changed, 2 insertions(+)

diff -puN include/linux/pid.h~put-rcu-ops-in-do_each_pid_task include/linux/pid.h

include/linux/pid.h renamed from include/linux/pid.h~put-rcu-ops-in-do_each_pid_task +1 -0
@@ -147,6 +147,7 @@ pid_t pid_vnr(struct pid *pid);
 #define do_each_pid_task(pid, type, task)				\
 	do {								\
 		struct hlist_node *pos___;				\
+		rcu_read_lock();					\
 		if (pid != NULL)					\
 			hlist_for_each_entry_rcu((task), pos___,	\
 				&pid->tasks[type], pids[type].node) {

include/linux/pid.h pid_t pid_vnr(struct pid *pid); · line 164

Quoted 5 lines
include/linux/pid.h renamed from include/linux/pid.h~put-rcu-ops-in-do_each_pid_task +1 -0
@@ -159,6 +160,7 @@ pid_t pid_vnr(struct pid *pid);
 				if (type == PIDTYPE_PID)		\
 					break;				\
 			}						\
+		rcu_read_unlock();					\
 	} while (0)

That probably could work.

-- Steve

#9 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

In reply to Eric W. Biederman
Quoted 27 lines · Eric W. Biederman
Quoted 12 lines · Dave Hansen
Quoted 11 lines · Dave Hansen
Quoted 10 lines · Eric W. Biederman

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1692

Quoted 5 lines
kernel/trace/ftrace.c +4 -0
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+     struct task_struct *p;
+

        rcu_read_lock();
Quoted 3 lines
kernel/trace/ftrace.c +3 -0
+     do_each_pid_task(*pid, PIDTYPE_PID, p) {
+             clear_tsk_trace_trace(p);
+     } while_each_pid_task(*pid, PIDTYPE_PID, p);
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines

rcu_read_unlock()

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1699

Quoted 4 lines
kernel/trace/ftrace.c +4 -0
+     put_pid(*pid);
+
+     *pid = NULL;
+}

Could we get away with sticking the rcu_read_{un}lock() inside

those

Quoted 1 line
Quoted 1 line

macros? Those are going to get used in pretty high level code and

we're

Quoted 3 lines
Quoted 2 lines

allowed to nest rcu_read_lock(). No danger of deadlocks or lock inversions.

Quoted 8 lines
Quoted 2 lines

Why don't any of the other users of do_each_pid_task() use rcu_read_lock()? They all seem to be under

read_lock(&tasklist_lock)

Quoted 1 line

(except one is under a write lock of the same).

We probably should. Historically read_lock(&tasklist_lock) implies rcu_read_lock(). And the tasklist lock is what we hold when it is safe.

So, Dipankar tells me that you really do need rcu_read_lock/unlock() for the guarantee here; the tasklist lock is not sufficient. The realtime kernel will preempt even those sections covered by spinlocks.

-- Dave

author maintainer
#10 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

Quoted 8 lines · Dave Hansen
Quoted 4 lines

We probably should. Historically read_lock(&tasklist_lock) implies rcu_read_lock(). And the tasklist lock is what we hold when it is safe.

So, Dipankar tells me that you really do need rcu_read_lock/unlock() for the guarantee here; the tasklist lock is not sufficient. The realtime kernel will preempt even those sections covered by spinlocks.

Yes it will.

-- Steve

author maintainer
#11 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

In reply to Dave Hansen
Quoted 24 lines · Dave Hansen
Quoted 13 lines · Dave Hansen
Quoted 12 lines · Eric W. Biederman

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1692

Quoted 5 lines
kernel/trace/ftrace.c +4 -0
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+     struct task_struct *p;
+

        rcu_read_lock();

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1695

Quoted 3 lines
kernel/trace/ftrace.c +3 -0
+     do_each_pid_task(*pid, PIDTYPE_PID, p) {
+             clear_tsk_trace_trace(p);
+     } while_each_pid_task(*pid, PIDTYPE_PID, p);

rcu_read_unlock()

Quoted 9 lines
Quoted 4 lines

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1699

Quoted 4 lines
kernel/trace/ftrace.c +4 -0
+     put_pid(*pid);
+
+     *pid = NULL;
+}

Could we get away with sticking the rcu_read_{un}lock() inside those macros? Those are going to get used in pretty high level code and we're allowed to nest rcu_read_lock(). No danger of deadlocks or lock inversions.

Quoted 3 lines

Why don't any of the other users of do_each_pid_task() use rcu_read_lock()? They all seem to be under read_lock(&tasklist_lock) (except one is under a write lock of the same).

Well, if the pid hashes are traversal safe (rcu style), then we only worry about a node or task being freed. I'm assuming that the node is protected via RCU as tasks are, then using only rcu_read_lock should be sufficient.

-- Steve

#12 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

In reply to Dave Hansen
Quoted 24 lines · Dave Hansen
Quoted 13 lines · Dave Hansen
Quoted 12 lines · Eric W. Biederman

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1692

Quoted 5 lines
kernel/trace/ftrace.c +4 -0
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+     struct task_struct *p;
+

        rcu_read_lock();

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1695

Quoted 3 lines
kernel/trace/ftrace.c +3 -0
+     do_each_pid_task(*pid, PIDTYPE_PID, p) {
+             clear_tsk_trace_trace(p);
+     } while_each_pid_task(*pid, PIDTYPE_PID, p);

rcu_read_unlock()

Quoted 9 lines
Quoted 4 lines

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1699

Quoted 4 lines
kernel/trace/ftrace.c +4 -0
+     put_pid(*pid);
+
+     *pid = NULL;
+}

Could we get away with sticking the rcu_read_{un}lock() inside those macros? Those are going to get used in pretty high level code and we're allowed to nest rcu_read_lock(). No danger of deadlocks or lock inversions.

Quoted 3 lines

Why don't any of the other users of do_each_pid_task() use rcu_read_lock()? They all seem to be under read_lock(&tasklist_lock) (except one is under a write lock of the same).

The pid hash list is protected by tasklist_lock, right ? If so, holding read_lock(&tasklist_lock) will make this safe, you don't need rcu_read_lock/unlock(). This isn't a lock-free reader.

Thanks
Dipankar

#13 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

In reply to Steven Rostedt
Quoted 7 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: clean up

Eric Biederman suggested using the struct pid for filtering on pids in the kernel. This patch is based off of a demonstration of an implementation that Eric sent me in an email.

Please find_get_vpid and pid_vnr.

Eric

author maintainer
#14 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

Quoted 11 lines · Eric W. Biederman
Quoted 7 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: clean up

Eric Biederman suggested using the struct pid for filtering on pids in the kernel. This patch is based off of a demonstration of an implementation that Eric sent me in an email.

Please find_get_vpid and pid_vnr.

I could not find a "find_get_vpid".

-- Steve

#15 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

Quoted 8 lines · Steven Rostedt
Quoted 6 lines · Eric W. Biederman
Quoted 4 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: clean up

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Eric Biederman suggested using the struct pid for filtering on pids in the kernel. This patch is based off of a demonstration of an implementation that Eric sent me in an email.

Please find_get_vpid and pid_vnr.

I could not find a "find_get_vpid".

Doh. Grumble Pavel Grumble.

find_get_pid is the right one.

Sorry. We have a stupid inconsistency in the naming here, If we were consistent it would be find_get_vpid.

pid_vnr in that case then pid_vnr is definitely what you want to use when talking to user space.

Eric

#16 lore raw

Re: [PATCH 2/3] ftrace: use struct pid

| find_get_pid is the right one.
| 
| Sorry.  We have a stupid inconsistency in the naming here,
| If we were consistent it would be find_get_vpid.

Yes, its inconsistent, but maybe find_vpid() should be find_pid() since there is no 'virtual struct pid' only a virtual pid number.

Sukadev

author maintainer
#17 lore raw

PATCH 3/3 ftrace: add ability to only trace swapper tasks

In reply to Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: New feature

This patch lets the swapper tasks of all CPUS be filtered by the set_ftrace_pid file.

If '0' is echoed into this file, then all the idle tasks (aka swapper) is flagged to be traced. This affects all CPU idle tasks.

Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/ftrace.c |   74 +++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 63 insertions(+), 11 deletions(-)
kernel/trace/ftrace.c +63 -11
index 10b1d7c..eb57dc1 100644
@@ -49,6 +49,7 @@ static int last_ftrace_enabled;
 
 /* set when tracing only a pid */
 struct pid *ftrace_pid_trace;
+static struct pid * const ftrace_swapper_pid = (struct pid *)1;
 
 /* Quick disabling of function tracer. */
 int function_trace_stop;

@@ -1678,7 +1679,9 @@ ftrace_pid_read(struct file *file, char __user *ubuf,
 	char buf[64];
 	int r;
 
-	if (ftrace_pid_trace)
+	if (ftrace_pid_trace == ftrace_swapper_pid)
+		r = sprintf(buf, "swapper tasks\n");
+	else if (ftrace_pid_trace)
 		r = sprintf(buf, "%u\n", pid_nr(ftrace_pid_trace));
 	else
 		r = sprintf(buf, "no pid\n");

@@ -1686,19 +1689,43 @@ ftrace_pid_read(struct file *file, char __user *ubuf,
 	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
 }
 
-static void clear_ftrace_pid_task(struct pid **pid)
+static void clear_ftrace_swapper(void)
 {
 	struct task_struct *p;
+	int cpu;
 
-	do_each_pid_task(*pid, PIDTYPE_PID, p) {
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		p = idle_task(cpu);
 		clear_tsk_trace_trace(p);
-	} while_each_pid_task(*pid, PIDTYPE_PID, p);
-	put_pid(*pid);
+	}
+	put_online_cpus();
+}
 
-	*pid = NULL;
+static void set_ftrace_swapper(void)
+{
+	struct task_struct *p;
+	int cpu;
+
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		p = idle_task(cpu);
+		set_tsk_trace_trace(p);
+	}
+	put_online_cpus();
 }
 
-static void set_ftrace_pid_task(struct pid *pid)
+static void clear_ftrace_pid(struct pid *pid)
+{
+	struct task_struct *p;
+
+	do_each_pid_task(pid, PIDTYPE_PID, p) {
+		clear_tsk_trace_trace(p);
+	} while_each_pid_task(pid, PIDTYPE_PID, p);
+	put_pid(pid);
+}
+
+static void set_ftrace_pid(struct pid *pid)
 {
 	struct task_struct *p;
 

@@ -1707,6 +1734,24 @@ static void set_ftrace_pid_task(struct pid *pid)
 	} while_each_pid_task(pid, PIDTYPE_PID, p);
 }
 
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+	if (*pid == ftrace_swapper_pid)
+		clear_ftrace_swapper();
+	else
+		clear_ftrace_pid(*pid);
+
+	*pid = NULL;
+}
+
+static void set_ftrace_pid_task(struct pid *pid)
+{
+	if (pid == ftrace_swapper_pid)
+		set_ftrace_swapper();
+	else
+		set_ftrace_pid(pid);
+}
+
 static ssize_t
 ftrace_pid_write(struct file *filp, const char __user *ubuf,
 		   size_t cnt, loff_t *ppos)

@@ -1737,11 +1782,18 @@ ftrace_pid_write(struct file *filp, const char __user *ubuf,
 		clear_ftrace_pid_task(&ftrace_pid_trace);
 
 	} else {
-		pid = find_get_pid(val);
+		/* swapper task is special */
+		if (!val) {
+			pid = ftrace_swapper_pid;
+			if (pid == ftrace_pid_trace)
+				goto out;
+		} else {
+			pid = find_get_pid(val);
 
-		if (pid == ftrace_pid_trace) {
-			put_pid(pid);
-			goto out;
+			if (pid == ftrace_pid_trace) {
+				put_pid(pid);
+				goto out;
+			}
 		}
 
 		if (ftrace_pid_trace)
Signature · 3 lines
1.5.6.5

--
#18 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 3 lines · Steven Rostedt

/* set when tracing only a pid */
struct pid *ftrace_pid_trace;
+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

swapper pid is 1? not 0?

Liming Wang

#19 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 6 lines · Wang Liming
Quoted 3 lines · Steven Rostedt

/* set when tracing only a pid */
struct pid *ftrace_pid_trace;
+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

swapper pid is 1? not 0?

hmm.. I skip "struct pid", please ignore this mail.

Quoted 9 lines

Liming Wang

Signature · 4 lines
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
#20 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

In reply to Steven Rostedt
Quoted 7 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: New feature

This patch lets the swapper tasks of all CPUS be filtered by the set_ftrace_pid file.

Quoted 7 lines

If '0' is echoed into this file, then all the idle tasks (aka swapper) is flagged to be traced. This affects all CPU idle tasks.

Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/ftrace.c |   74 +++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 63 insertions(+), 11 deletions(-)

okay, i've applied it - but i dont like the extra complexity of +50 lines at all.

This is an area where the 'PID namespaces via struct pid pointers' model breaks down and forces collateral complexity into other subsystems, and where a simple integer based filter is so intuitive.

Eric, can you see any way to simplify this? It looks horrible.

Ingo

#21 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

In reply to Steven Rostedt
Quoted 6 lines · On Thu, 04 Dec 2008 00:26:41 -0500 Steven Rostedt <[email protected]> wrote:

From: Steven Rostedt <[email protected]>

Impact: New feature

This patch lets the swapper tasks of all CPUS be filtered by the set_ftrace_pid file.

"filtered" is one of those nasty words. It is unclear whether the filteree is included or excluded.

Quoted 3 lines

If '0' is echoed into this file, then all the idle tasks (aka swapper) is flagged to be traced. This affects all CPU idle tasks.

s/is/are/

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

What does this patch actually do? Is swapper currently excluded from tracing for undisclosed reasons and this patch permits it to be traced? If so, why was swapper thus excluded? Or am I totally off track?

kernel/trace/ftrace.c static int last_ftrace_enabled; · line 52

Quoted 13 lines
---
 kernel/trace/ftrace.c |   74 +++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 63 insertions(+), 11 deletions(-)
kernel/trace/ftrace.c +1 -0
index 10b1d7c..eb57dc1 100644
@@ -49,6 +49,7 @@ static int last_ftrace_enabled;
 
 /* set when tracing only a pid */
 struct pid *ftrace_pid_trace;
+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

eh?

Quoted 48 lines
kernel/trace/ftrace.c +22 -6
 /* Quick disabling of function tracer. */
 int function_trace_stop;

@@ -1678,7 +1679,9 @@ ftrace_pid_read(struct file *file, char __user *ubuf,
 	char buf[64];
 	int r;
 
-	if (ftrace_pid_trace)
+	if (ftrace_pid_trace == ftrace_swapper_pid)
+		r = sprintf(buf, "swapper tasks\n");
+	else if (ftrace_pid_trace)
 		r = sprintf(buf, "%u\n", pid_nr(ftrace_pid_trace));
 	else
 		r = sprintf(buf, "no pid\n");

@@ -1686,19 +1689,43 @@ ftrace_pid_read(struct file *file, char __user *ubuf,
 	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
 }
 
-static void clear_ftrace_pid_task(struct pid **pid)
+static void clear_ftrace_swapper(void)
 {
 	struct task_struct *p;
+	int cpu;
 
-	do_each_pid_task(*pid, PIDTYPE_PID, p) {
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		p = idle_task(cpu);
 		clear_tsk_trace_trace(p);
-	} while_each_pid_task(*pid, PIDTYPE_PID, p);
-	put_pid(*pid);
+	}
+	put_online_cpus();
+}
 
-	*pid = NULL;
+static void set_ftrace_swapper(void)
+{
+	struct task_struct *p;
+	int cpu;
+
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		p = idle_task(cpu);
+		set_tsk_trace_trace(p);
+	}
+	put_online_cpus();
 }

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1726

Quoted 10 lines
kernel/trace/ftrace.c +9 -1
-static void set_ftrace_pid_task(struct pid *pid)
+static void clear_ftrace_pid(struct pid *pid)
+{
+	struct task_struct *p;
+
+	do_each_pid_task(pid, PIDTYPE_PID, p) {
+		clear_tsk_trace_trace(p);
+	} while_each_pid_task(pid, PIDTYPE_PID, p);
+	put_pid(pid);
+}

What locking does this traversal need, and does this function have it?

Quoted 52 lines
kernel/trace/ftrace.c +30 -4
+static void set_ftrace_pid(struct pid *pid)
 {
 	struct task_struct *p;
 

@@ -1707,6 +1734,24 @@ static void set_ftrace_pid_task(struct pid *pid)
 	} while_each_pid_task(pid, PIDTYPE_PID, p);
 }
 
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+	if (*pid == ftrace_swapper_pid)
+		clear_ftrace_swapper();
+	else
+		clear_ftrace_pid(*pid);
+
+	*pid = NULL;
+}
+
+static void set_ftrace_pid_task(struct pid *pid)
+{
+	if (pid == ftrace_swapper_pid)
+		set_ftrace_swapper();
+	else
+		set_ftrace_pid(pid);
+}
+
 static ssize_t
 ftrace_pid_write(struct file *filp, const char __user *ubuf,
 		   size_t cnt, loff_t *ppos)

@@ -1737,11 +1782,18 @@ ftrace_pid_write(struct file *filp, const char __user *ubuf,
 		clear_ftrace_pid_task(&ftrace_pid_trace);
 
 	} else {
-		pid = find_get_pid(val);
+		/* swapper task is special */
+		if (!val) {
+			pid = ftrace_swapper_pid;
+			if (pid == ftrace_pid_trace)
+				goto out;
+		} else {
+			pid = find_get_pid(val);
 
-		if (pid == ftrace_pid_trace) {
-			put_pid(pid);
-			goto out;
+			if (pid == ftrace_pid_trace) {
+				put_pid(pid);
+				goto out;
+			}
 		}
 
 		if (ftrace_pid_trace)
#22 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 9 lines · Andrew Morton
Quoted 1 line
Signed-off-by Steven Rostedt <[email protected]>

What does this patch actually do? Is swapper currently excluded from tracing for undisclosed reasons and this patch permits it to be traced? If so, why was swapper thus excluded? Or am I totally off track?

Quoted 1 line

+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

eh?

all side-effects of getting rid of the integer based PID namespace and replacing them with struct pid pointers.

Ingo

#23 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Andrew Morton
Quoted 1 line
Signed-off-by Steven Rostedt <[email protected]>

What does this patch actually do? Is swapper currently excluded from tracing for undisclosed reasons and this patch permits it to be traced? If so, why was swapper thus excluded? Or am I totally off track?

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

eh?

all side-effects of getting rid of the integer based PID namespace and replacing them with struct pid pointers.

Thanks for asking Andrew it looks like an unnecessary side effect.

Eric

author maintainer
#24 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 10 lines · Eric W. Biederman
Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Andrew Morton
Quoted 1 line
Signed-off-by Steven Rostedt <[email protected]>

What does this patch actually do? Is swapper currently excluded from tracing for undisclosed reasons and this patch permits it to be traced? If so, why was swapper thus excluded? Or am I totally off track?

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

eh?

all side-effects of getting rid of the integer based PID namespace and replacing them with struct pid pointers.

Thanks for asking Andrew it looks like an unnecessary side effect.

Well, it was necessary without hacking fork.c ;-)

-- Steve

#25 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 14 lines · Steven Rostedt
Quoted 12 lines · Eric W. Biederman
Quoted 10 lines · Ingo Molnar
Quoted 8 lines · Andrew Morton
Quoted 1 line
Signed-off-by Steven Rostedt <[email protected]>

What does this patch actually do? Is swapper currently excluded from tracing for undisclosed reasons and this patch permits it to be traced? If so, why was swapper thus excluded? Or am I totally off track?

Quoted 1 line

+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line

eh?

all side-effects of getting rid of the integer based PID namespace and replacing them with struct pid pointers.

Thanks for asking Andrew it looks like an unnecessary side effect.

Well, it was necessary without hacking fork.c ;-)

The (struct pid *)1 has always been unnecessary.

As for fork. It would be nice to remove most of the special cases for the idle thread. At least the counts are significant. The rest is pretty much a don't care at this point.

Eric

author maintainer
#26 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 5 lines · Eric W. Biederman
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines

+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

eh?

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

all side-effects of getting rid of the integer based PID namespace and replacing them with struct pid pointers.

Thanks for asking Andrew it looks like an unnecessary side effect.

Well, it was necessary without hacking fork.c ;-)

The (struct pid *)1 has always been unnecessary.

Well, I could set it to the &init_struct_pid as you said, but it will not change any of the code below it. So it does not matter what ftrace_swapper_pid is set to, as long as it is not set to something that can be a legitimate pid struct for something not the swapper task.

It will only matter when we fix the fork code.

Quoted 4 lines

As for fork. It would be nice to remove most of the special cases for the idle thread. At least the counts are significant. The rest is pretty much a don't care at this point.

Well, the swapper task should still have a pid of zero. That is probably important.

-- Steve

#27 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 13 lines · Steven Rostedt
Quoted 12 lines · Eric W. Biederman
Quoted 11 lines
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

eh?

all side-effects of getting rid of the integer based PID namespace and replacing them with struct pid pointers.

Thanks for asking Andrew it looks like an unnecessary side effect.

Well, it was necessary without hacking fork.c ;-)

Quoted 8 lines
Quoted 1 line

The (struct pid *)1 has always been unnecessary.

Well, I could set it to the &init_struct_pid as you said, but it will not change any of the code below it. So it does not matter what ftrace_swapper_pid is set to, as long as it is not set to something that can be a legitimate pid struct for something not the swapper task.

It will only matter when we fix the fork code.

Well that and if someone dereferences.

Quoted 6 lines
Quoted 3 lines

As for fork. It would be nice to remove most of the special cases for the idle thread. At least the counts are significant. The rest is pretty much a don't care at this point.

Well, the swapper task should still have a pid of zero. That is probably important.

Right. I simply meant most of the
if (likely(p->pid)) conditional except for the counts is pretty much a don't care. Keeping the idle tasks off of the process list and out of the counts is useful.

For this particular case what problem did you see with calling attach_pid with PIDTYPE_PID on init_struct_pid?

Eric

author maintainer
#28 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 8 lines · Eric W. Biederman

Right. I simply meant most of the
if (likely(p->pid)) conditional except for the counts is pretty much a don't care. Keeping the idle tasks off of the process list and out of the counts is useful.

For this particular case what problem did you see with calling attach_pid with PIDTYPE_PID on init_struct_pid?

On boot up, the CPU 0 idle task is attached to init_struct_pid, and not the others. If you do a "attach_pid" on the next idle task that is created, it will become the attched process, bumping off CPU 0's idle task from the init_struct_pid.

When doing the code you suggested, I end up with only marking the last idle task to be created.

-- Steve

#29 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 7 lines · Steven Rostedt
Quoted 6 lines · Eric W. Biederman

Right. I simply meant most of the
if (likely(p->pid)) conditional except for the counts is pretty much a don't care. Keeping the idle tasks off of the process list and out of the counts is useful.

Quoted 7 lines
Quoted 2 lines

For this particular case what problem did you see with calling attach_pid with PIDTYPE_PID on init_struct_pid?

On boot up, the CPU 0 idle task is attached to init_struct_pid, and not the others. If you do a "attach_pid" on the next idle task that is created, it will become the attched process, bumping off CPU 0's idle task from the init_struct_pid.

It should form a linked list. For other pid types we don't have a problem.

Quoted 2 lines

When doing the code you suggested, I end up with only marking the last idle task to be created.

Odd. It is all a linked list through the task structures. I'm guessing the initialization isn't quite right.

Weird.

Eric

author maintainer
#30 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 12 lines · Eric W. Biederman
Quoted 10 lines · Steven Rostedt
Quoted 8 lines · Eric W. Biederman

Right. I simply meant most of the
if (likely(p->pid)) conditional except for the counts is pretty much a don't care. Keeping the idle tasks off of the process list and out of the counts is useful.

For this particular case what problem did you see with calling attach_pid with PIDTYPE_PID on init_struct_pid?

Quoted 6 lines
Quoted 4 lines

On boot up, the CPU 0 idle task is attached to init_struct_pid, and not the others. If you do a "attach_pid" on the next idle task that is created, it will become the attched process, bumping off CPU 0's idle task from the init_struct_pid.

It should form a linked list. For other pid types we don't have a problem.

Other pids get allocated per task. In the beginning of copy_process we have:

        if (pid != &init_struct_pid) {
                retval = -ENOMEM;
                pid = alloc_pid(task_active_pid_ns(p));

Where alloc_pid allocates a pid structure. But this is only done if it is not a swapper task.

Quoted 8 lines
Quoted 3 lines

When doing the code you suggested, I end up with only marking the last idle task to be created.

Odd. It is all a linked list through the task structures. I'm guessing the initialization isn't quite right.

Weird.

Do I need to change the loop to do_each_pid_thread?

I'll try that later today.

-- Steve

#31 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 9 lines · Steven Rostedt

Other pids get allocated per task. In the beginning of copy_process we have:

        if (pid != &init_struct_pid) {
                retval = -ENOMEM;
                pid = alloc_pid(task_active_pid_ns(p));

Where alloc_pid allocates a pid structure. But this is only done if it is not a swapper task.

For PIDTYPE_PID. For sessions and process groups we don't always allocate them, and attach_pid is fine.

There is a bit of oddness in the pid freeing case, in using an unhashed pid, so cpu hotunplug might be a problem. But you certainly shouldn't be running into this.

Quoted 11 lines
Quoted 7 lines
Quoted 2 lines

When doing the code you suggested, I end up with only marking the last idle task to be created.

Odd. It is all a linked list through the task structures. I'm guessing the initialization isn't quite right.

Weird.

Do I need to change the loop to do_each_pid_thread?

I'll try that later today.

I don't think so. I'm pretty certain we aren't passing the necessary clone flags to make that case work for idle threads, and in fact we aren't even cloning from the idle task when we create additional idle threads so I don't see how do_each_pid_thread could work better.

That said do_each_pid_thread appears to be a proper superset of do_each_pid_task so it should not be harmful.

I am all for figuring out how to remove this special case if we can.

Eric

author maintainer
#32 lore raw

PATCH 1/1 ftrace: use init_struct_pid as swapper pid

In reply to Eric W. Biederman

Ingo,

Here's a little clean up patch.

Randy Dunlap should be happy that I changed my script to combine the header and patch in one email if I only have a single patch.

-- Steve

The following patch is in:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

branch: tip/devel

Steven Rostedt (1):
ftrace: use init_struct_pid as swapper pid

----

 kernel/trace/ftrace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
---------------------------
commit 954d6cd672e0e87219f0763f829345518647f293
Author: Steven Rostedt <[email protected]>
Date:   Thu Dec 4 23:19:12 2008 -0500

ftrace: use init_struct_pid as swapper pid

Impact: clean up

Using (struct pid *)-1 as the pointer for ftrace_swapper_pid is a little confusing for others. This patch uses the address of the actual init pid structure instead. This change is only for clarity. It does not affect the code itself. Hopefully soon the swapper tasks will all have their own pid structure and then we can clean up the code a bit more.

Signed-off-by Steven Rostedt <[email protected]>
kernel/trace/ftrace.c +1 -1
index d2b1565..2971fe4 100644
@@ -49,7 +49,7 @@ static int last_ftrace_enabled;
 
 /* set when tracing only a pid */
 struct pid *ftrace_pid_trace;
-static struct pid * const ftrace_swapper_pid = (struct pid *)1;
+static struct pid * const ftrace_swapper_pid = &init_struct_pid;
 
 /* Quick disabling of function tracer. */
 int function_trace_stop;
#33 lore raw

Re: [PATCH] ftrace: use init_struct_pid as swapper pid

Quoted 17 lines · Steven Rostedt

Ingo,

Here's a little clean up patch.

Randy Dunlap should be happy that I changed my script to combine the header and patch in one email if I only have a single patch.

-- Steve

The following patch is in:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

branch: tip/devel

Quoted 7 lines

Steven Rostedt (1):
ftrace: use init_struct_pid as swapper pid

----

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

---------------------------

pulled, thanks Steve!

Ingo

#34 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

In reply to Eric W. Biederman
Quoted 10 lines · Eric W. Biederman
Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Andrew Morton
Quoted 1 line
Signed-off-by Steven Rostedt <[email protected]>

What does this patch actually do? Is swapper currently excluded from tracing for undisclosed reasons and this patch permits it to be traced? If so, why was swapper thus excluded? Or am I totally off track?

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

eh?

all side-effects of getting rid of the integer based PID namespace and replacing them with struct pid pointers.

Thanks for asking Andrew it looks like an unnecessary side effect.

Will wait for how the end result looks like ;-)

Ingo

author maintainer
#35 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 11 lines · Ingo Molnar
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

eh?

all side-effects of getting rid of the integer based PID namespace and replacing them with struct pid pointers.

Thanks for asking Andrew it looks like an unnecessary side effect.

Will wait for how the end result looks like ;-)

I'm waiting on a fix for fork.c. I already wrote the code in ftrace when the fork code is fixed. It does clean it up nicely. But I do not know all the intricacies of the struct pid code to do the fork code correctly.

-- Steve

author maintainer
#36 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

In reply to Andrew Morton
Quoted 9 lines · Andrew Morton
Quoted 6 lines · On Thu, 04 Dec 2008 00:26:41 -0500 Steven Rostedt <[email protected]> wrote:

From: Steven Rostedt <[email protected]>

Impact: New feature

This patch lets the swapper tasks of all CPUS be filtered by the set_ftrace_pid file.

Quoted 8 lines

"filtered" is one of those nasty words. It is unclear whether the filteree is included or excluded.

Quoted 3 lines

If '0' is echoed into this file, then all the idle tasks (aka swapper) is flagged to be traced. This affects all CPU idle tasks.

s/is/are/

My English is much better before midnight. But warning, it is worse before my first coffee. ;-)

Quoted 6 lines
Quoted 2 lines
Signed-off-by Steven Rostedt <[email protected]>

What does this patch actually do? Is swapper currently excluded from tracing for undisclosed reasons and this patch permits it to be traced? If so, why was swapper thus excluded? Or am I totally off track?

Yes, it is excluded. For two reasons:

1) you can not get to the swapper task using struct pid
2) the swapper task is not part of the task link list. Well, the first swapper task may be, but not the ones for other CPUS.

In fork.c:

	if (likely(p->pid)) {
		[...]
		if (thread_group_leader(p)) {
			[...]
			list_add_tail_rcu(&p->tasks, &init_task.tasks);
Quoted 11 lines
Quoted 11 lines
---
 kernel/trace/ftrace.c |   74 +++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 63 insertions(+), 11 deletions(-)
kernel/trace/ftrace.c +0 -0
index 10b1d7c..eb57dc1 100644
@@ -49,6 +49,7 @@ static int last_ftrace_enabled;
Quoted 5 lines

kernel/trace/ftrace.c static int last_ftrace_enabled; · line 52

Quoted 3 lines
kernel/trace/ftrace.c +1 -0
 /* set when tracing only a pid */
 struct pid *ftrace_pid_trace;
+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

eh?

The swapper task has no pid structure, if we want to trace it, we need to find it outside the pid code. But other parts of ftrace use the ftrace_pid_trace to see if it it should only trace the tasks that have the trace bit set. We have:

	if (ftrace_pid_trace)
		/* only trace this task if it is marked to trace */

But, I get your point. That line deserves a comment ;-)

Quoted 49 lines
Quoted 49 lines
kernel/trace/ftrace.c +22 -6
 /* Quick disabling of function tracer. */
 int function_trace_stop;

@@ -1678,7 +1679,9 @@ ftrace_pid_read(struct file *file, char __user *ubuf,
 	char buf[64];
 	int r;
 
-	if (ftrace_pid_trace)
+	if (ftrace_pid_trace == ftrace_swapper_pid)
+		r = sprintf(buf, "swapper tasks\n");
+	else if (ftrace_pid_trace)
 		r = sprintf(buf, "%u\n", pid_nr(ftrace_pid_trace));
 	else
 		r = sprintf(buf, "no pid\n");

@@ -1686,19 +1689,43 @@ ftrace_pid_read(struct file *file, char __user *ubuf,
 	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
 }
 
-static void clear_ftrace_pid_task(struct pid **pid)
+static void clear_ftrace_swapper(void)
 {
 	struct task_struct *p;
+	int cpu;
 
-	do_each_pid_task(*pid, PIDTYPE_PID, p) {
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		p = idle_task(cpu);
 		clear_tsk_trace_trace(p);
-	} while_each_pid_task(*pid, PIDTYPE_PID, p);
-	put_pid(*pid);
+	}
+	put_online_cpus();
+}
 
-	*pid = NULL;
+static void set_ftrace_swapper(void)
+{
+	struct task_struct *p;
+	int cpu;
+
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		p = idle_task(cpu);
+		set_tsk_trace_trace(p);
+	}
+	put_online_cpus();
 }
Quoted 12 lines

kernel/trace/ftrace.c ftrace_pid_read(struct file *file, char __user *ubuf, · line 1726

Quoted 10 lines
kernel/trace/ftrace.c +9 -1
-static void set_ftrace_pid_task(struct pid *pid)
+static void clear_ftrace_pid(struct pid *pid)
+{
+	struct task_struct *p;
+
+	do_each_pid_task(pid, PIDTYPE_PID, p) {
+		clear_tsk_trace_trace(p);
+	} while_each_pid_task(pid, PIDTYPE_PID, p);
+	put_pid(pid);
+}

What locking does this traversal need, and does this function have it?

No idea, I only did what Eric suggested.

And people wonder why we still stick to "task->pid"

-- Steve

Quoted 56 lines
Quoted 53 lines
kernel/trace/ftrace.c +30 -4
+static void set_ftrace_pid(struct pid *pid)
 {
 	struct task_struct *p;
 

@@ -1707,6 +1734,24 @@ static void set_ftrace_pid_task(struct pid *pid)
 	} while_each_pid_task(pid, PIDTYPE_PID, p);
 }
 
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+	if (*pid == ftrace_swapper_pid)
+		clear_ftrace_swapper();
+	else
+		clear_ftrace_pid(*pid);
+
+	*pid = NULL;
+}
+
+static void set_ftrace_pid_task(struct pid *pid)
+{
+	if (pid == ftrace_swapper_pid)
+		set_ftrace_swapper();
+	else
+		set_ftrace_pid(pid);
+}
+
 static ssize_t
 ftrace_pid_write(struct file *filp, const char __user *ubuf,
 		   size_t cnt, loff_t *ppos)

@@ -1737,11 +1782,18 @@ ftrace_pid_write(struct file *filp, const char __user *ubuf,
 		clear_ftrace_pid_task(&ftrace_pid_trace);
 
 	} else {
-		pid = find_get_pid(val);
+		/* swapper task is special */
+		if (!val) {
+			pid = ftrace_swapper_pid;
+			if (pid == ftrace_pid_trace)
+				goto out;
+		} else {
+			pid = find_get_pid(val);
 
-		if (pid == ftrace_pid_trace) {
-			put_pid(pid);
-			goto out;
+			if (pid == ftrace_pid_trace) {
+				put_pid(pid);
+				goto out;
+			}
 		}
 
 		if (ftrace_pid_trace)
#37 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 57 lines · Steven Rostedt
Quoted 9 lines · Andrew Morton
Quoted 6 lines · On Thu, 04 Dec 2008 00:26:41 -0500 Steven Rostedt <[email protected]> wrote:

From: Steven Rostedt <[email protected]>

Impact: New feature

This patch lets the swapper tasks of all CPUS be filtered by the set_ftrace_pid file.

Quoted 8 lines

"filtered" is one of those nasty words. It is unclear whether the filteree is included or excluded.

Quoted 3 lines

If '0' is echoed into this file, then all the idle tasks (aka swapper) is flagged to be traced. This affects all CPU idle tasks.

s/is/are/

My English is much better before midnight. But warning, it is worse before my first coffee. ;-)

Quoted 6 lines
Quoted 2 lines
Signed-off-by Steven Rostedt <[email protected]>

What does this patch actually do? Is swapper currently excluded from tracing for undisclosed reasons and this patch permits it to be traced? If so, why was swapper thus excluded? Or am I totally off track?

Yes, it is excluded. For two reasons:

1) you can not get to the swapper task using struct pid
2) the swapper task is not part of the task link list. Well, the first swapper task may be, but not the ones for other CPUS.

In fork.c:

	if (likely(p->pid)) {
		[...]
		if (thread_group_leader(p)) {
			[...]
			list_add_tail_rcu(&p->tasks, &init_task.tasks);
Quoted 10 lines
Quoted 10 lines

---
kernel/trace/ftrace.c | 74 +++++++++++++++++++++++++++++++++++++++++-------

 1 files changed, 63 insertions(+), 11 deletions(-)
kernel/trace/ftrace.c +0 -0
index 10b1d7c..eb57dc1 100644
@@ -49,6 +49,7 @@ static int last_ftrace_enabled;
Quoted 7 lines
Quoted 5 lines

kernel/trace/ftrace.c static int last_ftrace_enabled; · line 52

Quoted 3 lines
kernel/trace/ftrace.c +1 -0
 /* set when tracing only a pid */
 struct pid *ftrace_pid_trace;
+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

eh?

The swapper task has no pid structure

It does have a pid structure it just isn't hashed.

Quoted 5 lines

, if we want to trace it, we
need to find it outside the pid code. But other parts of ftrace use the ftrace_pid_trace to see if it it should only trace the tasks that have the trace bit set. We have:

Quoted 8 lines
	if (ftrace_pid_trace)
		/* only trace this task if it is marked to trace */

But, I get your point. That line deserves a comment ;-)

Quoted 1 line

What locking does this traversal need, and does this function have it?

No idea, I only did what Eric suggested.

Sorry I'm tired going fast and was just sketching the highlights.

Quoted 1 line

And people wonder why we still stick to "task->pid"

Hey thanks for trying to figure it out.

Eric

#38 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

In reply to Steven Rostedt
Quoted 9 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Impact: New feature

This patch lets the swapper tasks of all CPUS be filtered by the set_ftrace_pid file.

If '0' is echoed into this file, then all the idle tasks (aka swapper) is flagged to be traced. This affects all CPU idle tasks.

Ok. Nits below.

Quoted 10 lines
Signed-off-by Steven Rostedt <[email protected]>
---
 kernel/trace/ftrace.c |   74 +++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 63 insertions(+), 11 deletions(-)
kernel/trace/ftrace.c +0 -0
index 10b1d7c..eb57dc1 100644

kernel/trace/ftrace.c static int last_ftrace_enabled; · line 52

Quoted 4 lines
kernel/trace/ftrace.c +1 -0
@@ -49,6 +49,7 @@ static int last_ftrace_enabled;
 
 /* set when tracing only a pid */
 struct pid *ftrace_pid_trace;
+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

The initializer should be spelled &init_struct_pid instead of (struct pid *)1;

Except for the special case of finding this unhashed pid, by making the attach_pid(p, PIDTYPE_PID, pid) unconditional in copy_process, you can make the rest of the special cases go away.

Eric

Quoted 117 lines
kernel/trace/ftrace.c +62 -11
 /* Quick disabling of function tracer. */
 int function_trace_stop;

@@ -1678,7 +1679,9 @@ ftrace_pid_read(struct file *file, char __user *ubuf,
 	char buf[64];
 	int r;
 
-	if (ftrace_pid_trace)
+	if (ftrace_pid_trace == ftrace_swapper_pid)
+		r = sprintf(buf, "swapper tasks\n");
+	else if (ftrace_pid_trace)
 		r = sprintf(buf, "%u\n", pid_nr(ftrace_pid_trace));
 	else
 		r = sprintf(buf, "no pid\n");

@@ -1686,19 +1689,43 @@ ftrace_pid_read(struct file *file, char __user *ubuf,
 	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
 }
 
-static void clear_ftrace_pid_task(struct pid **pid)
+static void clear_ftrace_swapper(void)
 {
 	struct task_struct *p;
+	int cpu;
 
-	do_each_pid_task(*pid, PIDTYPE_PID, p) {
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		p = idle_task(cpu);
 		clear_tsk_trace_trace(p);
-	} while_each_pid_task(*pid, PIDTYPE_PID, p);
-	put_pid(*pid);
+	}
+	put_online_cpus();
+}
 
-	*pid = NULL;
+static void set_ftrace_swapper(void)
+{
+	struct task_struct *p;
+	int cpu;
+
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		p = idle_task(cpu);
+		set_tsk_trace_trace(p);
+	}
+	put_online_cpus();
 }
 
-static void set_ftrace_pid_task(struct pid *pid)
+static void clear_ftrace_pid(struct pid *pid)
+{
+	struct task_struct *p;
+
+	do_each_pid_task(pid, PIDTYPE_PID, p) {
+		clear_tsk_trace_trace(p);
+	} while_each_pid_task(pid, PIDTYPE_PID, p);
+	put_pid(pid);
+}
+
+static void set_ftrace_pid(struct pid *pid)
 {
 	struct task_struct *p;
 

@@ -1707,6 +1734,24 @@ static void set_ftrace_pid_task(struct pid *pid)
 	} while_each_pid_task(pid, PIDTYPE_PID, p);
 }
 
+static void clear_ftrace_pid_task(struct pid **pid)
+{
+	if (*pid == ftrace_swapper_pid)
+		clear_ftrace_swapper();
+	else
+		clear_ftrace_pid(*pid);
+
+	*pid = NULL;
+}
+
+static void set_ftrace_pid_task(struct pid *pid)
+{
+	if (pid == ftrace_swapper_pid)
+		set_ftrace_swapper();
+	else
+		set_ftrace_pid(pid);
+}
+
 static ssize_t
 ftrace_pid_write(struct file *filp, const char __user *ubuf,
 		   size_t cnt, loff_t *ppos)

@@ -1737,11 +1782,18 @@ ftrace_pid_write(struct file *filp, const char __user
 *ubuf,
 		clear_ftrace_pid_task(&ftrace_pid_trace);
 
 	} else {
-		pid = find_get_pid(val);
+		/* swapper task is special */
+		if (!val) {
+			pid = ftrace_swapper_pid;
+			if (pid == ftrace_pid_trace)
+				goto out;
+		} else {
+			pid = find_get_pid(val);
 
-		if (pid == ftrace_pid_trace) {
-			put_pid(pid);
-			goto out;
+			if (pid == ftrace_pid_trace) {
+				put_pid(pid);
+				goto out;
+			}
 		}

if (ftrace_pid_trace)

Signature · 3 lines
1.5.6.5

--
author maintainer
#39 lore raw

Re: [PATCH 3/3] ftrace: add ability to only trace swapper tasks

Quoted 14 lines · Eric W. Biederman

kernel/trace/ftrace.c static int last_ftrace_enabled; · line 52

Quoted 10 lines · Steven Rostedt
kernel/trace/ftrace.c +1 -0
index 10b1d7c..eb57dc1 100644
@@ -49,6 +49,7 @@ static int last_ftrace_enabled;
 
 /* set when tracing only a pid */
 struct pid *ftrace_pid_trace;
+static struct pid * const ftrace_swapper_pid = (struct pid *)1;

The initializer should be spelled &init_struct_pid instead of (struct pid *)1;

Quoted 3 lines

Except for the special case of finding this unhashed pid, by making the attach_pid(p, PIDTYPE_PID, pid) unconditional in copy_process, you can make the rest of the special cases go away.

Not that easy. It turns out that all idle tasks share the init_struct_pid, and if you add the attach_pid() outside the if statement, you only change the owner of the init_struct_pid to the last CPU to come on line.

I tried to allocate the pid, but by doing that it seems that the alloc_pid code will assign a number other than 0. The changes needed to fork.c to make this work is quickly going beyond a trivial fix, where I do not want to become a pid container developer in order to implement it.

-- Steve

#40 lore raw
In reply to Steven Rostedt
Quoted 17 lines · Steven Rostedt

Ingo,

The following patches are to have ftrace use the pid struct for filtering on pids.

The following patches are in:

git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

branch: tip/devel

Steven Rostedt (3):
fix the do_each_pid_task macro
ftrace: use struct pid
ftrace: add ability to only trace swapper tasks

Quoted 5 lines

----

 include/linux/pid.h   |    4 +-
 kernel/trace/ftrace.c |  128 ++++++++++++++++++++++++++++++++++++------------
 kernel/trace/trace.h  |    4 +-
 3 files changed, 100 insertions(+), 36 deletions(-)

pulled, thanks Steve!

Ingo

#41 lore raw

PATCH 1/1 tracing: fix typo

small fixlet below.

Ingo

-------------->
From a3c1b9d13bfb96036face4ed424c604c432db393 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Thu, 4 Dec 2008 09:18:28 +0100
Subject: [PATCH] tracing: fix typo

Impact: fix build bug

Signed-off-by Ingo Molnar <[email protected]>
---
 kernel/trace/trace.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/trace/trace.h +1 -1
index 8b81b4d..96202b3 100644
@@ -529,7 +529,7 @@ static inline int ftrace_graph_addr(unsigned long addr)
 #else
 static inline int ftrace_trace_addr(unsigned long addr)
 {
-	return 1
+	return 1;
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
#42 lore raw

PATCH 1/1 tracing: fix typo and missing inline function

Quoted 1 line · Ingo Molnar

small fixlet below.

updated.

--------------->
From 6b2539302bee8e88c99e3c7d80c16a04dbe5e2ad Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Thu, 4 Dec 2008 09:18:28 +0100
Subject: [PATCH] tracing: fix typo and missing inline function

Impact: fix build bugs

Signed-off-by Ingo Molnar <[email protected]>
---
 kernel/trace/trace.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
kernel/trace/trace.h +5 -1
index 8b81b4d..b4b7b73 100644
@@ -529,7 +529,11 @@ static inline int ftrace_graph_addr(unsigned long addr)
 #else
 static inline int ftrace_trace_addr(unsigned long addr)
 {
-	return 1
+	return 1;
+}
+static inline int ftrace_graph_addr(unsigned long addr)
+{
+	return 1;
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
author maintainer
#43 lore raw

Re: [PATCH] tracing: fix typo and missing inline function

Quoted 26 lines · Ingo Molnar
Quoted 1 line · Ingo Molnar

small fixlet below.

updated.

--------------->

Quoted 1 line

From 6b2539302bee8e88c99e3c7d80c16a04dbe5e2ad Mon Sep 17 00:00:00 2001

From: Ingo Molnar <[email protected]>
Date: Thu, 4 Dec 2008 09:18:28 +0100
Subject: [PATCH] tracing: fix typo and missing inline function

Impact: fix build bugs

Signed-off-by Ingo Molnar <[email protected]>
---
 kernel/trace/trace.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
kernel/trace/trace.h +0 -0
index 8b81b4d..b4b7b73 100644
@@ -529,7 +529,11 @@ static inline int ftrace_graph_addr(unsigned long addr)
 #else

kernel/trace/trace.h static inline int ftrace_graph_addr(unsigned long addr) · line 538

Quoted 10 lines
kernel/trace/trace.h +5 -1
 static inline int ftrace_trace_addr(unsigned long addr)
 {
-	return 1
+	return 1;
+}
+static inline int ftrace_graph_addr(unsigned long addr)
+{
+	return 1;
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */

Thanks! I guess I need to test the "off" cases ;-)

-- Steve

#44 lore raw

Re: [PATCH] tracing: fix typo and missing inline function

Quoted 33 lines · Steven Rostedt
Quoted 30 lines · Ingo Molnar
Quoted 1 line · Ingo Molnar

small fixlet below.

updated.

--------------->

Quoted 1 line

From 6b2539302bee8e88c99e3c7d80c16a04dbe5e2ad Mon Sep 17 00:00:00 2001

From: Ingo Molnar <[email protected]>
Date: Thu, 4 Dec 2008 09:18:28 +0100
Subject: [PATCH] tracing: fix typo and missing inline function

Impact: fix build bugs

Signed-off-by Ingo Molnar <[email protected]>
---
 kernel/trace/trace.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
kernel/trace/trace.h +1 -1
index 8b81b4d..b4b7b73 100644
@@ -529,7 +529,11 @@ static inline int ftrace_graph_addr(unsigned long addr)
 #else
 static inline int ftrace_trace_addr(unsigned long addr)
 {
-	return 1
+	return 1;
Quoted 8 lines

kernel/trace/trace.h static inline int ftrace_graph_addr(unsigned long addr) · line 538

Quoted 6 lines
kernel/trace/trace.h +4 -0
+}
+static inline int ftrace_graph_addr(unsigned long addr)
+{
+	return 1;
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */

Thanks! I guess I need to test the "off" cases ;-)

As long as we are free of runtime bugs, we'll be just fine ;-)

Ingo