| Message ID | 1327166033-17922-1-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Michael Roth wrote: > In some cases initializing the alarm timers can lead to non-negligable > overhead from programs that link against qemu-tool.o. At least, > setting a max-resolution WinMM alarm timer via mm_start_timer() (the > current default for Windows) can increase the "tick rate" on Windows > OSs and affect frequency scaling, and in the case of tools that run > in guest OSs such has qemu-ga, the impact can be fairly dramatic > (+20%/20% user/sys time on a core 2 processor was observed from an idle > Windows XP guest). > > This patch doesn't address the issue directly (not sure what a good > solution would be for Windows, or what other situations it might be > noticeable), Is this a timer that need to fire soon after setting, every time? I wonder if a different kind of Windows timer, lower-resolution, could be used if the timeout is longer. If it has insufficient resolution, it could be set to trigger a little early, then set a high-resolution timer at that point. Maybe that could help for Linux CONFIG_NOHZ guests? -- Jamie
On 01/21/2012 09:39 PM, Jamie Lokier wrote: > Is this a timer that need to fire soon after setting, every time? > > I wonder if a different kind of Windows timer, lower-resolution, could > be used if the timeout is longer. If it has insufficient resolution, > it could be set to trigger a little early, then set a high-resolution > timer at that point. > > Maybe that could help for Linux CONFIG_NOHZ guests? No, it's an implementation detail of Windows multimedia timers. Just enabling them apparently burns CPU. There is another kind of timers for Windows but it didn't work reliably. Finding out why would be the right fix, but anyway Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Paolo
On 01/22/2012 06:32 AM, Paolo Bonzini wrote: > On 01/21/2012 09:39 PM, Jamie Lokier wrote: >> Is this a timer that need to fire soon after setting, every time? >> >> I wonder if a different kind of Windows timer, lower-resolution, could >> be used if the timeout is longer. If it has insufficient resolution, >> it could be set to trigger a little early, then set a high-resolution >> timer at that point. >> >> Maybe that could help for Linux CONFIG_NOHZ guests? > > No, it's an implementation detail of Windows multimedia timers. Just > enabling them apparently burns CPU. > > There is another kind of timers for Windows but it didn't work reliably. > Finding out why would be the right fix, but anyway I'd looked into timer queues, which the developer docs suggested had deprecated the use of mm timers, but I came across this which I figured was why mm was preferred (%30 average error for a 50ms periodic/oneshot) by the QEMU code: http://www.virtualdub.org/blog/pivot/entry.php?id=272 Apparently Windows 7 has some fixes for drift that makes them reasonable for a clock source, but still fairly low-res for an event timer. I suppose we could use some historical timer data to adaptively determine a reasonable minimum resolution to set...basically if the last timer fired X ns ago, we'd configure the resolution to be 1/10th of that periodic or whatever, and start them off low-res as James suggested to avoid unnecessary CPU burn, but it's a pretty loose heuristic. > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > Paolo > >
On 01/23/2012 01:12 AM, Michael Roth wrote: > I'd looked into timer queues, which the developer docs suggested had > deprecated the use of mm timers, but I came across this which I figured > was why mm was preferred (%30 average error for a 50ms periodic/oneshot) > by the QEMU code: > > http://www.virtualdub.org/blog/pivot/entry.php?id=272 > > Apparently Windows 7 has some fixes for drift that makes them reasonable > for a clock source, but still fairly low-res for an event timer. On the other hand, timeBeginPeriod affect basically all time sources, not just MM timers. Using timer queues together with timeBeginPeriod might work, after all. However, it's not too interesting since timeBeginPeriod is what causes the additional CPU usage. What is interesting is to move timeBeginPeriod/timeEndPeriod from qemu-timer.c elsewhere so that only QEMU uses it. The tools do not need precise timekeeping anyway. But while that would be good to have later when more tools use the shared main loop, your patch is still good for now. Paolo
On Sat, Jan 21, 2012 at 11:13:53AM -0600, Michael Roth wrote: > In some cases initializing the alarm timers can lead to non-negligable > overhead from programs that link against qemu-tool.o. At least, > setting a max-resolution WinMM alarm timer via mm_start_timer() (the > current default for Windows) can increase the "tick rate" on Windows > OSs and affect frequency scaling, and in the case of tools that run > in guest OSs such has qemu-ga, the impact can be fairly dramatic > (+20%/20% user/sys time on a core 2 processor was observed from an idle > Windows XP guest). > > This patch doesn't address the issue directly (not sure what a good > solution would be for Windows, or what other situations it might be > noticeable), but it at least limits the scope of the issue to programs > that "opt-in" to using the main-loop.c functions by only enabling alarm > timers when qemu_init_main_loop() is called, which is already required > to make use of those facilities, so existing users shouldn't be > affected. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > main-loop.c | 2 +- > main-loop.h | 12 ++++++++++++ > qemu-tool.c | 3 ++- > vl.c | 5 +++++ > 4 files changed, 20 insertions(+), 2 deletions(-) static struct qemu_alarm_timer alarm_timers[] = { #ifndef _WIN32 #ifdef __linux__ {"dynticks", dynticks_start_timer, dynticks_stop_timer, dynticks_rearm_timer}, #endif {"unix", unix_start_timer, unix_stop_timer, unix_rearm_timer}, #else {"mmtimer", mm_start_timer, mm_stop_timer, mm_rearm_timer}, {"dynticks", win32_start_timer, win32_stop_timer, win32_rearm_timer}, #endif It seems Windows host already has a "dynticks" implementation. Have you tried using this instead of "mmtimer"? mm_start_timer() is using 1 ms intervals! Stefan
On Sat, Jan 21, 2012 at 11:13:53AM -0600, Michael Roth wrote: > In some cases initializing the alarm timers can lead to non-negligable > overhead from programs that link against qemu-tool.o. At least, > setting a max-resolution WinMM alarm timer via mm_start_timer() (the > current default for Windows) can increase the "tick rate" on Windows > OSs and affect frequency scaling, and in the case of tools that run > in guest OSs such has qemu-ga, the impact can be fairly dramatic > (+20%/20% user/sys time on a core 2 processor was observed from an idle > Windows XP guest). > > This patch doesn't address the issue directly (not sure what a good > solution would be for Windows, or what other situations it might be > noticeable), but it at least limits the scope of the issue to programs > that "opt-in" to using the main-loop.c functions by only enabling alarm > timers when qemu_init_main_loop() is called, which is already required > to make use of those facilities, so existing users shouldn't be > affected. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > main-loop.c | 2 +- > main-loop.h | 12 ++++++++++++ > qemu-tool.c | 3 ++- > vl.c | 5 +++++ > 4 files changed, 20 insertions(+), 2 deletions(-) BTW the reason I suggest making Windows timers work efficiently ("dynticks") is because qemu-tool might legitimately want to make use of QEMU timers. Stefan
On 01/27/2012 06:46 AM, Stefan Hajnoczi wrote: > #ifndef _WIN32 > #ifdef __linux__ > {"dynticks", dynticks_start_timer, > dynticks_stop_timer, dynticks_rearm_timer}, > #endif > {"unix", unix_start_timer, unix_stop_timer, unix_rearm_timer}, > #else > {"mmtimer", mm_start_timer, mm_stop_timer, mm_rearm_timer}, > {"dynticks", win32_start_timer, win32_stop_timer, win32_rearm_timer}, > #endif > > It seems Windows host already has a "dynticks" implementation. Have you > tried using this instead of "mmtimer"? The dynticks Win32 timer doesn't boot Linux successfully, even though under Wine it works and it is actually more reliable than mmtimer (which is why I haven't thrown it away yet). > mm_start_timer() is using 1 ms intervals! No, it's setting a 1 ms system quantum via timeBeginPeriod. The actual implementation is using dynamic rather than periodic ticks (it has a rearm callback). We threw away periodic timers a few months ago. The problem is that Windows doesn't have something like Linux NOHZ and limits the timer resolution to the system quanta. That's 1 ms for mmtimer and 10 ms (the default) for dynticks right now. However, I suspect that the solution is to move timeBeginPeriod and timeEndPeriod from timer code to generic QEMU code, dynticks would start working on native Windows too. Besides possibly fixing QEMU, it would definitely fix Michael's problem, too. Tools do not need such a fine-grained timer, and shorter quanta cause the increased CPU usage that Michael observed. However, Michael's patch makes sense as a cleanup anyway. Since we have an initialization function, there's no need to have that _and_ a constructor. Paolo
On 01/21/2012 11:13 AM, Michael Roth wrote: > In some cases initializing the alarm timers can lead to non-negligable > overhead from programs that link against qemu-tool.o. At least, > setting a max-resolution WinMM alarm timer via mm_start_timer() (the > current default for Windows) can increase the "tick rate" on Windows > OSs and affect frequency scaling, and in the case of tools that run > in guest OSs such has qemu-ga, the impact can be fairly dramatic > (+20%/20% user/sys time on a core 2 processor was observed from an idle > Windows XP guest). > > This patch doesn't address the issue directly (not sure what a good > solution would be for Windows, or what other situations it might be > noticeable), but it at least limits the scope of the issue to programs > that "opt-in" to using the main-loop.c functions by only enabling alarm > timers when qemu_init_main_loop() is called, which is already required > to make use of those facilities, so existing users shouldn't be > affected. > > Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com> Applied. Thanks. Regards, Anthony Liguori > --- > main-loop.c | 2 +- > main-loop.h | 12 ++++++++++++ > qemu-tool.c | 3 ++- > vl.c | 5 +++++ > 4 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/main-loop.c b/main-loop.c > index 62d95b9..db23de0 100644 > --- a/main-loop.c > +++ b/main-loop.c > @@ -199,7 +199,7 @@ static int qemu_signal_init(void) > } > #endif > > -int qemu_init_main_loop(void) > +int main_loop_init(void) > { > int ret; > > diff --git a/main-loop.h b/main-loop.h > index f971013..4987041 100644 > --- a/main-loop.h > +++ b/main-loop.h > @@ -41,10 +41,22 @@ > * SIGUSR2, thread signals (SIGFPE, SIGILL, SIGSEGV, SIGBUS) and real-time > * signals if available. Remember that Windows in practice does not have > * signals, though. > + * > + * In the case of QEMU tools, this will also start/initialize timers. > */ > int qemu_init_main_loop(void); > > /** > + * main_loop_init: Initializes main loop > + * > + * Internal (but shared for compatibility reasons) initialization routine > + * for the main loop. This should not be used by applications directly, > + * use qemu_init_main_loop() instead. > + * > + */ > +int main_loop_init(void); > + > +/** > * main_loop_wait: Run one iteration of the main loop. > * > * If @nonblocking is true, poll for events, otherwise suspend until > diff --git a/qemu-tool.c b/qemu-tool.c > index 6b69668..183a583 100644 > --- a/qemu-tool.c > +++ b/qemu-tool.c > @@ -83,11 +83,12 @@ void qemu_clock_warp(QEMUClock *clock) > { > } > > -static void __attribute__((constructor)) init_main_loop(void) > +int qemu_init_main_loop(void) > { > init_clocks(); > init_timer_alarm(); > qemu_clock_enable(vm_clock, false); > + return main_loop_init(); > } > > void slirp_select_fill(int *pnfds, fd_set *readfds, > diff --git a/vl.c b/vl.c > index ba55b35..74a47e6 100644 > --- a/vl.c > +++ b/vl.c > @@ -2136,6 +2136,11 @@ static void free_and_trace(gpointer mem) > free(mem); > } > > +int qemu_init_main_loop(void) > +{ > + return main_loop_init(); > +} > + > int main(int argc, char **argv, char **envp) > { > const char *gdbstub_dev = NULL;
diff --git a/main-loop.c b/main-loop.c index 62d95b9..db23de0 100644 --- a/main-loop.c +++ b/main-loop.c @@ -199,7 +199,7 @@ static int qemu_signal_init(void) } #endif -int qemu_init_main_loop(void) +int main_loop_init(void) { int ret; diff --git a/main-loop.h b/main-loop.h index f971013..4987041 100644 --- a/main-loop.h +++ b/main-loop.h @@ -41,10 +41,22 @@ * SIGUSR2, thread signals (SIGFPE, SIGILL, SIGSEGV, SIGBUS) and real-time * signals if available. Remember that Windows in practice does not have * signals, though. + * + * In the case of QEMU tools, this will also start/initialize timers. */ int qemu_init_main_loop(void); /** + * main_loop_init: Initializes main loop + * + * Internal (but shared for compatibility reasons) initialization routine + * for the main loop. This should not be used by applications directly, + * use qemu_init_main_loop() instead. + * + */ +int main_loop_init(void); + +/** * main_loop_wait: Run one iteration of the main loop. * * If @nonblocking is true, poll for events, otherwise suspend until diff --git a/qemu-tool.c b/qemu-tool.c index 6b69668..183a583 100644 --- a/qemu-tool.c +++ b/qemu-tool.c @@ -83,11 +83,12 @@ void qemu_clock_warp(QEMUClock *clock) { } -static void __attribute__((constructor)) init_main_loop(void) +int qemu_init_main_loop(void) { init_clocks(); init_timer_alarm(); qemu_clock_enable(vm_clock, false); + return main_loop_init(); } void slirp_select_fill(int *pnfds, fd_set *readfds, diff --git a/vl.c b/vl.c index ba55b35..74a47e6 100644 --- a/vl.c +++ b/vl.c @@ -2136,6 +2136,11 @@ static void free_and_trace(gpointer mem) free(mem); } +int qemu_init_main_loop(void) +{ + return main_loop_init(); +} + int main(int argc, char **argv, char **envp) { const char *gdbstub_dev = NULL;
In some cases initializing the alarm timers can lead to non-negligable overhead from programs that link against qemu-tool.o. At least, setting a max-resolution WinMM alarm timer via mm_start_timer() (the current default for Windows) can increase the "tick rate" on Windows OSs and affect frequency scaling, and in the case of tools that run in guest OSs such has qemu-ga, the impact can be fairly dramatic (+20%/20% user/sys time on a core 2 processor was observed from an idle Windows XP guest). This patch doesn't address the issue directly (not sure what a good solution would be for Windows, or what other situations it might be noticeable), but it at least limits the scope of the issue to programs that "opt-in" to using the main-loop.c functions by only enabling alarm timers when qemu_init_main_loop() is called, which is already required to make use of those facilities, so existing users shouldn't be affected. Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> --- main-loop.c | 2 +- main-loop.h | 12 ++++++++++++ qemu-tool.c | 3 ++- vl.c | 5 +++++ 4 files changed, 20 insertions(+), 2 deletions(-)