| Message ID | 1399381193-2662-1-git-send-email-batuzovk@ispras.ru |
|---|---|
| State | New |
| Headers | show |
On 6 May 2014, at 13:59, Kirill Batuzov wrote: > Clocks are initialized in qemu_init_main_loop. They are not needed before it. > Initializing them twice is not only unnecessary but is harmful: it results in > memory leak and potentially can lead to a situation where different parts of > QEMU use different sets of timers. > > To avoid it remove init_clocks call from main and add an assertion to > qemu_clock_init that corresponding clock has not been initialized yet. > > Signed-off-by: Kirill Batuzov <batuzovk@ispras.ru> > --- > qemu-timer.c | 3 +++ > vl.c | 1 - > 2 files changed, 3 insertions(+), 1 deletion(-) > > The init_clocks call was added to qemu_init_main_loop in commit > 172061a0a0d98c974ea8d5ed715195237bc44225. init_clocks was made idempotent > in prior commit 744ca8e3754e6808c6b5331d287adc533fca0ad3. Back then it was > not possible to remove init_clocks call from main because rtc_clock variable > was of type QEMUClock * and was used in option processing. So clocks needed to > be initialized before command line options could be processed. > > Commit 7bf8fbde449600926370f744b2b3d9cc819ca74f removed idempotence property > from init_clocks. Commit 884f17c235095af99b92dd8cd10bb824a5b0f777 removed > the need to call init_clocks from main, but did not remove the call. So if I read this right, init_clocks() is now only done in vl.c. I think this is problematic, as qemu-img (for instance) still uses the mainloop (it calls qemu_init_main_loop() from main), but does not use vl.c as far as I know. Perhaps restoring the idempotence would be a better plan.
On 6 May 2014 16:57, Alex Bligh <alex@alex.org.uk> wrote: > > On 6 May 2014, at 13:59, Kirill Batuzov wrote: > >> Clocks are initialized in qemu_init_main_loop. They are not needed before it. >> Initializing them twice is not only unnecessary but is harmful: it results in >> memory leak and potentially can lead to a situation where different parts of >> QEMU use different sets of timers. >> >> To avoid it remove init_clocks call from main and add an assertion to >> qemu_clock_init that corresponding clock has not been initialized yet. >> >> Signed-off-by: Kirill Batuzov <batuzovk@ispras.ru> >> --- >> qemu-timer.c | 3 +++ >> vl.c | 1 - >> 2 files changed, 3 insertions(+), 1 deletion(-) > So if I read this right, init_clocks() is now only done in vl.c. No, you're misreading it -- the commit message says "remove init_clocks call from main" and you can see from the diffstat it's removed from vl.c. It's now done only in qemu_clock_init(). thanks -- PMM
On 6 May 2014, at 17:01, Peter Maydell wrote: >> >> So if I read this right, init_clocks() is now only done in vl.c. > > No, you're misreading it -- the commit message says "remove > init_clocks call from main" and you can see from the diffstat > it's removed from vl.c. It's now done only in qemu_clock_init(). Indeed. I get to wear the dunce's cap for a bit. Apologies. Twice, actually, for not removing it from main() in the first place. Reviewed-By: Alex Bligh <alex@alex.org.uk>
On Tue, May 06, 2014 at 04:59:53PM +0400, Kirill Batuzov wrote: > Clocks are initialized in qemu_init_main_loop. They are not needed before it. > Initializing them twice is not only unnecessary but is harmful: it results in > memory leak and potentially can lead to a situation where different parts of > QEMU use different sets of timers. > > To avoid it remove init_clocks call from main and add an assertion to > qemu_clock_init that corresponding clock has not been initialized yet. > > Signed-off-by: Kirill Batuzov <batuzovk@ispras.ru> > --- > qemu-timer.c | 3 +++ > vl.c | 1 - > 2 files changed, 3 insertions(+), 1 deletion(-) > > The init_clocks call was added to qemu_init_main_loop in commit > 172061a0a0d98c974ea8d5ed715195237bc44225. init_clocks was made idempotent > in prior commit 744ca8e3754e6808c6b5331d287adc533fca0ad3. Back then it was > not possible to remove init_clocks call from main because rtc_clock variable > was of type QEMUClock * and was used in option processing. So clocks needed to > be initialized before command line options could be processed. > > Commit 7bf8fbde449600926370f744b2b3d9cc819ca74f removed idempotence property > from init_clocks. Commit 884f17c235095af99b92dd8cd10bb824a5b0f777 removed > the need to call init_clocks from main, but did not remove the call. Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
diff --git a/qemu-timer.c b/qemu-timer.c index e15ce47..335a0e5 100644 --- a/qemu-timer.c +++ b/qemu-timer.c @@ -126,6 +126,9 @@ static void qemu_clock_init(QEMUClockType type) { QEMUClock *clock = qemu_clock_ptr(type); + /* Assert that the clock of type TYPE has not been initialized yet. */ + assert(main_loop_tlg.tl[type] == NULL); + clock->type = type; clock->enabled = true; clock->last = INT64_MIN; diff --git a/vl.c b/vl.c index 9975e5a..a903776 100644 --- a/vl.c +++ b/vl.c @@ -2999,7 +2999,6 @@ int main(int argc, char **argv, char **envp) runstate_init(); - init_clocks(); rtc_clock = QEMU_CLOCK_HOST; qemu_init_auxval(envp);
Clocks are initialized in qemu_init_main_loop. They are not needed before it. Initializing them twice is not only unnecessary but is harmful: it results in memory leak and potentially can lead to a situation where different parts of QEMU use different sets of timers. To avoid it remove init_clocks call from main and add an assertion to qemu_clock_init that corresponding clock has not been initialized yet. Signed-off-by: Kirill Batuzov <batuzovk@ispras.ru> --- qemu-timer.c | 3 +++ vl.c | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) The init_clocks call was added to qemu_init_main_loop in commit 172061a0a0d98c974ea8d5ed715195237bc44225. init_clocks was made idempotent in prior commit 744ca8e3754e6808c6b5331d287adc533fca0ad3. Back then it was not possible to remove init_clocks call from main because rtc_clock variable was of type QEMUClock * and was used in option processing. So clocks needed to be initialized before command line options could be processed. Commit 7bf8fbde449600926370f744b2b3d9cc819ca74f removed idempotence property from init_clocks. Commit 884f17c235095af99b92dd8cd10bb824a5b0f777 removed the need to call init_clocks from main, but did not remove the call.