powerpc: mitigate impact of decrementer reset

PATCH In review Paul Clarke 6 participants 14 messages Oct 7, 2014 – Nov 18, 2014 1 message not in the archive PowerPC

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Paul Clarke

author

2014-10-07 19:13 UTC #1 lore raw

PATCH 1/1 powerpc: mitigate impact of decrementer reset

The POWER ISA defines an always-running decrementer which can be used to schedule interrupts after a certain time interval has elapsed. The decrementer counts down at the same frequency as the Time Base, which is 512 MHz. The maximum value of the decrementer is 0x7fffffff. This works out to a maximum interval of about 4.19 seconds.

If a larger interval is desired, the kernel will set the decrementer to its maximum value and reset it after it expires (underflows) a sufficient number of times until the desired interval has elapsed.

The negative effect of this is that an unwanted latency spike will impact normal processing at most every 4.19 seconds. On an IBM POWER8-based system, this spike was measured at about 25-30 microseconds, much of which was basic, opportunistic housekeeping tasks that could otherwise have waited.

This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.

Signed-off-by Paul A. Clarke <[email protected]>

---
 arch/powerpc/kernel/time.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

arch/powerpc/kernel/time.c +13 -0

index 368ab37..962a06b 100644
@@ -528,6 +528,7 @@ void timer_interrupt(struct pt_regs * regs)
 {
 	struct pt_regs *old_regs;
 	u64 *next_tb = &__get_cpu_var(decrementers_next_tb);
+	u64 now;
 
 	/* Ensure a positive value is written to the decrementer, or else
 	 * some CPUs will continue to take decrementer exceptions.

@@ -550,6 +551,18 @@ void timer_interrupt(struct pt_regs * regs)
 	 */
 	may_hard_irq_enable();
 
+	/* If this is simply the decrementer expiring (underflow) due to
+	 * the limited size of the decrementer, and not a set timer,
+	 * reset (if needed) and return
+	 */
+	now = get_tb_or_rtc();
+	if (now < *next_tb) {
+		now = *next_tb - now;
+		if (now <= DECREMENTER_MAX)
+			set_dec((int)now);
+		__get_cpu_var(irq_stat).timer_irqs_others++;
+		return;
+	}
 
 #if defined(CONFIG_PPC32) && defined(CONFIG_PPC_PMAC)
 	if (atomic_read(&ppc_n_lost_interrupts) != 0)

Signature · 1 line

2.1.2.330.g565301e

Michael Ellerman

reviewer

2014-10-08 02:52 UTC #2 lore raw

Quoted 16 lines · Paul Clarke

The POWER ISA defines an always-running decrementer which can be used to schedule interrupts after a certain time interval has elapsed. The decrementer counts down at the same frequency as the Time Base, which is 512 MHz. The maximum value of the decrementer is 0x7fffffff. This works out to a maximum interval of about 4.19 seconds.
If a larger interval is desired, the kernel will set the decrementer to its maximum value and reset it after it expires (underflows) a sufficient number of times until the desired interval has elapsed.
The negative effect of this is that an unwanted latency spike will impact normal processing at most every 4.19 seconds. On an IBM POWER8-based system, this spike was measured at about 25-30 microseconds, much of which was basic, opportunistic housekeeping tasks that could otherwise have waited.

Quoted 4 lines

This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.

Hi Paul,

Thanks for the excellent changelog. But this patch makes me a bit nervous :)

Do you know where the latency is coming from? Is it primarily the irq work?

If so I'd prefer if we could move the short circuit into __timer_interrupt() itself. That way we'd still have the trace points usable, and it would hopefully result in less duplicated logic.

cheers

Preeti U Murthy

2014-10-08 10:27 UTC #3 lore raw

Quoted 24 lines · Michael Ellerman

Quoted 16 lines · Paul Clarke
The POWER ISA defines an always-running decrementer which can be used to schedule interrupts after a certain time interval has elapsed. The decrementer counts down at the same frequency as the Time Base, which is 512 MHz. The maximum value of the decrementer is 0x7fffffff. This works out to a maximum interval of about 4.19 seconds.
If a larger interval is desired, the kernel will set the decrementer to its maximum value and reset it after it expires (underflows) a sufficient number of times until the desired interval has elapsed.
The negative effect of this is that an unwanted latency spike will impact normal processing at most every 4.19 seconds. On an IBM POWER8-based system, this spike was measured at about 25-30 microseconds, much of which was basic, opportunistic housekeeping tasks that could otherwise have waited.
Quoted 4 lines
This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.
Hi Paul,

