[Qemu-devel] [PATCH] vl.c: remove init_clocks call from main

[Qemu-devel] [PATCH] vl.c: remove init_clocks call from main

[Kirill Batuzov]

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.

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);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] vl.c: remove init_clocks call from main

[Alex Bligh]

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.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] vl.c: remove init_clocks call from main

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] vl.c: remove init_clocks call from main

[Alex Bligh]

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>

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] vl.c: remove init_clocks call from main

[Stefan Hajnoczi]

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