On 18/11/19 22:44, Marcelo Tosatti wrote:
> Second patch blocks NTPd from synchronizing to a source at all
> (can't even confirm if it fails to synchronize after a while).
>
> Problem seems to be that calling from timer interrupt path:
>
> /*
> * if the periodic timer's update is due to period re-configuration,
> * we should count the clock since last interrupt.
> */
> if (old_period) {
> int64_t last_periodic_clock, next_periodic_clock;
>
> next_periodic_clock = muldiv64(s->next_periodic_time,
> RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> last_periodic_clock = next_periodic_clock - old_period;
> lost_clock = cur_clock - last_periodic_clock;
> assert(lost_clock >= 0);
> }
>
> Adds the difference between when the timer interrupt actually executed
> (cur_clock) and when it should have executed (last_periodic_clock)
> as reinject time (which will end up injecting more timer interrupts
> than necessary, so the clock runs faster than it should).
Yes, I made a similar reasoning. Thanks for the patch, I don't like
that it adds complication over complication but I guess it would be okay
for 4.2, if Alex confirms it works for him. Mine is a much bigger
change .
> Perhaps this is the reason for the 5%+ performance delta?
>
> The following, on top of Paolo's two patches, fixes it for me
> (and NTPd is able to maintain clock synchronized while playing video on
> Windows 7).
FYI, here is my attempt at cleaning up everything:
------------------- 8< ----------------
>From c0a53b1c9a331ac4cf5846b477ba0fb795933a34 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonz...@redhat.com>
Date: Sun, 17 Nov 2019 10:07:38 +0100
Subject: [PATCH 1/5] Revert "mc146818rtc: fix timer interrupt reinjection"
This reverts commit b429de730174b388ea5760e3debb0d542ea3c261, except
that the reversal of the outer "if (period)" is left in.
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
hw/rtc/mc146818rtc.c | 67 ++++++++++++++++++++++++++--------------------------
1 file changed, 33 insertions(+), 34 deletions(-)
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index ee6bf82..9869dc5 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -174,7 +174,6 @@ periodic_timer_update(RTCState *s, int64_t current_time,
uint32_t old_period)
int64_t cur_clock, next_irq_clock, lost_clock = 0;
period = rtc_periodic_clock_ticks(s);
-
if (!period) {
s->irq_coalesced = 0;
timer_del(s->periodic_timer);
@@ -197,42 +196,42 @@ periodic_timer_update(RTCState *s, int64_t current_time,
uint32_t old_period)
last_periodic_clock = next_periodic_clock - old_period;
lost_clock = cur_clock - last_periodic_clock;
assert(lost_clock >= 0);
+ }
+ /*
+ * s->irq_coalesced can change for two reasons:
+ *
+ * a) if one or more periodic timer interrupts have been lost,
+ * lost_clock will be more that a period.
+ *
+ * b) when the period may be reconfigured, we expect the OS to
+ * treat delayed tick as the new period. So, when switching
+ * from a shorter to a longer period, scale down the missing,
+ * because the OS will treat past delayed ticks as longer
+ * (leftovers are put back into lost_clock). When switching
+ * to a shorter period, scale up the missing ticks since the
+ * OS handler will treat past delayed ticks as shorter.
+ */
+ if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+ uint32_t old_irq_coalesced = s->irq_coalesced;
+
+ s->period = period;
+ lost_clock += old_irq_coalesced * old_period;
+ s->irq_coalesced = lost_clock / s->period;
+ lost_clock %= s->period;
+ if (old_irq_coalesced != s->irq_coalesced ||
+ old_period != s->period) {
+ DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, "
+ "period scaled from %d to %d\n", old_irq_coalesced,
+ s->irq_coalesced, old_period, s->period);
+ rtc_coalesced_timer_update(s);
+ }
+ } else {
/*
- * s->irq_coalesced can change for two reasons:
- *
- * a) if one or more periodic timer interrupts have been lost,
- * lost_clock will be more that a period.
- *
- * b) when the period may be reconfigured, we expect the OS to
- * treat delayed tick as the new period. So, when switching
- * from a shorter to a longer period, scale down the missing,
- * because the OS will treat past delayed ticks as longer
- * (leftovers are put back into lost_clock). When switching
- * to a shorter period, scale up the missing ticks since the
- * OS handler will treat past delayed ticks as shorter.
+ * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
+ * is not used, we should make the time progress anyway.
*/
- if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
- uint32_t old_irq_coalesced = s->irq_coalesced;
-
- s->period = period;
- lost_clock += old_irq_coalesced * old_period;
- s->irq_coalesced = lost_clock / s->period;
- lost_clock %= s->period;
- if (old_irq_coalesced != s->irq_coalesced ||
- old_period != s->period) {
- DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, "
- "period scaled from %d to %d\n", old_irq_coalesced,
- s->irq_coalesced, old_period, s->period);
- rtc_coalesced_timer_update(s);
- }
- } else {
- /*
- * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
- * is not used, we should make the time progress anyway.
- */
- lost_clock = MIN(lost_clock, period);
- }
+ lost_clock = MIN(lost_clock, period);
}
assert(lost_clock >= 0 && lost_clock <= period);
--
1.8.3.1
>From 6a989dedd43b1885bd3f2178d686bf7a4fe06a08 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonz...@redhat.com>
Date: Sun, 17 Nov 2019 10:28:14 +0100
Subject: [PATCH 2/5] mc146818rtc: keep s->period up to date
Right now s->period is only used if s->lost_tick_policy ==
LOST_TICK_POLICY_SLEW. Not having to recompute it all the time
will come in handy in upcoming refactoring, so just store it in
RTCState.
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
hw/rtc/mc146818rtc.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 9869dc5..da7837c 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -174,6 +174,8 @@ periodic_timer_update(RTCState *s, int64_t current_time,
uint32_t old_period)
int64_t cur_clock, next_irq_clock, lost_clock = 0;
period = rtc_periodic_clock_ticks(s);
+ s->period = period;
+
if (!period) {
s->irq_coalesced = 0;
timer_del(s->periodic_timer);
@@ -215,7 +217,6 @@ periodic_timer_update(RTCState *s, int64_t current_time,
uint32_t old_period)
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
uint32_t old_irq_coalesced = s->irq_coalesced;
- s->period = period;
lost_clock += old_irq_coalesced * old_period;
s->irq_coalesced = lost_clock / s->period;
lost_clock %= s->period;
@@ -231,12 +232,12 @@ periodic_timer_update(RTCState *s, int64_t current_time,
uint32_t old_period)
* no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
* is not used, we should make the time progress anyway.
*/
- lost_clock = MIN(lost_clock, period);
+ lost_clock = MIN(lost_clock, s->period);
}
- assert(lost_clock >= 0 && lost_clock <= period);
+ assert(lost_clock >= 0 && lost_clock <= s->period);
- next_irq_clock = cur_clock + period - lost_clock;
+ next_irq_clock = cur_clock + s->period - lost_clock;
s->next_periodic_time = periodic_clock_to_ns(next_irq_clock) + 1;
timer_mod(s->periodic_timer, s->next_periodic_time);
}
@@ -794,6 +795,7 @@ static int rtc_post_load(void *opaque, int version_id)
s->offset = 0;
check_update_timer(s);
}
+ s->period = rtc_periodic_clock_ticks(s);
/* The periodic timer is deterministic in record/replay mode,
* so there is no need to update it after loading the vmstate.
--
1.8.3.1
>From 258e46c4f234385c60857ef3542c335bf6560608 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonz...@redhat.com>
Date: Mon, 18 Nov 2019 11:50:31 +0100
Subject: [PATCH 3/5] mc146818rtc: clean up update of coalesced timer for
periodic_timer_update
Remove knowledge of old_period from the code that sets up the
next periodic timer deadline. Instead, account the missed IRQs into
lost_clock before that part runs.
This prepares for splitting periodic_timer_update in two parts,
one for period reconfiguration and one that sets up the next
periodic timer deadline.
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
hw/rtc/mc146818rtc.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index da7837c..47d966c 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -198,6 +198,12 @@ periodic_timer_update(RTCState *s, int64_t current_time,
uint32_t old_period)
last_periodic_clock = next_periodic_clock - old_period;
lost_clock = cur_clock - last_periodic_clock;
assert(lost_clock >= 0);
+
+ if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+ lost_clock += s->irq_coalesced * old_period;
+ s->irq_coalesced = 0;
+ timer_del(s->coalesced_timer);
+ }
}
/*
@@ -215,18 +221,9 @@ periodic_timer_update(RTCState *s, int64_t current_time,
uint32_t old_period)
* OS handler will treat past delayed ticks as shorter.
*/
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
- uint32_t old_irq_coalesced = s->irq_coalesced;
-
- lost_clock += old_irq_coalesced * old_period;
s->irq_coalesced = lost_clock / s->period;
lost_clock %= s->period;
- if (old_irq_coalesced != s->irq_coalesced ||
- old_period != s->period) {
- DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, "
- "period scaled from %d to %d\n", old_irq_coalesced,
- s->irq_coalesced, old_period, s->period);
- rtc_coalesced_timer_update(s);
- }
+ rtc_coalesced_timer_update(s);
} else {
/*
* no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
@@ -883,6 +880,7 @@ static void rtc_reset(void *opaque)
if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
s->irq_coalesced = 0;
s->irq_reinject_on_ack_count = 0;
+ rtc_coalesced_timer_update(s);
}
}
--
1.8.3.1
>From c722009caa5d4959499a89d63d74082164385f45 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonz...@redhat.com>
Date: Mon, 18 Nov 2019 12:04:43 +0100
Subject: [PATCH 4/5] mc146818rtc: reorganize arguments of
periodic_timer_update
This is mostly preparation for the next patch. Because the next patch
will pass the number of lost 32 kHz ticks to periodic_timer_update,
it makes sense that the current time is also passed in 32 kHz units.
This makes calling periodic_timer_update from cmos_ioport_write a
bit unwieldy, so extract periodic_timer_reconfigure.
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
hw/rtc/mc146818rtc.c | 38 ++++++++++++++++++++++++++------------
1 file changed, 26 insertions(+), 12 deletions(-)
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 47d966c..8a9e004 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -163,15 +163,23 @@ static uint32_t rtc_periodic_clock_ticks(RTCState *s)
return periodic_period_to_clock(period_code);
}
+static uint32_t rtc_periodic_clock_get_ticks(void)
+{
+ int64_t current_time = qemu_clock_get_ns(rtc_clock);
+
+ /* compute 32 khz clock */
+ return muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
+}
+
/*
* handle periodic timer. @old_period indicates the periodic timer update
* is just due to period adjustment.
*/
static void
-periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
+periodic_timer_update(RTCState *s, int64_t cur_clock, uint32_t old_period)
{
uint32_t period;
- int64_t cur_clock, next_irq_clock, lost_clock = 0;
+ int64_t next_irq_clock, lost_clock = 0;
period = rtc_periodic_clock_ticks(s);
s->period = period;
@@ -182,10 +190,6 @@ periodic_timer_update(RTCState *s, int64_t current_time,
uint32_t old_period)
return;
}
- /* compute 32 khz clock */
- cur_clock =
- muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
-
/*
* if the periodic timer's update is due to period re-configuration,
* we should count the clock since last interrupt.
@@ -239,11 +243,23 @@ periodic_timer_update(RTCState *s, int64_t current_time,
uint32_t old_period)
timer_mod(s->periodic_timer, s->next_periodic_time);
}
+static void
+periodic_timer_reconfigure(RTCState *s, uint32_t old_period)
+{
+ int64_t cur_clock = rtc_periodic_clock_get_ticks();
+
+ periodic_timer_update(s, cur_clock, old_period);
+}
+
static void rtc_periodic_timer(void *opaque)
{
RTCState *s = opaque;
+ int64_t last_periodic_clock;
+
+ last_periodic_clock =
+ muldiv64(s->next_periodic_time, RTC_CLOCK_RATE,
NANOSECONDS_PER_SECOND);
- periodic_timer_update(s, s->next_periodic_time, 0);
+ periodic_timer_update(s, last_periodic_clock, 0);
s->cmos_data[RTC_REG_C] |= REG_C_PF;
if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
@@ -508,8 +524,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
(s->cmos_data[RTC_REG_A] & REG_A_UIP);
if (update_periodic_timer) {
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
- old_period);
+ periodic_timer_reconfigure(s, old_period);
}
check_update_timer(s);
@@ -547,8 +562,7 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
s->cmos_data[RTC_REG_B] = data;
if (update_periodic_timer) {
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
- old_period);
+ periodic_timer_reconfigure(s, old_period);
}
check_update_timer(s);
@@ -802,7 +816,7 @@ static int rtc_post_load(void *opaque, int version_id)
uint64_t now = qemu_clock_get_ns(rtc_clock);
if (now < s->next_periodic_time ||
now > (s->next_periodic_time + get_max_clock_jump())) {
- periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), 0);
+ periodic_timer_reconfigure(s, s->period);
}
}
--
1.8.3.1
>From 8994fed352d8147d2dea99710728cc15fb9f2cc2 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonz...@redhat.com>
Date: Mon, 18 Nov 2019 11:59:59 +0100
Subject: [PATCH 5/5] mc146818rtc: fix timer interrupt reinjection again
Commit 369b41359af46bded5799c9ef8be2b641d92e043 broke timer interrupt
reinjection when there is no period change by the guest. In that
case, old_period is 0, which ends up zeroing irq_coalesced (counter of
reinjected interrupts).
The consequence is Windows 7 is unable to synchronize time via NTP.
Easily reproducible by playing a fullscreen video with cirrus and VNC.
That part of periodic_timer_update that introduces the bug is
only needed due to reconfiguration of the period, so move it to
periodic_timer_reconfigure. In that path, old_period == 0 does mean
that the periodic timer was off.
Reported-by: Marcelo Tosatti <mtosa...@redhat.com>
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
hw/rtc/mc146818rtc.c | 72 +++++++++++++++++++++++++---------------------------
1 file changed, 34 insertions(+), 38 deletions(-)
diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 8a9e004..823f706 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -171,44 +171,10 @@ static uint32_t rtc_periodic_clock_get_ticks(void)
return muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
}
-/*
- * handle periodic timer. @old_period indicates the periodic timer update
- * is just due to period adjustment.
- */
static void
-periodic_timer_update(RTCState *s, int64_t cur_clock, uint32_t old_period)
+periodic_timer_update(RTCState *s, int64_t cur_clock, int64_t lost_clock)
{
- uint32_t period;
- int64_t next_irq_clock, lost_clock = 0;
-
- period = rtc_periodic_clock_ticks(s);
- s->period = period;
-
- if (!period) {
- s->irq_coalesced = 0;
- timer_del(s->periodic_timer);
- return;
- }
-
- /*
- * if the periodic timer's update is due to period re-configuration,
- * we should count the clock since last interrupt.
- */
- if (old_period) {
- int64_t last_periodic_clock, next_periodic_clock;
-
- next_periodic_clock = muldiv64(s->next_periodic_time,
- RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
- last_periodic_clock = next_periodic_clock - old_period;
- lost_clock = cur_clock - last_periodic_clock;
- assert(lost_clock >= 0);
-
- if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
- lost_clock += s->irq_coalesced * old_period;
- s->irq_coalesced = 0;
- timer_del(s->coalesced_timer);
- }
- }
+ int64_t next_irq_clock;
/*
* s->irq_coalesced can change for two reasons:
@@ -243,23 +209,53 @@ periodic_timer_update(RTCState *s, int64_t cur_clock,
uint32_t old_period)
timer_mod(s->periodic_timer, s->next_periodic_time);
}
+/* adjust periodic timer on period adjustment */
static void
periodic_timer_reconfigure(RTCState *s, uint32_t old_period)
{
int64_t cur_clock = rtc_periodic_clock_get_ticks();
+ int64_t lost_clock = 0;
- periodic_timer_update(s, cur_clock, old_period);
+ s->period = rtc_periodic_clock_ticks(s);
+ if (!s->period) {
+ s->irq_coalesced = 0;
+ timer_del(s->periodic_timer);
+ return;
+ }
+
+ /*
+ * if the periodic timer was active before, account the time since
+ * the last interrupt.
+ */
+ if (old_period) {
+ int64_t last_periodic_clock, next_periodic_clock;
+
+ next_periodic_clock = muldiv64(s->next_periodic_time,
+ RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
+ last_periodic_clock = next_periodic_clock - old_period;
+ lost_clock = cur_clock - last_periodic_clock;
+ assert(lost_clock >= 0);
+
+ if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) {
+ lost_clock += s->irq_coalesced * old_period;
+ s->irq_coalesced = 0;
+ timer_del(s->coalesced_timer);
+ }
+ }
+
+ periodic_timer_update(s, cur_clock, lost_clock);
}
static void rtc_periodic_timer(void *opaque)
{
RTCState *s = opaque;
+ int64_t cur_clock = rtc_periodic_clock_get_ticks();
int64_t last_periodic_clock;
last_periodic_clock =
muldiv64(s->next_periodic_time, RTC_CLOCK_RATE,
NANOSECONDS_PER_SECOND);
- periodic_timer_update(s, last_periodic_clock, 0);
+ periodic_timer_update(s, cur_clock, cur_clock - last_periodic_clock);
s->cmos_data[RTC_REG_C] |= REG_C_PF;
if (s->cmos_data[RTC_REG_B] & REG_B_PIE) {
s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
--
1.8.3.1