Quoted 7 lines

Thanks for the excellent changelog. But this patch makes me a bit nervous :)
Do you know where the latency is coming from? Is it primarily the irq work?
If so I'd prefer if we could move the short circuit into __timer_interrupt() itself. That way we'd still have the trace points usable, and it would hopefully result in less duplicated logic.

I agree, this is perhaps the better approach.

Regards
Preeti U Murthy

Quoted 7 lines

cheers
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://lists.ozlabs.org/listinfo/linuxppc-dev

Paul Clarke

author

2014-11-05 17:06 UTC #4 lore raw

In reply to Michael Ellerman

Sorry it took me so long to get back to this...

Quoted 17 lines · Michael Ellerman

Quoted 16 lines · Paul Clarke
The POWER ISA defines an always-running decrementer which can be used to schedule interrupts after a certain time interval has elapsed. The decrementer counts down at the same frequency as the Time Base, which is 512 MHz. The maximum value of the decrementer is 0x7fffffff. This works out to a maximum interval of about 4.19 seconds.
If a larger interval is desired, the kernel will set the decrementer to its maximum value and reset it after it expires (underflows) a sufficient number of times until the desired interval has elapsed.
The negative effect of this is that an unwanted latency spike will impact normal processing at most every 4.19 seconds. On an IBM POWER8-based system, this spike was measured at about 25-30 microseconds, much of which was basic, opportunistic housekeeping tasks that could otherwise have waited.

Quoted 8 lines

Quoted 4 lines
This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.
Thanks for the excellent changelog. But this patch makes me a bit nervous :)
Do you know where the latency is coming from? Is it primarily the irq work?

Yes, it is all under irq_enter (measured at ~10us) and irq_exit (~12us).

Quoted 3 lines

If so I'd prefer if we could move the short circuit into __timer_interrupt() itself. That way we'd still have the trace points usable, and it would hopefully result in less duplicated logic.

But irq_enter and irq_exit are called in timer_interrupt, before __timer_interrupt is called. I don't see how that helps. The time spent in __timer_interrupt is minuscule by comparison.

Are you suggesting that irq_enter/exit be moved into __timer_interrupt as well? (I'm not sure how that would impact the existing call to __timer_interrupt from tick_broadcast_ipi_handler? And if there is no impact, what's the point of separating timer_interrupt and __timer_interrupt?)

Regards,
PC

Michael Ellerman

reviewer

2014-11-13 02:39 UTC #5 lore raw

Quoted 9 lines · Paul Clarke

Sorry it took me so long to get back to this...
Quoted 5 lines · Michael Ellerman
Quoted 4 lines · Paul Clarke
This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.

Quoted 5 lines

Quoted 3 lines
Thanks for the excellent changelog. But this patch makes me a bit nervous :)
Do you know where the latency is coming from? Is it primarily the irq work?
Yes, it is all under irq_enter (measured at ~10us) and irq_exit (~12us).

Hmm, OK. I actually meant irq_work_run().

AIUI irq_enter/exit() are just state tracking, they shouldn't be actually running work.

How are you measuring it?

Quoted 7 lines

Quoted 3 lines
If so I'd prefer if we could move the short circuit into __timer_interrupt() itself. That way we'd still have the trace points usable, and it would hopefully result in less duplicated logic.
But irq_enter and irq_exit are called in timer_interrupt, before __timer_interrupt is called. I don't see how that helps. The time spent in __timer_interrupt is minuscule by comparison.

Right, it won't help if it's irq_enter() that is causing the delay. But I was assuming it was irq_work_run().

Quoted 5 lines

Are you suggesting that irq_enter/exit be moved into __timer_interrupt as well? (I'm not sure how that would impact the existing call to __timer_interrupt from tick_broadcast_ipi_handler? And if there is no impact, what's the point of separating timer_interrupt and __timer_interrupt?)

The point is __timer_interrupt() is called from tick_broadcast_ipi_handler(), which is called from smp_ipi_demux(), from icp_hv_ipi_action(), from __do_irq(), which has already done irq_enter() (and will do irq_exit()).

cheers

