idle issues running sembench on 128 cpus

Thomas,
I've been looking at performance running sembench on a 128-cpu system and I'm running into some issues in the idle loop.

Initially, I was seeing a lot of contention on the clockevents_lock in clockevents_notify(). Assuming it is only protecting clockevents_chain, and not the handlers themselves, I changed this to an rwlock (with thoughts of using rcu if successful).

This didn't help, but exposed an underlying problem with high contention on tick_broadcast_lock in tick_broadcast_oneshot_control(). I think with this many cpus, tick_handle_oneshot_broadcast() is holding that lock a long time, causing the idle cpus to spin on the lock.

I am able to avoid this problem with either kernel parameter, "idle=mwait" or "processor.max_cstate=1". Similarly, defining CONFIG_INTEL_IDLE=y and using the kernel parameter intel_idle.max_cstate=1 exposes a different spinlock, pm_qos_lock, but I found this patch which fixes that contention:
https://lists.linux-foundation.org/pipermail/linux-pm/2011-February/030266.html
https://patchwork.kernel.org/patch/550721/

Of course, we'd like to find a way to reduce the spinlock contention and not resort to prohibiting the cpus from entering C3 state at all. I don't see a simple fix, and want to know if you've seen anything like this before and given it any thought.

I also don't know if it makes sense to be able to tune the cpuidle governors to add more resistance to enter the C3 state, or even being able to switch to a performance governor at runtime, similar to cpufreq.

I'd like to hear your thoughts before I dive any deeper into this.

I am able to avoid this problem with either kernel parameter, "idle=mwait" or "processor.max_cstate=1". Similarly, defining CONFIG_INTEL_IDLE=y and using the kernel parameter intel_idle.max_cstate=1 exposes a different spinlock, pm_qos_lock, but I found this patch which fixes that contention:
https://lists.linux-foundation.org/pipermail/linux-pm/2011-February/030266.html
https://patchwork.kernel.org/patch/550721/

Of course, we'd like to find a way to reduce the spinlock contention and not resort to prohibiting the cpus from entering C3 state at all. I don't see a simple fix, and want to know if you've seen anything like this before and given it any thought.

I also don't know if it makes sense to be able to tune the cpuidle governors to add more resistance to enter the C3 state, or even being able to switch to a performance governor at runtime, similar to cpufreq.

I'd like to hear your thoughts before I dive any deeper into this.

Quoted 6 lines · Dave Kleikamp

I also don't know if it makes sense to be able to tune the cpuidle governors to add more resistance to enter the C3 state, or even being able to switch to a performance governor at runtime, similar to cpufreq.

I'd like to hear your thoughts before I dive any deeper into this.

It's fixed on Westmere. There the APIC timer will always tick and all that logic is not needed anymore and disabled.

That is mostly fixed. One problem right now is that the CLOCK_EVT_FEAT_C3STOP test is inside the lock. But we can easily move it out, assuming the clock_event_device gets RCU freed or has a reference count.

But yes it would be still good to fix Nehalem too.

One fix would be to make all the masks hierarchical, similar to what RCU does. Perhaps even some code could be shared with RCU on that because it's a very similar problem.

No, it does not even need refcounting. We can access it outside of the

lock as this is atomic context called on the cpu which is about to go idle and therefor the device cannot go away. Easy and straightforward fix.

Quoted 7 lines

But yes it would be still good to fix Nehalem too.

One fix would be to make all the masks hierarchical, similar to what RCU does. Perhaps even some code could be shared with RCU on that because it's a very similar problem.

In theory. It's not about the mask. The mask is uninteresting. It's about the expiry time, which we have to protect. There is nothing hierarchical about that. It all boils down on _ONE_ single functional

device and you don't want to miss out your deadline just because you decided to be extra clever. RCU does not care much whether you run the callbacks a tick later on not. Time and timekeeping does.

Quoted 1 line

No, it does not even need refcounting. We can access it outside of the

Ok.

Quoted 3 lines

lock as this is atomic context called on the cpu which is about to go idle and therefor the device cannot go away. Easy and straightforward fix.

Ok. Patch appended. Looks good?

BTW why must the lock be irqsave?

Quoted 10 lines

Quoted 6 lines

But yes it would be still good to fix Nehalem too.

One fix would be to make all the masks hierarchical, similar to what RCU does. Perhaps even some code could be shared with RCU on that because it's a very similar problem.

In theory. It's not about the mask. The mask is uninteresting. It's about the expiry time, which we have to protect. There is nothing hierarchical about that. It all boils down on _ONE_ single functional

The mask can be used to see if another thread on this core is still running. If yes you don't need that. Right now Linux doesn't know that, but it could be taught. The only problem is that once the other guy goes idle too their timeouts have to be merged.

This would cut contention in half.

Also if it's HPET you could actually use multiple independent HPET channels. I remember us discussing this a long time ago... Not sure if it's worth it, but it may be a small relief.

Quoted 3 lines

device and you don't want to miss out your deadline just because you decided to be extra clever. RCU does not care much whether you run the callbacks a tick later on not. Time and timekeeping does.

