Prev: Strange read data corruption on ext4/LVM/md
Next: Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5
From: David Rientjes on 15 Jul 2010 16:00
On Thu, 15 Jul 2010, Zhang, Yanmin wrote:

> We run some sub-cases (fork, exec, pipe, tcp, udp) of aim7 on 8-socket machine.
> Perf shows write_lock_irq(&tasklist_lock) consumes more than 50% cpu time.
>
> One hot caller is exit_ptrace. If the exiting process doesn't ptrace other
> processes, kernel needn't apply for the write lock on tasklist_lock.
>
> With below patch against kernel 2.6.35-rc5, we get more than 10% result improvement.
>
> Signed-off-by: Zhang Yanmin <yanmin_zhang(a)linux.intel.com>

Acked-by: David Rientjes <rientjes(a)google.com>

We're guarded against ptrace_attach() because tracer->exit_state is
non-zero at this point in the exit path.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 21 Jul 2010 18:00
On Thu, 15 Jul 2010 14:51:03 +0800
"Zhang, Yanmin" <yanmin_zhang(a)linux.intel.com> wrote:

> We run some sub-cases (fork, exec, pipe, tcp, udp) of aim7 on 8-socket machine.
> Perf shows write_lock_irq(&tasklist_lock) consumes more than 50% cpu time.
>
> One hot caller is exit_ptrace. If the exiting process doesn't ptrace other
> processes, kernel needn't apply for the write lock on tasklist_lock.
>
> With below patch against kernel 2.6.35-rc5, we get more than 10% result improvement.
>
> Signed-off-by: Zhang Yanmin <yanmin_zhang(a)linux.intel.com>
>
> ---
>
> diff -Nraup linux-2.6.35-rc5/kernel/ptrace.c linux-2.6.35-rc5_ptrace/kernel/ptrace.c
> --- linux-2.6.35-rc5/kernel/ptrace.c 2010-07-16 14:01:15.000000000 +0800
> +++ linux-2.6.35-rc5_ptrace/kernel/ptrace.c 2010-07-16 14:03:20.000000000 +0800
> @@ -331,6 +331,9 @@ void exit_ptrace(struct task_struct *tra
> struct task_struct *p, *n;
> LIST_HEAD(ptrace_dead);
>
> + if (list_empty(&tracer->ptraced))
> + return;
> +
> write_lock_irq(&tasklist_lock);
> list_for_each_entry_safe(p, n, &tracer->ptraced, ptrace_entry) {
> if (__ptrace_detach(tracer, p))

hah, nice patch - an easy 10%. I snuck a cc:stable into the changelog
in the hope that those guys mistake it for a bugfix ;)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Roland McGrath on 21 Jul 2010 18:30
> > @@ -331,6 +331,9 @@ void exit_ptrace(struct task_struct *tra
> > struct task_struct *p, *n;
> > LIST_HEAD(ptrace_dead);
> >
> > + if (list_empty(&tracer->ptraced))
> > + return;
> > +
> > write_lock_irq(&tasklist_lock);
> > list_for_each_entry_safe(p, n, &tracer->ptraced, ptrace_entry) {
> > if (__ptrace_detach(tracer, p))

I think we may have tried that before. Oleg can tell us if it's really
safe vs a race with PTRACE_TRACEME or something like that.


Thanks,
Roland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Zhang, Yanmin on 23 Jul 2010 04:50
On Thu, 2010-07-22 at 11:05 +0200, Oleg Nesterov wrote:
> I am not surpized perf blaims tasklist, but I am really surpized this patch
> adds 10% improvement...
I changed aim7 workfile to focus on fork/exec and other a couple of sub-cases.
And this behavior is clear on 8-socket machines.

>
> On 07/21, Roland McGrath wrote:
> >
> > > > @@ -331,6 +331,9 @@ void exit_ptrace(struct task_struct *tra
> > > > struct task_struct *p, *n;
> > > > LIST_HEAD(ptrace_dead);
> > > >
> > > > + if (list_empty(&tracer->ptraced))
> > > > + return;
> > > > +
> > > > write_lock_irq(&tasklist_lock);
> > > > list_for_each_entry_safe(p, n, &tracer->ptraced, ptrace_entry) {
> > > > if (__ptrace_detach(tracer, p))
> >
> > I think we may have tried that before. Oleg can tell us if it's really
> > safe vs a race with PTRACE_TRACEME or something like that.
>
> Yes, this can race with ptrace_traceme(). Without tasklist_lock in
> exit_ptrace(), it is possible that ptrace_traceme() starts __ptrace_link()
> before it sees PF_EXITING, and completes before the result of list_add()
> is visible to the exiting parent. tasklist acts as a barrier.
Thanks for your kind explanation.

>
> So, this list_empty() check needs taskslit at least for reading. But, we
> are going to take it for writing right after exit_ptrace() returns, afaics
> we can add this fastpatch check for free.
>
> Uncompiled/untested.
>
> Oleg.
>
> kernel/ptrace.c | 10 +++++++---
> kernel/exit.c | 3 ++-
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> --- x/kernel/ptrace.c
> +++ x/kernel/ptrace.c
> @@ -324,26 +324,30 @@ int ptrace_detach(struct task_struct *ch
> }
>
> /*
> - * Detach all tasks we were using ptrace on.
> + * Detach all tasks we were using ptrace on. Called with tasklist held.
> */
> void exit_ptrace(struct task_struct *tracer)
> {
> struct task_struct *p, *n;
> LIST_HEAD(ptrace_dead);
>
> - write_lock_irq(&tasklist_lock);
> + if (likely(list_empty(&tracer->ptraced)))
> + return;
> +
> list_for_each_entry_safe(p, n, &tracer->ptraced, ptrace_entry) {
> if (__ptrace_detach(tracer, p))
> list_add(&p->ptrace_entry, &ptrace_dead);
> }
> - write_unlock_irq(&tasklist_lock);
>
> + write_unlock_irq(&tasklist_lock);
> BUG_ON(!list_empty(&tracer->ptraced));
>
> list_for_each_entry_safe(p, n, &ptrace_dead, ptrace_entry) {
> list_del_init(&p->ptrace_entry);
> release_task(p);
> }
> +
> + write_lock_irq(&tasklist_lock);
> }
>
> int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len)
> --- x/kernel/exit.c
> +++ x/kernel/exit.c
> @@ -771,9 +771,10 @@ static void forget_original_parent(struc
After applying my patch (although it's incorrect as there is a race with TRACEME),
perf shows write_lock_irq in forget_original_parent consumes less than 40% cpu time on
8-socket machine.

Is it possible to optimize it to use finer locks instead of the global tasklist_lock?


> struct task_struct *p, *n, *reaper;
> LIST_HEAD(dead_children);
>
> + write_lock_irq(&tasklist_lock);
> +
> exit_ptrace(father);
>
> - write_lock_irq(&tasklist_lock);
> reaper = find_new_reaper(father);
>
> list_for_each_entry_safe(p, n, &father->children, sibling) {
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Zhang, Yanmin on 26 Jul 2010 01:10
On Fri, 2010-07-23 at 19:34 +0200, Oleg Nesterov wrote:
> On 07/23, Zhang, Yanmin wrote:
> >
> > On Thu, 2010-07-22 at 11:05 +0200, Oleg Nesterov wrote:
> > > I am not surpized perf blaims tasklist, but I am really surpized this patch
> > > adds 10% improvement...
> > I changed aim7 workfile to focus on fork/exec and other a couple of sub-cases.
> > And this behavior is clear on 8-socket machines.
>
> Thanks...
>
> > After applying my patch (although it's incorrect as there is a race with TRACEME),
> > perf shows write_lock_irq in forget_original_parent consumes less than 40% cpu time on
> > 8-socket machine.
>
> Any chance you can test the patch I sent? It should have the same effect,
> otherwise there is something interesting.
1) with my patch, we got about 13% improvement;
2) With your patch, we got about 11% improvement;

Performance is very sensitive to spinlock contention on large machines.

>
> > Is it possible to optimize it to use finer locks instead of the global tasklist_lock?
>
> Heh. We must optimize it. But it is not clear when ;)
Thanks. It's better to remove the big lock.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: Strange read data corruption on ext4/LVM/md
Next: Stop ARM boards crashing when CUPS is loaded - 2.6.35-rc5