Paul Clarke

author

2014-11-13 19:34 UTC #6 lore raw

Quoted 12 lines · Michael Ellerman

Quoted 11 lines · Paul Clarke
Quoted 9 lines · Michael Ellerman
Quoted 4 lines · Paul Clarke
This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.
Thanks for the excellent changelog. But this patch makes me a bit nervous :)
Do you know where the latency is coming from? Is it primarily the irq work?

Quoted 8 lines

Quoted 1 line
Yes, it is all under irq_enter (measured at ~10us) and irq_exit (~12us).
Hmm, OK. I actually meant irq_work_run().
AIUI irq_enter/exit() are just state tracking, they shouldn't be actually running work.
How are you measuring it?

ftrace function_graph tracer:
--

  127.425212 |                |  .irq_enter() {
  127.425213 |                |    .rcu_irq_enter() {
  127.425213 |  + 12.206 us   |      .rcu_eqs_exit_common.isra.41();
  127.425226 |  + 12.750 us   |    }
... RCU is a big hitter
  127.425226 |                |    .vtime_common_account_irq_enter() {
  127.425226 |                |      .vtime_account_user() {
  127.425226 |    0.032 us    |        ._raw_spin_lock();
  127.425227 |    0.034 us    |        .get_vtime_delta();
  127.425227 |                |        .account_user_time() {
  127.425228 |    0.030 us    |          .cpuacct_account_field();
  127.425228 |                |          .acct_account_cputime() {
  127.425228 |    0.082 us    |            .__acct_update_integrals();
  127.425229 |    0.562 us    |          }
  127.425229 |    1.500 us    |        }
  127.425229 |    2.954 us    |      }
  127.425230 |    3.434 us    |    }
... but even accounting is not insignificant
  127.425230 |  + 17.218 us   |  }
  127.425230 |                |  /* timer_interrupt_entry: [...] */
... nothing to see here, because there's nothing to do except reset the decrementer
  127.425230 |                |  /* timer_interrupt_exit: [...] */
... (less than 1 us spent doing the "required" work)
  127.425231 |                |  .irq_exit() {
  127.425231 |                |    .vtime_gen_account_irq_exit() {
  127.425231 |    0.036 us    |      ._raw_spin_lock();
  127.425232 |                |      .__vtime_account_system() {
  127.425232 |    0.030 us    |        .get_vtime_delta();
  127.425232 |                |        .account_system_time() {
  127.425233 |    0.030 us    |          .cpuacct_account_field();
  127.425233 |                |          .acct_account_cputime() {
  127.425233 |    0.072 us    |            .__acct_update_integrals();
  127.425234 |    0.564 us    |          }
  127.425234 |    1.546 us    |        }
  127.425234 |    2.528 us    |      }
  127.425235 |    3.700 us    |    }
... significant accounting time
  127.425235 |    0.032 us    |    .idle_cpu();
  127.425235 |                |    .tick_nohz_irq_exit() {
  127.425236 |                |      .can_stop_full_tick() {
  127.425236 |    0.022 us    |        .sched_can_stop_tick();
  127.425236 |    0.020 us    |        .posix_cpu_timers_can_stop_tick()
  127.425237 |    0.970 us    |      }
  127.425237 |    0.082 us    |      .ktime_get();
  127.425238 |                |      .tick_nohz_stop_sched_tick() {
  127.425238 |    0.032 us    |        .timekeeping_max_deferment();
  127.425238 |                |        .get_next_timer_interrupt() {
  127.425239 |    0.038 us    |          ._raw_spin_lock();
  127.425239 |                |          .hrtimer_get_next_event() {
  127.425239 |    0.030 us    |            ._raw_spin_lock_irqsave();
  127.425240 |    0.028 us    |            ._raw_spin_unlock_irqrestore
  127.425240 |    0.984 us    |          }
  127.425241 |    1.936 us    |        }
  127.425241 |    0.032 us    |        .scheduler_tick_max_deferment();
  127.425241 |    3.438 us    |      }
  127.425242 |    5.880 us    |    }
  127.425242 |                |    .rcu_irq_exit() {
  127.425242 |    0.102 us    |      .rcu_eqs_enter_common.isra.40();
  127.425243 |    0.576 us    |    }
  127.425243 |  + 12.156 us   |  }

