LinuxLists.cc - [PATCH 3/3] ftrace: add ability to only trace swapper tasks

[permalink] [raw]

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

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

2008-12-04 08:19:21

by Andrew Morton

[permalink] [raw]

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

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.

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

2008-12-04 09:11:30

[permalink] [raw]

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

* Andrew Morton <[email protected]> wrote:

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

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

2008-12-04 12:56:47

[permalink] [raw]

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

Steven Rostedt <[email protected]> writes:

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

> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> 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
>
> --

2008-12-04 13:06:51

[permalink] [raw]

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

Ingo Molnar <[email protected]> writes:

> * Andrew Morton <[email protected]> wrote:
>
>> > 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?
>
>> > +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

2008-12-04 13:12:41

[permalink] [raw]

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

On Thu, 4 Dec 2008, Andrew Morton wrote:

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

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

2008-12-04 13:56:42

[permalink] [raw]

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

Steven Rostedt <[email protected]> writes:

> On Thu, 4 Dec 2008, Andrew Morton wrote:
>
>> 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.
>>
>> > 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 <[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);
>
>
>

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

2008-12-04 14:46:39

[permalink] [raw]

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

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

> Ingo Molnar <[email protected]> writes:
>
> > * Andrew Morton <[email protected]> wrote:
> >
> >> > 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?
> >
> >> > +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

2008-12-04 14:57:52

[permalink] [raw]

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

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

> Steven Rostedt <[email protected]> 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

2008-12-04 15:37:31

[permalink] [raw]

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

* Eric W. Biederman <[email protected]> wrote:

> Ingo Molnar <[email protected]> writes:
>
> > * Andrew Morton <[email protected]> wrote:
> >
> >> > 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?
> >
> >> > +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

2008-12-04 15:47:39

[permalink] [raw]

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

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

2008-12-04 20:46:41

[permalink] [raw]

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

Steven Rostedt <[email protected]> writes:

> On Thu, 4 Dec 2008, Eric W. Biederman wrote:
>
>> Ingo Molnar <[email protected]> writes:
>>
>> > * Andrew Morton <[email protected]> wrote:
>> >
>> >> > 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?
>> >
>> >> > +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

2008-12-04 20:58:00

[permalink] [raw]

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

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

2008-12-04 21:46:33

[permalink] [raw]

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

Steven Rostedt <[email protected]> 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

2008-12-04 21:56:53

[permalink] [raw]

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

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

2008-12-05 04:31:16

[permalink] [raw]

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

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

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;

2008-12-05 07:46:40

[permalink] [raw]

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

Steven Rostedt <[email protected]> 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

2008-12-05 12:23:19

[permalink] [raw]

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

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

> Steven Rostedt <[email protected]> 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

2008-12-05 13:52:27

[permalink] [raw]

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

* Steven Rostedt <[email protected]> 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

2008-12-05 16:36:41