Performance regression in 2.6.7-rc3

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all

The OSDL robot monkeys revealed a massive reproducible regression in the dbt3-pgsql benchmark which could be related to MBligh's measure regression.

2.6.7-rc2: http://khack.osdl.org/stp/293625/
Composite Query Processing Power Throughput Numerical Quantity
199.38 152.52 260.63

vs

2.6.7-rc3: http://khack.osdl.org/stp/293704/
Composite Query Processing Power Throughput Numerical Quantity
152.13 146.36 158.12

with a little bit of detective work and help from Wli we tracked down that this patch caused it:
[PATCH] sched: improve wakeup-affinity
A massive increase in idle time was observed and the throughput dropped by 40% Reversing this patch gave these results:

backsched1: http://khack.osdl.org/stp/293865/
Composite Query Processing Power Throughput Numerical Quantity
193.93 145.95 257.67

It may be best to reverse this patch until the regression is better understood.
Here is a patch reversing it:

Con
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFAyk4SZUg7+tp6mRURAk5RAKCKIxzmRS3u+gs8W5gVyGgvL6glUgCeNggF 8mBUT4HtP4g5d/MViraZ9ds=
=vj+s
-----END PGP SIGNATURE-----

@@ -770,8 +770,7 @@ static int try_to_wake_up(task_t * p, un
 		this_load -= SCHED_LOAD_SCALE;
 
 	/* Don't pull the task off an idle CPU to a busy one */
-	if (load < SCHED_LOAD_SCALE && load + this_load > SCHED_LOAD_SCALE
-			&& this_load > load)
+	if (load < SCHED_LOAD_SCALE/2 && this_load > SCHED_LOAD_SCALE/2)
 		goto out_set_cpu;
 
 	new_cpu = this_cpu; /* Wake to this CPU if we can */

@@ -1633,7 +1632,8 @@ nextgroup:
 	return busiest;
 
 out_balanced:
-	if (busiest && idle != NOT_IDLE && max_load > SCHED_LOAD_SCALE) {
+	if (busiest && (idle == NEWLY_IDLE ||
+			(idle == IDLE && max_load > SCHED_LOAD_SCALE)) ) {
 		*imbalance = 1;
 		return busiest;
 	}

OK thanks, I'm looking into it.

agreed. It is weird because Nick said that pgsql was tested with the patch - and we applied the patch based on those good results. Nick?

Anyway, does the patch below fix the pgsql problem? It reverts to the more agressive idle-balancing variant (which isnt strictly necessary for the bw_pipe problem).

Ingo

@@ -1625,8 +1626,7 @@
 	return busiest;
 
 out_balanced:
-	if (busiest && idle != NOT_IDLE && max_load > SCHED_LOAD_SCALE) {
+	if (busiest && (idle == NEWLY_IDLE ||
+			(idle == IDLE && max_load > SCHED_LOAD_SCALE)) ) {
 		*imbalance = 1;
 		return busiest;

}

Better than the patch backed out but still worse than it was before:
http://khack.osdl.org/stp/293958/
Composite Query Processing Power Throughput Numerical Quantity
161.42 152.90 170.41

Con

Well, I found where at least half my regression on kernel compiles came from. It's sched_balance_context. Which makes no sense to me ... given that it seems to switch on CLONE_VM, and otherwise make no changes. But I went and double-checked my results ... so ... confused.

The patch adds find_idlest_cpu and wake_up_forked_thread, but otherwise just seems to do:

- if (!(clone_flags & CLONE_STOPPED))
- wake_up_forked_process(p); /* do this last */
- else
+ if (!(clone_flags & CLONE_STOPPED)) {
+ /*

+                        * Do the wakeup last. On SMP we treat fork() and
+                        * CLONE_VM separately, because fork() has already
+                        * created cache footprint on this CPU (due to
+                        * copying the pagetables), hence migration would
+                        * probably be costy. Threads on the other hand
+                        * have less traction to the current CPU, and if
+                        * there's an imbalance then the scheduler can
+                        * migrate this fresh thread now, before it
+                        * accumulates a larger cache footprint:
+                        */

+ if (clone_flags & CLONE_VM)
+ wake_up_forked_thread(p);
+ else
+ wake_up_forked_process(p);
+ } else

                        p->state = TASK_STOPPED;
                ++total_forks;

How the hell can that have any effect on non-threaded workloads? Perhaps some part of kernel compile *is* multi-threaded. It does seem to get called somehow ... from the profile:

129 find_idlest_cpu
83 wake_up_forked_thread

Here's the diffprofile between the two:

      5835     4.0% total
      1100    27.1% __copy_from_user_ll
       627     3.6% do_anonymous_page
       363     6.8% page_add_rmap
       357    79.5% strnlen_user
       338    43.3% finish_task_switch
       308  3080.0% flush_signal_handlers
       272     7.0% zap_pte_range
       239     2.1% page_remove_rmap
       230    16.9% free_hot_cold_page
       224     9.7% buffered_rmqueue
       196    44.9% pte_alloc_one
       171    40.4% copy_process
       162   450.0% complete
       155     9.2% do_no_page
       147    19.5% set_page_dirty
       133    17.9% clear_page_tables
       131    13.8% do_wp_page
       129     0.0% find_idlest_cpu
       121     3.4% find_trylock_page
...
      -113    -7.0% atomic_dec_and_lock
     -1062    -2.2% default_idle

Which looks to me just like worse task affinity.

I still think balance on clone is the wrong thing to do by default. On anything but a benchmark, you have more than one process running on the system, and you WANT to keep threads of that process on the same node, not scatter them to the winds.

M.

make(1) with vfork(2) perhaps?

Visit Openswan at http://www.openswan.org/
Email:  Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

I think balance on clone probably needs to be turned off by default presently.

It slows down a simple thread creation test by a factor of 7(!) here, and has quite a few not too difficult to imagine performance problems.

I agree. However, I still think we should do my suggested "wake_up_new(p,clone_flags)" thing, and then have the logic on whether to try to care about threading or not be in schedule.c, not in kernel/fork.c.

The fact is, fork.c shouldn't try to make scheduling decisions. But it could inform the scheduler about the new process, and THAT can then make the decisions.

Linus

I agree, it is a fine suggestion. Would be a trivial change, and would clean things up nicely.

agreed, and i did it in a similar way initially (by adding the clone flags to wake_up_process()) but went for the smaller patch. The only reason i pushed it into fork.c initially was to avoid having to change dozens of other files (most of them in various architectures) that use wake_up_process(). It wasnt (and still isnt) clear at all whether we want to do any fork/clone-time balancing.

Ingo

Very likely. And in the vfork() case it is definitely WRONG to try to reschedule (either threads _or_ processes), since the parent is going to go to sleep real soon now.

I think this code:

                        if (clone_flags & CLONE_VM)
                                wake_up_forked_thread(p);
                        else
                                wake_up_forked_process(p);

is just wrong, and it should be replaced with

			wake_up_new_process(p, clone_flags);

and then "wake_up_new_process()" can do the right thing, which is basically:

if (clone_flags & CLONE_VFORK)
synchronous wakeup, same as pipe-will-block case
else if (clone_flags & CLONE_VM)
thread-wakeup-case
else
process-wakeup-case

No?

Linus

Looks much better ... but I'd still dispute whether we need to throw non-vfork threads cross node by default. I'd suggest that's disabled by default, and is either enabled by a global userspace option, or a per-process one (or the option of both). Most thing (except benchmarks) simply don't want this in real life ...

M.

Sigh, yes, Mark did run a test for me, but I think it was dbt2-pgsql. This one is dbt3-pgsql. Also, his system was a 4 logical CPU Xeon.

Strangely enough, Mark's setup was showing a fairly large too-much-idle regression not long ago, while these 8-ways weren't.

Anyway, Linus has reverted my patch now, which is the right thing to do. Your sync wakeup change is still in there, so that will hopefully help bw_pipe scores.

I have some changes which bring throughput up to 240, however I'm not sure if it would be wise to try to fix everything before 2.6.7.

I'd be happy for 2.6.7 to be released with kernel/sched.c as it is now. What are your thoughts?

Quoting Nick Piggin <[email protected]>:

The sync wakeup change was worth about 1% detriment to this benchmark which, if offset by significant performance gains elsewhere, is worth tolerating.

Con