This one was almost 30 us total (17.218 + 12.156 = 29.374 us), just to reset the decrementer.

Quoted 17 lines

Quoted 7 lines
Quoted 3 lines
If so I'd prefer if we could move the short circuit into __timer_interrupt() itself. That way we'd still have the trace points usable, and it would hopefully result in less duplicated logic.
But irq_enter and irq_exit are called in timer_interrupt, before __timer_interrupt is called. I don't see how that helps. The time spent in __timer_interrupt is minuscule by comparison.
Right, it won't help if it's irq_enter() that is causing the delay. But I was assuming it was irq_work_run().
Quoted 5 lines
Are you suggesting that irq_enter/exit be moved into __timer_interrupt as well? (I'm not sure how that would impact the existing call to __timer_interrupt from tick_broadcast_ipi_handler? And if there is no impact, what's the point of separating timer_interrupt and __timer_interrupt?)

Quoted 3 lines

The point is __timer_interrupt() is called from tick_broadcast_ipi_handler(), which is called from smp_ipi_demux(), from icp_hv_ipi_action(), from __do_irq(), which has already done irq_enter() (and will do irq_exit()).

If that's the only impact, maybe an "IRQ entered" flag would suffice to either prevent a 2nd call to irq_enter(), or allow irq_enter to be "reentrant" (where it just returns if it was called and the last call was not paired with an irq_exit yet? Alternatively, a new parameter to __timer_interrupt() which indicates the same.

Heinz Wrobel

2014-10-08 06:11 UTC #7 lore raw

In reply to Paul Clarke

