On Sun, 17 Nov 2019 11:12:43 +0100
Paolo Bonzini <pbonz...@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 <pbonz...@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 <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 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);
