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

[Alex Williamson]

On Mon, 18 Nov 2019 19:44:30 -0200
Marcelo Tosatti <address@hidden> wrote:

> 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?

Yes, Paolo's two patches plus this seems to resolve both the functional
and performance issues.  Thanks,

Alex
 
> --- 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);
>          }
>      }
>