UGF1bCwNCg0Kd2hhdCBpZiB5b3VyIHRiIHdyYXBzIGR1cmluZyB0aGUgIHRlc3Q/DQoNCj4gLS0t LS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTGludXhwcGMtZGV2IFttYWlsdG86bGlu dXhwcGMtZGV2LQ0KPiBib3VuY2VzK2hlaW56Lndyb2JlbD1mcmVlc2NhbGUuY29tQGxpc3RzLm96 bGFicy5vcmddIE9uIEJlaGFsZiBPZiBQYXVsDQo+IENsYXJrZQ0KPiBTZW50OiBUdWVzZGF5LCBP Y3RvYmVyIDA3LCAyMDE0IDIxOjEzDQo+IFRvOiBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9y Zw0KPiBTdWJqZWN0OiBbUEFUQ0hdIHBvd2VycGM6IG1pdGlnYXRlIGltcGFjdCBvZiBkZWNyZW1l bnRlciByZXNldA0KPiANCj4gVGhlIFBPV0VSIElTQSBkZWZpbmVzIGFuIGFsd2F5cy1ydW5uaW5n IGRlY3JlbWVudGVyIHdoaWNoIGNhbiBiZSB1c2VkIHRvDQo+IHNjaGVkdWxlIGludGVycnVwdHMg YWZ0ZXIgYSBjZXJ0YWluIHRpbWUgaW50ZXJ2YWwgaGFzIGVsYXBzZWQuDQo+IFRoZSBkZWNyZW1l bnRlciBjb3VudHMgZG93biBhdCB0aGUgc2FtZSBmcmVxdWVuY3kgYXMgdGhlIFRpbWUgQmFzZSwg d2hpY2gNCj4gaXMgNTEyIE1Iei4gIFRoZSBtYXhpbXVtIHZhbHVlIG9mIHRoZSBkZWNyZW1lbnRl ciBpcyAweDdmZmZmZmZmLg0KPiBUaGlzIHdvcmtzIG91dCB0byBhIG1heGltdW0gaW50ZXJ2YWwg b2YgYWJvdXQgNC4xOSBzZWNvbmRzLg0KPiANCj4gSWYgYSBsYXJnZXIgaW50ZXJ2YWwgaXMgZGVz aXJlZCwgdGhlIGtlcm5lbCB3aWxsIHNldCB0aGUgZGVjcmVtZW50ZXIgdG8gaXRzDQo+IG1heGlt dW0gdmFsdWUgYW5kIHJlc2V0IGl0IGFmdGVyIGl0IGV4cGlyZXMgKHVuZGVyZmxvd3MpIGEgc3Vm ZmljaWVudCBudW1iZXIgb2YNCj4gdGltZXMgdW50aWwgdGhlIGRlc2lyZWQgaW50ZXJ2YWwgaGFz IGVsYXBzZWQuDQo+IA0KPiBUaGUgbmVnYXRpdmUgZWZmZWN0IG9mIHRoaXMgaXMgdGhhdCBhbiB1 bndhbnRlZCBsYXRlbmN5IHNwaWtlIHdpbGwgaW1wYWN0IG5vcm1hbA0KPiBwcm9jZXNzaW5nIGF0 IG1vc3QgZXZlcnkgNC4xOSBzZWNvbmRzLiAgT24gYW4gSUJNIFBPV0VSOC1iYXNlZCBzeXN0ZW0s IHRoaXMNCj4gc3Bpa2Ugd2FzIG1lYXN1cmVkIGF0IGFib3V0IDI1LTMwIG1pY3Jvc2Vjb25kcywg bXVjaCBvZiB3aGljaCB3YXMgYmFzaWMsDQo+IG9wcG9ydHVuaXN0aWMgaG91c2VrZWVwaW5nIHRh c2tzIHRoYXQgY291bGQgb3RoZXJ3aXNlIGhhdmUgd2FpdGVkLg0KPiANCj4gVGhpcyBwYXRjaCBz aG9ydC1jaXJjdWl0cyB0aGUgcmVzZXQgb2YgdGhlIGRlY3JlbWVudGVyLCBleGl0aW5nIGFmdGVy IHRoZQ0KPiBkZWNyZW1lbnRlciByZXNldCwgYnV0IGJlZm9yZSB0aGUgaG91c2VrZWVwaW5nIHRh c2tzIGlmIHRoZSBvbmx5IG5lZWQgZm9yIHRoZQ0KPiBpbnRlcnJ1cHQgaXMgc2ltcGx5IHRvIHJl c2V0IGl0LiAgQWZ0ZXIgdGhpcyBwYXRjaCwgdGhlIGxhdGVuY3kgc3Bpa2Ugd2FzIG1lYXN1cmVk DQo+IGF0IGFib3V0IDE1MCBuYW5vc2Vjb25kcy4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IFBhdWwg QS4gQ2xhcmtlIDxwY0B1cy5pYm0uY29tPg0KPiAtLS0NCj4gICBhcmNoL3Bvd2VycGMva2VybmVs L3RpbWUuYyB8IDEzICsrKysrKysrKysrKysNCj4gICAxIGZpbGUgY2hhbmdlZCwgMTMgaW5zZXJ0 aW9ucygrKQ0KPiANCj4gZGlmZiAtLWdpdCBhL2FyY2gvcG93ZXJwYy9rZXJuZWwvdGltZS5jIGIv YXJjaC9wb3dlcnBjL2tlcm5lbC90aW1lLmMgaW5kZXgNCj4gMzY4YWIzNy4uOTYyYTA2YiAxMDA2 NDQNCj4gLS0tIGEvYXJjaC9wb3dlcnBjL2tlcm5lbC90aW1lLmMNCj4gKysrIGIvYXJjaC9wb3dl cnBjL2tlcm5lbC90aW1lLmMNCj4gQEAgLTUyOCw2ICs1MjgsNyBAQCB2b2lkIHRpbWVyX2ludGVy cnVwdChzdHJ1Y3QgcHRfcmVncyAqIHJlZ3MpDQo+ICAgew0KPiAgIAlzdHJ1Y3QgcHRfcmVncyAq b2xkX3JlZ3M7DQo+ICAgCXU2NCAqbmV4dF90YiA9ICZfX2dldF9jcHVfdmFyKGRlY3JlbWVudGVy c19uZXh0X3RiKTsNCj4gKwl1NjQgbm93Ow0KPiANCj4gICAJLyogRW5zdXJlIGEgcG9zaXRpdmUg dmFsdWUgaXMgd3JpdHRlbiB0byB0aGUgZGVjcmVtZW50ZXIsIG9yIGVsc2UNCj4gICAJICogc29t ZSBDUFVzIHdpbGwgY29udGludWUgdG8gdGFrZSBkZWNyZW1lbnRlciBleGNlcHRpb25zLg0KPiBA QCAtNTUwLDYgKzU1MSwxOCBAQCB2b2lkIHRpbWVyX2ludGVycnVwdChzdHJ1Y3QgcHRfcmVncyAq IHJlZ3MpDQo+ICAgCSAqLw0KPiAgIAltYXlfaGFyZF9pcnFfZW5hYmxlKCk7DQo+IA0KPiArCS8q IElmIHRoaXMgaXMgc2ltcGx5IHRoZSBkZWNyZW1lbnRlciBleHBpcmluZyAodW5kZXJmbG93KSBk dWUgdG8NCj4gKwkgKiB0aGUgbGltaXRlZCBzaXplIG9mIHRoZSBkZWNyZW1lbnRlciwgYW5kIG5v dCBhIHNldCB0aW1lciwNCj4gKwkgKiByZXNldCAoaWYgbmVlZGVkKSBhbmQgcmV0dXJuDQo+ICsJ ICovDQo+ICsJbm93ID0gZ2V0X3RiX29yX3J0YygpOw0KPiArCWlmIChub3cgPCAqbmV4dF90Yikg ew0KDQpXaGF0IGlmICJub3ciIGFuZCAqbmV4dF90YiBhcmUgbm90IG9uIHRoZSBzYW1lIHdyYXAg Y291bnQ/IFRoZXkgYXJlIGJvdGggbW9kdWxvIHZhbHVlcyBBRkFDUy4NClNob3VsZG4ndCB0aGlz IGJlIHJpZ2h0IGhlcmUgbW9yZSBsaWtlIGEgImlmICgoKm5leHRfdGIgLSBub3cpIDwgMl42Myki IHN0eWxlIHRlc3QgdG8gY2hlY2sgZm9yIGRlbHRhcyB3aXRoaW4gdGhlIHJhbmdlIGluc3RlYWQg b2YgYWJzb2x1dGUgdmFsdWVzPw0KDQo+ICsJCW5vdyA9ICpuZXh0X3RiIC0gbm93Ow0KPiArCQlp ZiAobm93IDw9IERFQ1JFTUVOVEVSX01BWCkNCj4gKwkJCXNldF9kZWMoKGludClub3cpOw0KPiAr CQlfX2dldF9jcHVfdmFyKGlycV9zdGF0KS50aW1lcl9pcnFzX290aGVycysrOw0KPiArCQlyZXR1 cm47DQo+ICsJfQ0KPiANCj4gICAjaWYgZGVmaW5lZChDT05GSUdfUFBDMzIpICYmIGRlZmluZWQo Q09ORklHX1BQQ19QTUFDKQ0KPiAgIAlpZiAoYXRvbWljX3JlYWQoJnBwY19uX2xvc3RfaW50ZXJy dXB0cykgIT0gMCkNCj4gLS0NCj4gMi4xLjIuMzMwLmc1NjUzMDFlDQoNCkJSLA0KDQpIZWlueg0K

