From: Xiao Guangrong <guangrong.xiao@gmail.com> To: Paolo Bonzini <pbonzini@redhat.com>, mst@redhat.com, mtosatti@redhat.com Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, yunfangtai@tencent.com, Xiao Guangrong <xiaoguangrong@tencent.com> Subject: Re: [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update Date: Thu, 4 May 2017 11:25:11 +0800 [thread overview] Message-ID: <132f19ec-7f48-308e-29b1-4ddd1dbb6188@gmail.com> (raw) In-Reply-To: <7df8de22-9ee9-00b5-f21b-ad7ec27f63c3@redhat.com> On 05/03/2017 11:39 PM, Paolo Bonzini wrote: > > > On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote: >> From: Xiao Guangrong <xiaoguangrong@tencent.com> >> >> Move the x86 specific code in periodic_timer_update() to a common place, >> the actual logic is not changed >> >> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> >> --- >> hw/timer/mc146818rtc.c | 112 +++++++++++++++++++++++++++++-------------------- >> 1 file changed, 66 insertions(+), 46 deletions(-) >> >> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c >> index 3bf559d..d7b7c56 100644 >> --- a/hw/timer/mc146818rtc.c >> +++ b/hw/timer/mc146818rtc.c >> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque) >> >> rtc_coalesced_timer_update(s); >> } >> + >> +static int64_t >> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock) >> +{ >> + if (period != s->period) { >> + int64_t scale_lost_clock; >> + int current_irq_coalesced = s->irq_coalesced; >> + >> + s->irq_coalesced = (current_irq_coalesced * s->period) / period; >> + >> + /* >> + * calculate the lost clock after it is scaled which should be >> + * compensated in the next interrupt. >> + */ >> + scale_lost_clock = current_irq_coalesced * s->period - >> + s->irq_coalesced * period; >> + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks " >> + "are compensated.\n", current_irq_coalesced, >> + s->irq_coalesced, scale_lost_clock); >> + lost_clock += scale_lost_clock; >> + s->period = period; > > This should be moved up to the caller. It should not. As you pointed out below, all these code are only needed for LOST_TICK_POLICY_SLEW that is x86 specific. Or use "if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW)" without "#ifdef TARGET_I386" is acceptable as only x86 can make it SLEW, Unnecessary branch checks will little slow down other architectures, but i think it is acceptable, right? :) > > Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is > zero. So I *think* what you get is equivalent to > > if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) { > return; > } > > /* ... comment ... */ > lost_interrupt = (s->irq_coalesced * s->period) / period; > lost_clock += (s->irq_coalesced * s->period) % period; > lost_interrupt += lost_clock / period; > lost_clock %= period; > > s->irq_coalesced = load_interrupt; > rtc_coalesced_timer_update(s); > > or equivalently: > > lost_clock += s->irq_coalesced * s->period; > > s->irq_coalesced = lost_clock / period; > lost_clock %= period; > rtc_coalesced_timer_update(s); > Exactly right, it is much better, will apply it. > I think you should probably merge these three patches and document the > resulting logic, because it's simpler than building it a patch at a time. Okay, i will consider it carefully in the next version. Thank you, Paolo!
WARNING: multiple messages have this Message-ID (diff)
From: Xiao Guangrong <guangrong.xiao@gmail.com> To: Paolo Bonzini <pbonzini@redhat.com>, mst@redhat.com, mtosatti@redhat.com Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, yunfangtai@tencent.com, Xiao Guangrong <xiaoguangrong@tencent.com> Subject: Re: [Qemu-devel] [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update Date: Thu, 4 May 2017 11:25:11 +0800 [thread overview] Message-ID: <132f19ec-7f48-308e-29b1-4ddd1dbb6188@gmail.com> (raw) In-Reply-To: <7df8de22-9ee9-00b5-f21b-ad7ec27f63c3@redhat.com> On 05/03/2017 11:39 PM, Paolo Bonzini wrote: > > > On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote: >> From: Xiao Guangrong <xiaoguangrong@tencent.com> >> >> Move the x86 specific code in periodic_timer_update() to a common place, >> the actual logic is not changed >> >> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> >> --- >> hw/timer/mc146818rtc.c | 112 +++++++++++++++++++++++++++++-------------------- >> 1 file changed, 66 insertions(+), 46 deletions(-) >> >> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c >> index 3bf559d..d7b7c56 100644 >> --- a/hw/timer/mc146818rtc.c >> +++ b/hw/timer/mc146818rtc.c >> @@ -144,6 +144,63 @@ static void rtc_coalesced_timer(void *opaque) >> >> rtc_coalesced_timer_update(s); >> } >> + >> +static int64_t >> +arch_periodic_timer_update(RTCState *s, int period, int64_t lost_clock) >> +{ >> + if (period != s->period) { >> + int64_t scale_lost_clock; >> + int current_irq_coalesced = s->irq_coalesced; >> + >> + s->irq_coalesced = (current_irq_coalesced * s->period) / period; >> + >> + /* >> + * calculate the lost clock after it is scaled which should be >> + * compensated in the next interrupt. >> + */ >> + scale_lost_clock = current_irq_coalesced * s->period - >> + s->irq_coalesced * period; >> + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks " >> + "are compensated.\n", current_irq_coalesced, >> + s->irq_coalesced, scale_lost_clock); >> + lost_clock += scale_lost_clock; >> + s->period = period; > > This should be moved up to the caller. It should not. As you pointed out below, all these code are only needed for LOST_TICK_POLICY_SLEW that is x86 specific. Or use "if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW)" without "#ifdef TARGET_I386" is acceptable as only x86 can make it SLEW, Unnecessary branch checks will little slow down other architectures, but i think it is acceptable, right? :) > > Also, if s->lost_tick_policy is not SLEW, s->irq_coalesced on input is > zero. So I *think* what you get is equivalent to > > if (s->lost_tick_policy != LOST_TICK_POLICY_SLEW) { > return; > } > > /* ... comment ... */ > lost_interrupt = (s->irq_coalesced * s->period) / period; > lost_clock += (s->irq_coalesced * s->period) % period; > lost_interrupt += lost_clock / period; > lost_clock %= period; > > s->irq_coalesced = load_interrupt; > rtc_coalesced_timer_update(s); > > or equivalently: > > lost_clock += s->irq_coalesced * s->period; > > s->irq_coalesced = lost_clock / period; > lost_clock %= period; > rtc_coalesced_timer_update(s); > Exactly right, it is much better, will apply it. > I think you should probably merge these three patches and document the > resulting logic, because it's simpler than building it a patch at a time. Okay, i will consider it carefully in the next version. Thank you, Paolo!
next prev parent reply other threads:[~2017-05-04 3:25 UTC|newest] Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-04-12 9:51 [PATCH 0/5] mc146818rtc: fix Windows VM clock faster guangrong.xiao 2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao 2017-04-12 9:51 ` [PATCH 1/5] mc146818rtc: update periodic timer only if it is needed guangrong.xiao 2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao 2017-05-03 15:42 ` Paolo Bonzini 2017-05-03 15:42 ` [Qemu-devel] " Paolo Bonzini 2017-05-04 3:27 ` Xiao Guangrong 2017-05-04 3:27 ` [Qemu-devel] " Xiao Guangrong 2017-04-12 9:51 ` [PATCH 2/5] mc146818rtc: fix clock lost after scaling coalesced irq guangrong.xiao 2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao 2017-05-03 15:15 ` Paolo Bonzini 2017-05-03 15:15 ` [Qemu-devel] " Paolo Bonzini 2017-05-04 2:51 ` Xiao Guangrong 2017-05-04 2:51 ` [Qemu-devel] " Xiao Guangrong 2017-04-12 9:51 ` [PATCH 3/5] mc146818rtc: properly count the time for the next interrupt guangrong.xiao 2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao 2017-05-03 15:32 ` Paolo Bonzini 2017-05-03 15:32 ` [Qemu-devel] " Paolo Bonzini 2017-05-04 2:54 ` Xiao Guangrong 2017-05-04 2:54 ` [Qemu-devel] " Xiao Guangrong 2017-05-04 12:02 ` Paolo Bonzini 2017-05-04 12:02 ` [Qemu-devel] " Paolo Bonzini 2017-04-12 9:51 ` [PATCH 4/5] mc146818rtc: move x86 specific code out of periodic_timer_update guangrong.xiao 2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao 2017-05-03 15:39 ` Paolo Bonzini 2017-05-03 15:39 ` [Qemu-devel] " Paolo Bonzini 2017-05-04 3:25 ` Xiao Guangrong [this message] 2017-05-04 3:25 ` Xiao Guangrong 2017-05-04 7:08 ` Paolo Bonzini 2017-05-04 7:08 ` [Qemu-devel] " Paolo Bonzini 2017-04-12 9:51 ` [PATCH 5/5] mc146818rtc: embrace all x86 specific code guangrong.xiao 2017-04-12 9:51 ` [Qemu-devel] " guangrong.xiao 2017-04-13 6:37 ` [PATCH 0/5] mc146818rtc: fix Windows VM clock faster Paolo Bonzini 2017-04-13 6:37 ` [Qemu-devel] " Paolo Bonzini 2017-04-13 8:39 ` Xiao Guangrong 2017-04-13 8:39 ` [Qemu-devel] " Xiao Guangrong 2017-04-13 8:52 ` Xiao Guangrong 2017-04-13 8:52 ` [Qemu-devel] " Xiao Guangrong 2017-04-13 9:05 ` 答复: " Zhanghailiang 2017-04-13 9:05 ` [Qemu-devel] " Zhanghailiang 2017-04-13 9:18 ` Xiao Guangrong 2017-04-13 9:18 ` [Qemu-devel] " Xiao Guangrong 2017-04-13 9:29 ` Hailiang Zhang 2017-04-13 9:29 ` [Qemu-devel] " Hailiang Zhang 2017-04-13 9:35 ` Xiao Guangrong 2017-04-13 9:35 ` [Qemu-devel] " Xiao Guangrong 2017-04-13 9:38 ` Hailiang Zhang 2017-04-13 9:38 ` [Qemu-devel] " Hailiang Zhang 2017-04-19 2:02 ` Xiao Guangrong 2017-04-19 2:02 ` [Qemu-devel] " Xiao Guangrong 2017-04-19 10:41 ` Hailiang Zhang 2017-04-19 10:41 ` [Qemu-devel] " Hailiang Zhang 2017-04-19 11:13 ` Xiao Guangrong 2017-04-19 11:13 ` [Qemu-devel] " Xiao Guangrong 2017-04-19 16:44 ` Paolo Bonzini 2017-04-19 16:44 ` [Qemu-devel] " Paolo Bonzini 2017-04-14 5:09 ` Paolo Bonzini 2017-04-14 5:09 ` [Qemu-devel] " Paolo Bonzini 2017-04-14 6:07 ` Xiao Guangrong
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=132f19ec-7f48-308e-29b1-4ddd1dbb6188@gmail.com \ --to=guangrong.xiao@gmail.com \ --cc=kvm@vger.kernel.org \ --cc=mst@redhat.com \ --cc=mtosatti@redhat.com \ --cc=pbonzini@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=xiaoguangrong@tencent.com \ --cc=yunfangtai@tencent.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.