On Mon, 18 Nov 2019 19:44:30 -0200
Marcelo Tosatti <mtosa...@redhat.com> 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);
> }
> }
>