Paul Clarke

author

2014-10-08 12:27 UTC #8 lore raw

Quoted 1 line · Heinz Wrobel

what if your tb wraps during the test?

Per the Power ISA, Time Base is 64 bits, monotonically increasing, and is writable only in hypervisor state. To my understanding, it is set to zero at boot (although this is not prescribed).

Also, as noted by others, the logic is roughly duplicated (with some differences) from the analogous code in __timer_interrupt just above it.

I don't see wrapping as a concern.

Benjamin Herrenschmidt

2014-11-10 10:08 UTC #9 lore raw

In reply to Paul Clarke

Quoted 16 lines · Paul Clarke

The POWER ISA defines an always-running decrementer which can be used to schedule interrupts after a certain time interval has elapsed. The decrementer counts down at the same frequency as the Time Base, which is 512 MHz. The maximum value of the decrementer is 0x7fffffff. This works out to a maximum interval of about 4.19 seconds.
If a larger interval is desired, the kernel will set the decrementer to its maximum value and reset it after it expires (underflows) a sufficient number of times until the desired interval has elapsed.
The negative effect of this is that an unwanted latency spike will impact normal processing at most every 4.19 seconds. On an IBM POWER8-based system, this spike was measured at about 25-30 microseconds, much of which was basic, opportunistic housekeeping tasks that could otherwise have waited.

Quoted 4 lines

This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.

Doesn't this break the irq_work stuff ? We trigger it with a set_dec(1); and your patch will probably cause it to be skipped...

Cheers,
Ben.

Quoted 36 lines

Signed-off-by Paul A. Clarke <[email protected]>

---
  arch/powerpc/kernel/time.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

