[PATCH 0/3] ftrace: clean ups for tip

[PATCH 0/3] ftrace: clean ups for tip

[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

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

[PATCH 1/3] fix the do_each_pid_task macro

[Steven Rostedt]

[-- Attachment #1: 0001-fix-the-do_each_pid_task-macro.patch --]
[-- Type: text/plain, Size: 964 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

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 <srostedt@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/pid.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/pid.h b/include/linux/pid.h
index d7e98ff..bb206c5 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -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
-- 
1.5.6.5

-- 

[PATCH 2/3] ftrace: use struct pid

[Steven Rostedt]

[-- Attachment #1: 0002-ftrace-use-struct-pid.patch --]
[-- Type: text/plain, Size: 4319 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

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 <srostedt@redhat.com>
---
 kernel/trace/ftrace.c |   76 ++++++++++++++++++++++++++++--------------------
 kernel/trace/trace.h  |    4 +-
 2 files changed, 46 insertions(+), 34 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 57592a9..10b1d7c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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 */
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 95fff37..8b81b4d 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -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);
-- 
1.5.6.5

-- 

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

[Steven Rostedt]

[-- Attachment #1: 0003-ftrace-add-ability-to-only-trace-swapper-tasks.patch --]
[-- Type: text/plain, Size: 3369 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

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 <srostedt@redhat.com>
---
 kernel/trace/ftrace.c |   74 +++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 63 insertions(+), 11 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 10b1d7c..eb57dc1 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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)
-- 
1.5.6.5

-- 

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

[Wang Liming]

Steven Rostedt wrote:
 > /* 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

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

[Wang Liming]

Wang Liming wrote:
> Steven Rostedt wrote:
>  > /* 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.


> 
> 
> Liming Wang
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> 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 <srostedt@redhat.com>
> ---
>  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

Re: [PATCH 0/3] ftrace: clean ups for tip

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 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(-)

pulled, thanks Steve!

	Ingo

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

[Andrew Morton]

On Thu, 04 Dec 2008 00:26:41 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <srostedt@redhat.com>
> 
> 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.

> 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/

> Signed-off-by: Steven Rostedt <srostedt@redhat.com>

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 |   74 +++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 63 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 10b1d7c..eb57dc1 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -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?

>  /* 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);
> +}

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

> +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)

[PATCH] tracing: fix typo

[Ingo Molnar]

small fixlet below.

	Ingo

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

Impact: fix build bug

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/trace/trace.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 8b81b4d..96202b3 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -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 */
 

[PATCH] tracing: fix typo and missing inline function

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> small fixlet below.

updated.

--------------->
>From 6b2539302bee8e88c99e3c7d80c16a04dbe5e2ad Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
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 <mingo@elte.hu>
---
 kernel/trace/trace.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 8b81b4d..b4b7b73 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -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 */
 

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

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> 
> 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?

> > +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

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

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> From: Steven Rostedt <srostedt@redhat.com>
>
> 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.

> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  kernel/trace/ftrace.c |   76 ++++++++++++++++++++++++++++--------------------
>  kernel/trace/trace.h  |    4 +-
>  2 files changed, 46 insertions(+), 34 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 57592a9..10b1d7c 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -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;
> +
	rcu_read_lock();

> +	do_each_pid_task(*pid, PIDTYPE_PID, p) {
> +		clear_tsk_trace_trace(p);
> +	} while_each_pid_task(*pid, PIDTYPE_PID, p);
	rcu_read_unlock()

> +	put_pid(*pid);
> +
> +	*pid = NULL;
> +}
> +
> +static void set_ftrace_pid_task(struct pid *pid)
> +{
> +	struct task_struct *p;
> +
	rcu_read_lock();
> +	do_each_pid_task(pid, PIDTYPE_PID, p) {
> +		set_tsk_trace_trace(p);
> +	} while_each_pid_task(pid, PIDTYPE_PID, p);
	rcu_read_unlock();
> +}
> +
>  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 */
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 95fff37..8b81b4d 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -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);
> -- 
> 1.5.6.5
>
> -- 

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

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> From: Steven Rostedt <srostedt@redhat.com>
>
> 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.

> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  kernel/trace/ftrace.c |   74 +++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 63 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 10b1d7c..eb57dc1 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -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

>  
>  /* 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)
> -- 
> 1.5.6.5
>
> -- 

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

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> From: Steven Rostedt <srostedt@redhat.com>
>
> 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

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

[Dave Hansen]

On Thu, 2008-12-04 at 04:42 -0800, Eric W. Biederman wrote:
> 
> > +static void clear_ftrace_pid_task(struct pid **pid)
> > +{
> > +     struct task_struct *p;
> > +
>         rcu_read_lock();
> 
> > +     do_each_pid_task(*pid, PIDTYPE_PID, p) {
> > +             clear_tsk_trace_trace(p);
> > +     } while_each_pid_task(*pid, PIDTYPE_PID, p);
>         rcu_read_unlock()
> 
> > +     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

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

[Eric W. Biederman]

Ingo Molnar <mingo@elte.hu> writes:

> * Andrew Morton <akpm@linux-foundation.org> wrote:
>
>> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
>> 
>> 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?
>
>> > +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

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

[Dave Hansen]

On Thu, 2008-12-04 at 04:56 -0800, Dave Hansen wrote:
> On Thu, 2008-12-04 at 04:42 -0800, Eric W. Biederman wrote:
> > 
> > > +static void clear_ftrace_pid_task(struct pid **pid)
> > > +{
> > > +     struct task_struct *p;
> > > +
> >         rcu_read_lock();
> > 
> > > +     do_each_pid_task(*pid, PIDTYPE_PID, p) {
> > > +             clear_tsk_trace_trace(p);
> > > +     } while_each_pid_task(*pid, PIDTYPE_PID, p);
> >         rcu_read_unlock()
> > 
> > > +     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

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Andrew Morton wrote:

> On Thu, 04 Dec 2008 00:26:41 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > 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.
> 
> > 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. ;-)

> 
> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> 
> 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);



> 
> > ---
> >  kernel/trace/ftrace.c |   74 +++++++++++++++++++++++++++++++++++++++++-------
> >  1 files changed, 63 insertions(+), 11 deletions(-)
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 10b1d7c..eb57dc1 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -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?

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 ;-)


> 
> >  /* 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);
> > +}
> 
> 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

> 
> > +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)
> 
> 
> 

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Ingo Molnar wrote:

> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > small fixlet below.
> 
> updated.
> 
> --------------->
> >From 6b2539302bee8e88c99e3c7d80c16a04dbe5e2ad Mon Sep 17 00:00:00 2001
> From: Ingo Molnar <mingo@elte.hu>
> 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 <mingo@elte.hu>
> ---
>  kernel/trace/trace.h |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 8b81b4d..b4b7b73 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -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 */

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

-- Steve

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

[Eric W. Biederman]

Dave Hansen <dave@linux.vnet.ibm.com> writes:

> On Thu, 2008-12-04 at 04:56 -0800, Dave Hansen wrote:
>> On Thu, 2008-12-04 at 04:42 -0800, Eric W. Biederman wrote:
>> > 
>> > > +static void clear_ftrace_pid_task(struct pid **pid)
>> > > +{
>> > > +     struct task_struct *p;
>> > > +
>> >         rcu_read_lock();
>> > 
>> > > +     do_each_pid_task(*pid, PIDTYPE_PID, p) {
>> > > +             clear_tsk_trace_trace(p);
>> > > +     } while_each_pid_task(*pid, PIDTYPE_PID, p);
>> >         rcu_read_unlock()
>> > 
>> > > +     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).

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

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

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Thu, 4 Dec 2008, Andrew Morton wrote:
>
>> On Thu, 04 Dec 2008 00:26:41 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
>> 
>> > From: Steven Rostedt <srostedt@redhat.com>
>> > 
>> > 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.
>> 
>> > 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. ;-)
>
>> 
>> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
>> 
>> 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);
>
>
>

>> > ---
>> > kernel/trace/ftrace.c | 74 +++++++++++++++++++++++++++++++++++++++++-------
>> >  1 files changed, 63 insertions(+), 11 deletions(-)
>> > 
>> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>> > index 10b1d7c..eb57dc1 100644
>> > --- a/kernel/trace/ftrace.c
>> > +++ b/kernel/trace/ftrace.c
>> > @@ -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?
>
> The swapper task has no pid structure
It does have a pid structure it just isn't hashed.

>, 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 ;-)