You can at least check lockless if someone else has a <= timeout, right?

-Andi

---

Move C3 stop test outside lock

Avoid taking locks in the idle path for systems where the timer doesn't stop in C3.

Signed-off-by Andi Kleen <[email protected]>

kernel/time/tick-broadcast.c +0 -1

index da800ff..9cf0415 100644
@@ -456,23 +456,22 @@ void tick_broadcast_oneshot_control(unsigned long reason)
 	unsigned long flags;
 	int cpu;
 
-	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);

Quoted 8 lines

Quoted 1 line

hierarchical about that. It all boils down on _ONE_ single functional

The mask can be used to see if another thread on this core is still running. If yes you don't need that. Right now Linux doesn't know that, but it could be taught. The only problem is that once the other guy goes idle too their timeouts have to be merged.

This would cut contention in half.

That makes sense, but merging the timeouts race free will be a real PITA.

Quoted 4 lines

Also if it's HPET you could actually use multiple independent HPET channels. I remember us discussing this a long time ago... Not sure if it's worth it, but it may be a small relief.

Multiple broadcast devices. That sounds still horrible :)

Might be worth a try. Need some sleep to remember why I discarded that idea long ago.

Quoted 2 lines · Thomas Gleixner

That makes sense, but merging the timeouts race free will be a real PITA.

For this case one could actually use a spinlock between the siblings. That shouldn't be a problem as long as it's not a global spinlock.

Quoted 5 lines

Quoted 3 lines

Also if it's HPET you could actually use multiple independent HPET channels. I remember us discussing this a long time ago... Not sure if it's worth it, but it may be a small relief.

Multiple broadcast devices. That sounds still horrible :)

It would cut contention in half or more at least. Not great, but sometimes you take everything you can get.

Here's a new patch without the raw. Boots on my Westmere.

Quoted 6 lines · Andi Kleen

Quoted 2 lines · Thomas Gleixner

That makes sense, but merging the timeouts race free will be a real PITA.

For this case one could actually use a spinlock between the siblings. That shouldn't be a problem as long as it's not a global spinlock.

Care to give it a try ?

kernel/time/tick-broadcast.c void tick_broadcast_oneshot_control(unsigned long reason) · line 466

Quoted 3 lines

Here's a new patch without the raw. Boots on my Westmere.

kernel/time/tick-broadcast.c +1 -0

+	cpu = raw_smp_processor_id();

Hmm. quilt refresh perhaps ? I know that feeling :)

Quoted 9 lines · Thomas Gleixner

Quoted 6 lines · Andi Kleen

Quoted 2 lines · Thomas Gleixner

That makes sense, but merging the timeouts race free will be a real PITA.

For this case one could actually use a spinlock between the siblings. That shouldn't be a problem as long as it's not a global spinlock.

Care to give it a try ?

Ok, will try tomorrow.

Quoted 5 lines

kernel/time/tick-broadcast.c void tick_broadcast_oneshot_control(unsigned long reason) · line 466

Quoted 3 lines

Here's a new patch without the raw. Boots on my Westmere.

kernel/time/tick-broadcast.c +1 -0

+	cpu = raw_smp_processor_id();

Hmm. quilt refresh perhaps ? I know that feeling :)

The scp to copy the patch was too slow. Noticed it later and sent a new one.

Quoted 9 lines · Andi Kleen

Quoted 1 line

No, it does not even need refcounting. We can access it outside of the

Ok.

Quoted 3 lines

lock as this is atomic context called on the cpu which is about to go idle and therefor the device cannot go away. Easy and straightforward fix.

Ok. Patch appended. Looks good?

Mostly. See below.

Quoted 1 line

BTW why must the lock be irqsave?

Good question. Probably safety frist paranoia :)

Indeed that code should only be called from irq disabled regions, so we could avoid the irqsave there. Otherwise that needs to be irqsave for obvious reasons.

Quoted 1 line

Here's a new patch without the raw. Boots on my Westmere.

Sorry appended the old patch. Here's really the new version.

From: Andi Kleen<[email protected]>
Date: Wed, 4 May 2011 15:09:27 -0700
Subject: [PATCH] Move C3 stop test outside lock

Avoid taking locks in the idle path for systems where the timer doesn't stop in C3.

Signed-off-by Andi Kleen<[email protected]>

kernel/time/tick-broadcast.c +1 -3

index da800ff..7f565b7 100644
@@ -456,23 +456,22 @@ void tick_broadcast_oneshot_control(unsigned long reason)
  	unsigned long flags;
  	int cpu;
 
-	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
-
  	/*
  	 * Periodic mode does not care about the enter/exit of power
  	 * states
  	 */
  	if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
-		goto out;
+		return;

kernel/time/tick-broadcast.c +1 -0

+	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
  	if (reason == CLOCK_EVT_NOTIFY_BROADCAST_ENTER) {
  		if (!cpumask_test_cpu(cpu, tick_get_broadcast_oneshot_mask())) {
  			cpumask_set_cpu(cpu, tick_get_broadcast_oneshot_mask());