arch/powerpc/kernel/time.c +13 -0

index 368ab37..962a06b 100644
@@ -528,6 +528,7 @@ void timer_interrupt(struct pt_regs * regs)
  {
  	struct pt_regs *old_regs;
  	u64 *next_tb = &__get_cpu_var(decrementers_next_tb);
+	u64 now;
 
  	/* Ensure a positive value is written to the decrementer, or else
  	 * some CPUs will continue to take decrementer exceptions.

@@ -550,6 +551,18 @@ void timer_interrupt(struct pt_regs * regs)
  	 */
  	may_hard_irq_enable();
 
+	/* If this is simply the decrementer expiring (underflow) due to
+	 * the limited size of the decrementer, and not a set timer,
+	 * reset (if needed) and return
+	 */
+	now = get_tb_or_rtc();
+	if (now < *next_tb) {
+		now = *next_tb - now;
+		if (now <= DECREMENTER_MAX)
+			set_dec((int)now);
+		__get_cpu_var(irq_stat).timer_irqs_others++;
+		return;
+	}
 
  #if defined(CONFIG_PPC32) && defined(CONFIG_PPC_PMAC)
  	if (atomic_read(&ppc_n_lost_interrupts) != 0)

Paul Clarke

author

2014-11-10 20:58 UTC #10 lore raw

Quoted 17 lines · Benjamin Herrenschmidt

Quoted 16 lines · Paul Clarke
The POWER ISA defines an always-running decrementer which can be used to schedule interrupts after a certain time interval has elapsed. The decrementer counts down at the same frequency as the Time Base, which is 512 MHz. The maximum value of the decrementer is 0x7fffffff. This works out to a maximum interval of about 4.19 seconds.
If a larger interval is desired, the kernel will set the decrementer to its maximum value and reset it after it expires (underflows) a sufficient number of times until the desired interval has elapsed.
The negative effect of this is that an unwanted latency spike will impact normal processing at most every 4.19 seconds. On an IBM POWER8-based system, this spike was measured at about 25-30 microseconds, much of which was basic, opportunistic housekeeping tasks that could otherwise have waited.

Quoted 7 lines

Quoted 4 lines
This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.
Doesn't this break the irq_work stuff ? We trigger it with a set_dec(1); and your patch will probably cause it to be skipped...

You're right.

I'm confused by the division between timer_interrupt() and __timer_interrupt(). The former is called with interrupts disabled (and enables them), but also calls irq_enter()/irq_exit(). Why are those calls not in __timer_interrupt()? (If they were, the short-circuit logic might be a bit easier to put directly in __timer_interrupt(), which would eliminate any duplicate code.)

It looks like __timer_interrupt is only called directly by the broadcast timer IPI handler. (Why is __timer_interrupt not static?) Does this path not need irq_enter/irq_exit?

Quoted 34 lines

Signed-off-by Paul A. Clarke <[email protected]>

---
   arch/powerpc/kernel/time.c | 13 +++++++++++++
   1 file changed, 13 insertions(+)

arch/powerpc/kernel/time.c +13 -0

index 368ab37..962a06b 100644
@@ -528,6 +528,7 @@ void timer_interrupt(struct pt_regs * regs)
   {
   	struct pt_regs *old_regs;
   	u64 *next_tb = &__get_cpu_var(decrementers_next_tb);
+	u64 now;
 
   	/* Ensure a positive value is written to the decrementer, or else
   	 * some CPUs will continue to take decrementer exceptions.

@@ -550,6 +551,18 @@ void timer_interrupt(struct pt_regs * regs)
   	 */
   	may_hard_irq_enable();
 
+	/* If this is simply the decrementer expiring (underflow) due to
+	 * the limited size of the decrementer, and not a set timer,
+	 * reset (if needed) and return
+	 */
+	now = get_tb_or_rtc();
+	if (now < *next_tb) {
+		now = *next_tb - now;
+		if (now <= DECREMENTER_MAX)
+			set_dec((int)now);
+		__get_cpu_var(irq_stat).timer_irqs_others++;
+		return;
+	}

Quoted 2 lines

arch/powerpc/kernel/time.c void timer_interrupt(struct pt_regs * regs) · line 568
Quoted 2 lines
arch/powerpc/kernel/time.c +0 -0
   #if defined(CONFIG_PPC32) && defined(CONFIG_PPC_PMAC)
   	if (atomic_read(&ppc_n_lost_interrupts) != 0)

Regards,
PC

Michael Ellerman

reviewer

2014-11-13 02:42 UTC #11 lore raw

Quoted 11 lines · Paul Clarke

Quoted 8 lines · Benjamin Herrenschmidt
Quoted 4 lines · Paul Clarke
This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.
Doesn't this break the irq_work stuff ? We trigger it with a set_dec(1); and your patch will probably cause it to be skipped...
You're right.

Yeah, thanks Ben, that would have been bad.

So we'll need to come up with a different approach.

Quoted 10 lines

I'm confused by the division between timer_interrupt() and __timer_interrupt(). The former is called with interrupts disabled (and enables them), but also calls irq_enter()/irq_exit(). Why are those calls not in __timer_interrupt()? (If they were, the short-circuit logic might be a bit easier to put directly in __timer_interrupt(), which would eliminate any duplicate code.)
It looks like __timer_interrupt is only called directly by the broadcast timer IPI handler. (Why is __timer_interrupt not static?) Does this path not need irq_enter/irq_exit?

I think I answered most of this in the other mail I just sent, but let me know if not.

And __timer_interrupt() is static, if you have a new enough kernel :)

cheers

Paul E. McKenney

2014-11-17 19:18 UTC #12 lore raw

Quoted 24 lines · Michael Ellerman

Quoted 11 lines · Paul Clarke
Quoted 8 lines · Benjamin Herrenschmidt
Quoted 4 lines · Paul Clarke
This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.
Doesn't this break the irq_work stuff ? We trigger it with a set_dec(1); and your patch will probably cause it to be skipped...
You're right.
Yeah, thanks Ben, that would have been bad.
So we'll need to come up with a different approach.
Quoted 7 lines
I'm confused by the division between timer_interrupt() and __timer_interrupt(). The former is called with interrupts disabled (and enables them), but also calls irq_enter()/irq_exit(). Why are those calls not in __timer_interrupt()? (If they were, the short-circuit logic might be a bit easier to put directly in __timer_interrupt(), which would eliminate any duplicate code.)

Quoted 8 lines

Quoted 3 lines
It looks like __timer_interrupt is only called directly by the broadcast timer IPI handler. (Why is __timer_interrupt not static?) Does this path not need irq_enter/irq_exit?
I think I answered most of this in the other mail I just sent, but let me know if not.
And __timer_interrupt() is static, if you have a new enough kernel :)