>> 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.

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

Hey thanks for trying to figure it out.

Eric

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Eric W. Biederman wrote:

> Steven Rostedt <rostedt@goodmis.org> writes:
> 
> > From: Steven Rostedt <srostedt@redhat.com>
> >
> > 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

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Dave Hansen wrote:

> On Thu, 2008-12-04 at 04:56 -0800, Dave Hansen wrote:
> > On Thu, 2008-12-04 at 04:42 -0800, Eric W. Biederman wrote:
> > > 
> > > > +static void clear_ftrace_pid_task(struct pid **pid)
> > > > +{
> > > > +     struct task_struct *p;
> > > > +
> > >         rcu_read_lock();
> > > 
> > > > +     do_each_pid_task(*pid, PIDTYPE_PID, p) {
> > > > +             clear_tsk_trace_trace(p);
> > > > +     } while_each_pid_task(*pid, PIDTYPE_PID, p);
> > >         rcu_read_unlock()
> > > 
> > > > +     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).

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

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

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Thu, 4 Dec 2008, Eric W. Biederman wrote:
>
>> Steven Rostedt <rostedt@goodmis.org> writes:
>> 
>> > From: Steven Rostedt <srostedt@redhat.com>
>> >
>> > 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".

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

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Eric W. Biederman wrote:

