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

[Alex Williamson <alex.williamson@redhat.com>]

On Sun, 17 Nov 2019 11:12:43 +0100
Paolo Bonzini <pbonzini@redhat.com> 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?

It resolves the majority of the regression, but I think there's still a
performance issue.  Passmark PerformanceTest in the guest shows a 5+%
decrease versus a straight revert.  Thanks,

Alex

> ------------- 8< ---------------
> From 48dc9d140c636067b8de1ab8e25b819151c83162 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@redhat.com>
> Date: Sun, 17 Nov 2019 10:07:38 +0100
> Subject: [PATCH 1/2] 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 <pbonzini@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 ee6bf82b40..9869dc5031 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);