If I am understanding this correctly, it underscores the need for more bits in the decrementer register. :-/

Thanx, Paul

Michael Ellerman

reviewer

2014-11-18 01:46 UTC #13 lore raw

Quoted 12 lines · Paul E. McKenney

Quoted 11 lines · Michael Ellerman
Quoted 10 lines · Paul Clarke
Quoted 8 lines · Benjamin Herrenschmidt
Quoted 4 lines · Paul Clarke
This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.
Doesn't this break the irq_work stuff ? We trigger it with a set_dec(1); and your patch will probably cause it to be skipped...

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
You're right.
Yeah, thanks Ben, that would have been bad.
So we'll need to come up with a different approach.
If I am understanding this correctly, it underscores the need for more bits in the decrementer register. :-/

Yes that is the root cause of the problem :)

cheers

Paul E. McKenney

2014-11-18 03:08 UTC #14 lore raw

Quoted 15 lines · Michael Ellerman

Quoted 14 lines · Paul E. McKenney
Quoted 13 lines · Michael Ellerman
Quoted 11 lines · Paul Clarke
Quoted 8 lines · Benjamin Herrenschmidt
Quoted 4 lines · Paul Clarke
This patch short-circuits the reset of the decrementer, exiting after the decrementer reset, but before the housekeeping tasks if the only need for the interrupt is simply to reset it. After this patch, the latency spike was measured at about 150 nanoseconds.
Doesn't this break the irq_work stuff ? We trigger it with a set_dec(1); and your patch will probably cause it to be skipped...
You're right.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Yeah, thanks Ben, that would have been bad.
So we'll need to come up with a different approach.
If I am understanding this correctly, it underscores the need for more bits in the decrementer register. :-/
Yes that is the root cause of the problem :)

Sigh!!! I was hoping! ;-)

Thanx, Paul