> Ingo Molnar <mingo@elte.hu> writes:
> 
> > * Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> >> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> >> 
> >> 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?
> >
> >> > +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

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Eric W. Biederman wrote:

> Steven Rostedt <rostedt@goodmis.org> writes:
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 10b1d7c..eb57dc1 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -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.

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

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

[Dave Hansen]

On Thu, 2008-12-04 at 05:40 -0800, Eric W. Biederman wrote:
> Dave Hansen <dave@linux.vnet.ibm.com> writes:
> > On Thu, 2008-12-04 at 04:56 -0800, Dave Hansen wrote:
> >> On Thu, 2008-12-04 at 04:42 -0800, Eric W. Biederman wrote:
> >> > 
> >> > > +static void clear_ftrace_pid_task(struct pid **pid)
> >> > > +{
> >> > > +     struct task_struct *p;
> >> > > +
> >> >         rcu_read_lock();
> >> > 
> >> > > +     do_each_pid_task(*pid, PIDTYPE_PID, p) {
> >> > > +             clear_tsk_trace_trace(p);
> >> > > +     } while_each_pid_task(*pid, PIDTYPE_PID, p);
> >> >         rcu_read_unlock()
> >> > 
> >> > > +     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).
> 
> 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. ;)

> 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 <dave@linux.vnet.ibm.com>

---

 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
--- linux-2.6.git/include/linux/pid.h~put-rcu-ops-in-do_each_pid_task	2008-12-04 06:03:09.000000000 -0800
+++ linux-2.6.git-dave/include/linux/pid.h	2008-12-04 06:19:35.000000000 -0800
@@ -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

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

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Thu, 4 Dec 2008, Ingo Molnar wrote:
> 
> > 
> > * Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > > small fixlet below.
> > 
> > updated.
> > 
> > --------------->
> > >From 6b2539302bee8e88c99e3c7d80c16a04dbe5e2ad Mon Sep 17 00:00:00 2001
> > From: Ingo Molnar <mingo@elte.hu>
> > 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 <mingo@elte.hu>
> > ---
> >  kernel/trace/trace.h |    6 +++++-
> >  1 files changed, 5 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 8b81b4d..b4b7b73 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -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 */
> 
> 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

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Dave Hansen wrote:
> > >> 
> > >> 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).
> > 
> > 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.

