Re: [PATCH v2] mc146818rtc: fix timer interrupt reinjection

[Marcelo Tosatti]

On Sun, Nov 17, 2019 at 11:12:43AM +0100, Paolo Bonzini wrote:
> On 17/11/19 05:31, Alex Williamson wrote:
> > The 'merge' option gives me a similar error.  The 'delay' option is
> > the only other choice where I can actually start the VM, but this
> > results in the commandline:
> > 
> > -rtc base=localtime
> > 
> > (no driftfix specified)
> 
> none is the default, so that's okay.
> 
> > This does appear to resolve the issue, but of course compatibility
> > with existing configurations has regressed. Thanks,
> 
> Yeah, I guess this was just a suggestion to double-check the cause of 
> the regression.
> 
> The problem could be that periodic_timer_update is using old_period == 0 
> for two cases: no period change, and old period was 0 (periodic timer 
> off).


> Something like the following distinguishes the two cases by always using
> s->period (currently it was only used for driftfix=slew) and passing
> s->period instead of 0 when there is no period change.
> 
> More cleanups are possible, but this is the smallest patch that implements
> the idea.  The first patch is big but, indentation changes aside, it's
> moving a single closed brace.
> 
> Alex/Marcelo, can you check if it fixes both of your test cases?

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

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

Alex perhaps you can give it a try?

--- hw/rtc/mc146818rtc.c.orig   2019-11-18 19:16:49.077479836 -0200
+++ hw/rtc/mc146818rtc.c        2019-11-18 19:22:35.706803090 -0200
@@ -168,7 +168,7 @@
  * 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 current_time, uint32_t old_period, 
bool period_change)
 {
     uint32_t period;
     int64_t cur_clock, next_irq_clock, lost_clock = 0;
@@ -190,7 +190,7 @@
      * if the periodic timer's update is due to period re-configuration,
      * we should count the clock since last interrupt.
      */
-    if (old_period) {
+    if (old_period && period_change) {
         int64_t last_periodic_clock, next_periodic_clock;
 
         next_periodic_clock = muldiv64(s->next_periodic_time,
@@ -246,7 +246,7 @@
 {
     RTCState *s = opaque;
 
-    periodic_timer_update(s, s->next_periodic_time, s->period);
+    periodic_timer_update(s, s->next_periodic_time, s->period, false);
     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;
@@ -512,7 +512,7 @@
 
             if (update_periodic_timer) {
                 periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
-                                      old_period);
+                                      old_period, true);
             }
 
             check_update_timer(s);
@@ -551,7 +551,7 @@
 
             if (update_periodic_timer) {
                 periodic_timer_update(s, qemu_clock_get_ns(rtc_clock),
-                                      old_period);
+                                      old_period, true);
             }
 
             check_update_timer(s);
@@ -805,7 +805,7 @@
         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), s->period);
+            periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), s->period, 
false);
         }
     }