Don't apply for write lock on tasklist_lock if parent doesn't ptrace other processes

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

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

---

diff -Nraup linux-2.6.35-rc5/kernel/ptrace.c linux-2.6.35-rc5_ptrace/kernel/ptrace.c

kernel/ptrace.c void exit_ptrace(struct task_struct *tra · line 339

Quoted 10 lines

kernel/ptrace.c +3 -0

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

Quoted 5 lines

Quoted 5 lines

(quoted diff) +0 -0

@@ -331,6 +331,9 @@ void exit_ptrace(struct task_struct *tra
 	struct task_struct *p, *n;
 	LIST_HEAD(ptrace_dead);
 

Quoted 6 lines

Review void exit_ptrace(struct task_struct *tra · line 339

Quoted 6 lines

(quoted diff) +3 -0

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

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.

/*
- * Detach all tasks we were using ptrace on.
+ * Detach all tasks we were using ptrace on. Called with tasklist held.

Quoted 4 lines

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.

That looks good to me, but it could use some more scare comments.

Quoted 3 lines

/*
- * Detach all tasks we were using ptrace on.
+ * Detach all tasks we were using ptrace on. Called with tasklist held.

* Called with tasklist held for writing, and returns with it held too.
* But note it can release and reacquire the lock.

kernel/exit.c static void forget_original_parent(struc · line 775

Quoted 2 lines

kernel/exit.c +2 -0

+	write_lock_irq(&tasklist_lock);
+

	/*
	 * Note that exit_ptrace() might drop tasklist_lock and reacquire it.
	 */

Quoted 1 line

exit_ptrace(father);

kernel/exit.c static void forget_original_parent(struc · line 778

Quoted 2 lines

kernel/exit.c +0 -1

-	write_lock_irq(&tasklist_lock);
 	reaper = find_new_reaper(father);

I am not surpized perf blaims tasklist, but I am really surpized this patch adds 10% improvement...

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

kernel/ptrace.c +0 -0

@@ -331,6 +331,9 @@ void exit_ptrace(struct task_struct *tra
 	struct task_struct *p, *n;
 	LIST_HEAD(ptrace_dead);
 

Quoted 9 lines

Quoted 6 lines

kernel/ptrace.c int ptrace_detach(struct task_struct *ch · line 338

Quoted 6 lines

kernel/ptrace.c +3 -0

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

kernel/exit.c +2 -1

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

Quoted 2 lines · Oleg Nesterov

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.

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?

Quoted 6 lines · Yanmin

Quoted 2 lines · Oleg Nesterov

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

Quoted 3 lines

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.

Quoted 2 lines

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

Quoted 6 lines · Oleg Nesterov

Quoted 4 lines · Yanmin

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.

Quoted 2 lines

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.

Quoted 1 line

Heh. We must optimize it. But it is not clear when ;)

Thanks. It's better to remove the big lock.

The only difference is that we are doing the function call + list_empty()
under tasklist, just a few instructions compared to "do a lot more work"
in forget_original_parent().

How this can make the 2% difference ? This looks like a noise to me, or do you think I missed something?

Quoted 8 lines · Yanmin

Quoted 6 lines · Oleg Nesterov

Quoted 4 lines · Yanmin

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.

Quoted 6 lines

Quoted 2 lines

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.

Zhang, thank you very much.

But. In this case I do not trust these results or I missed something. I mean, they do not look 100% accurate.

With your patch:

	forget_original_parent:

		exit_ptrace:
			if (list_empty(ptraced))
				return;


		write_lock_irq(tasklist);

		... do a lot more work ...

With my patch:

	forget_original_parent:

		write_lock_irq(tasklist);
	
		exit_ptrace:
			if (list_empty(ptraced))
				return;

		... do a lot more work ...

The only difference is that we are doing the function call + list_empty() under tasklist, just a few instructions compared to "do a lot more work" in forget_original_parent().

How this can make the 2% difference ?

This looks like a noise to me,
or do you think I missed something?

Quoted 3 lines

Quoted 2 lines

Heh. We must optimize it. But it is not clear when ;)

Thanks. It's better to remove the big lock.

Yes. The only problem this is very much nontrival with the current code.