> 
> > 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 <dave@linux.vnet.ibm.com>
> 
> ---
> 
>  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
> --- linux-2.6.git/include/linux/pid.h~put-rcu-ops-in-do_each_pid_task	2008-12-04 06:03:09.000000000 -0800
> +++ linux-2.6.git-dave/include/linux/pid.h	2008-12-04 06:19:35.000000000 -0800
> @@ -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)

That probably could work.

-- Steve

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

[Ingo Molnar]

* Eric W. Biederman <ebiederm@xmission.com> wrote:

> Ingo Molnar <mingo@elte.hu> writes:
> 
> > * Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> >> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> >> 
> >> 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?
> >
> >> > +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

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

[Dave Hansen]

On Thu, 2008-12-04 at 05:40 -0800, Eric W. Biederman wrote:
> Dave Hansen <dave@linux.vnet.ibm.com> writes:
> > On Thu, 2008-12-04 at 04:56 -0800, Dave Hansen wrote:
> >> On Thu, 2008-12-04 at 04:42 -0800, Eric W. Biederman wrote:
> >> > 
> >> > > +static void clear_ftrace_pid_task(struct pid **pid)
> >> > > +{
> >> > > +     struct task_struct *p;
> >> > > +
> >> >         rcu_read_lock();
> >> > 
> >> > > +     do_each_pid_task(*pid, PIDTYPE_PID, p) {
> >> > > +             clear_tsk_trace_trace(p);
> >> > > +     } while_each_pid_task(*pid, PIDTYPE_PID, p);
> >> >         rcu_read_unlock()
> >> > 
> >> > > +     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).
> 
> 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

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Dave Hansen wrote:
> > 
> > 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

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Ingo Molnar wrote:
> > >
> > >> > +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

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

[Sukadev Bhattiprolu]

| 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

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

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Thu, 4 Dec 2008, Eric W. Biederman wrote:
>
>> Ingo Molnar <mingo@elte.hu> writes:
>> 
>> > * Andrew Morton <akpm@linux-foundation.org> wrote:
>> >
>> >> > Signed-off-by: Steven Rostedt <srostedt@redhat.com>
>> >> 
>> >> 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?
>> >
>> >> > +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 ;-)

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

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Eric W. Biederman wrote:
> >> >
> >> >> > +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 ;-)
> 
> 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.

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

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

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Thu, 4 Dec 2008, Eric W. Biederman wrote:
>> >> >
>> >> >> > +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 ;-)
>> 
>> 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.  

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

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Eric W. Biederman wrote:
> 
> 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

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

[Dipankar Sarma]

On Thu, Dec 04, 2008 at 05:07:02AM -0800, Dave Hansen wrote:
> On Thu, 2008-12-04 at 04:56 -0800, Dave Hansen wrote:
> > On Thu, 2008-12-04 at 04:42 -0800, Eric W. Biederman wrote:
> > > 
> > > > +static void clear_ftrace_pid_task(struct pid **pid)
> > > > +{
> > > > +     struct task_struct *p;
> > > > +
> > >         rcu_read_lock();
> > > 
> > > > +     do_each_pid_task(*pid, PIDTYPE_PID, p) {
> > > > +             clear_tsk_trace_trace(p);
> > > > +     } while_each_pid_task(*pid, PIDTYPE_PID, p);
> > >         rcu_read_unlock()
> > > 
> > > > +     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).

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

[PATCH] ftrace: use init_struct_pid as swapper pid

[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


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 <srostedt@redhat.com>
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 <srostedt@redhat.com>

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index d2b1565..2971fe4 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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;

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

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Thu, 4 Dec 2008, Eric W. Biederman wrote:
>> 
>> 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.

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

> 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

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

[Steven Rostedt]

On Thu, 4 Dec 2008, Eric W. Biederman wrote:

> Steven Rostedt <rostedt@goodmis.org> writes:
> 
> > On Thu, 4 Dec 2008, Eric W. Biederman wrote:
> >> 
> >> 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.
> 
> 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.

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

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

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> 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(-)
> ---------------------------

pulled, thanks Steve!

	Ingo

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

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> 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.

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