[Qemu-devel] [PATCH] vl: Adjust the place of processing '-mon'

[Qemu-devel] [PATCH] vl: Adjust the place of processing '-mon'

[zhanghailiang]

From: Xiangyou Xie <xiexiangyou@huawei.com>

If VM is configured with large size of hugepage, when startup,
it will consume lots of time to zero the hugepage memory in the function
'os_mem_prealloc'.
Libvirtd will wait 30 seconds for qemu to establish the monitor,
If the timeout triggers, libvirtd will send TERM signal to kill qemu.

To solve the problem, adjust the processing of '-mon' to the ahead of '-object'.

Signed-off-by: Xiangyou Xie <xiexiangyou@huawei.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 vl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 86bdce0..d0c03fe 100644
--- a/vl.c
+++ b/vl.c
@@ -4000,6 +4000,10 @@ int main(int argc, char **argv, char **envp)
         exit(0);
     }
 
+    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
+        exit(1);
+    }
+
     if (qemu_opts_foreach(qemu_find_opts("object"),
                           object_create, NULL, 0) != 0) {
         exit(1);
@@ -4154,10 +4158,6 @@ int main(int argc, char **argv, char **envp)
 
     parse_numa_opts();
 
-    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
-        exit(1);
-    }
-
     if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
         exit(1);
     if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] vl: Adjust the place of processing '-mon'

[Paolo Bonzini]

On 04/03/2015 11:59, zhanghailiang wrote:
> From: Xiangyou Xie <xiexiangyou@huawei.com>
> 
> If VM is configured with large size of hugepage, when startup,
> it will consume lots of time to zero the hugepage memory in the function
> 'os_mem_prealloc'.
> Libvirtd will wait 30 seconds for qemu to establish the monitor,
> If the timeout triggers, libvirtd will send TERM signal to kill qemu.
> 
> To solve the problem, adjust the processing of '-mon' to the ahead of '-object'.
> 
> Signed-off-by: Xiangyou Xie <xiexiangyou@huawei.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  vl.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 86bdce0..d0c03fe 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4000,6 +4000,10 @@ int main(int argc, char **argv, char **envp)
>          exit(0);
>      }
>  
> +    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
> +        exit(1);
> +    }
> +
>      if (qemu_opts_foreach(qemu_find_opts("object"),
>                            object_create, NULL, 0) != 0) {
>          exit(1);
> @@ -4154,10 +4158,6 @@ int main(int argc, char **argv, char **envp)
>  
>      parse_numa_opts();
>  
> -    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
> -        exit(1);
> -    }
> -
>      if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>          exit(1);
>      if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] vl: Adjust the place of processing '-mon'

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> From: Xiangyou Xie <xiexiangyou@huawei.com>
>
> If VM is configured with large size of hugepage, when startup,
> it will consume lots of time to zero the hugepage memory in the function
> 'os_mem_prealloc'.
> Libvirtd will wait 30 seconds for qemu to establish the monitor,
> If the timeout triggers, libvirtd will send TERM signal to kill qemu.
>
> To solve the problem, adjust the processing of '-mon' to the ahead of '-object'.
>
> Signed-off-by: Xiangyou Xie <xiexiangyou@huawei.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  vl.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 86bdce0..d0c03fe 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4000,6 +4000,10 @@ int main(int argc, char **argv, char **envp)
>          exit(0);
>      }
>  
> +    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
> +        exit(1);
> +    }
> +
>      if (qemu_opts_foreach(qemu_find_opts("object"),
>                            object_create, NULL, 0) != 0) {
>          exit(1);
> @@ -4154,10 +4158,6 @@ int main(int argc, char **argv, char **envp)
>  
>      parse_numa_opts();
>  
> -    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
> -        exit(1);
> -    }
> -
>      if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>          exit(1);
>      if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)


Errors after monitor initialization look ugly when a monitor is on
stdio:

    $ qemu -nodefaults -monitor stdio -vga xxx
    QEMU 2.2.50 monitor - type 'help' for more information
    (qemu) Unknown vga type: xxx
    $

This patch initializes monitors earlier, thus makes more errors look
ugly.  Do we care?

If not: should we initialize them even earlier?  They depend on
character devices, so what about right after loop calling
chardev_init_func()?

Our startup is a big happy ad hoc mess.  A more organizes program would
read and check configuration first (quick, can fail), then allocate
resources (quick, can fail), then initialize (somewhat slow, failure
unlikely).

Re: [Qemu-devel] [PATCH] vl: Adjust the place of processing '-mon'

[zhanghailiang]

On 2015/3/5 1:02, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> From: Xiangyou Xie <xiexiangyou@huawei.com>
>>
>> If VM is configured with large size of hugepage, when startup,
>> it will consume lots of time to zero the hugepage memory in the function
>> 'os_mem_prealloc'.
>> Libvirtd will wait 30 seconds for qemu to establish the monitor,
>> If the timeout triggers, libvirtd will send TERM signal to kill qemu.
>>
>> To solve the problem, adjust the processing of '-mon' to the ahead of '-object'.
>>
>> Signed-off-by: Xiangyou Xie <xiexiangyou@huawei.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   vl.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 86bdce0..d0c03fe 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4000,6 +4000,10 @@ int main(int argc, char **argv, char **envp)
>>           exit(0);
>>       }
>>
>> +    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
>> +        exit(1);
>> +    }
>> +
>>       if (qemu_opts_foreach(qemu_find_opts("object"),
>>                             object_create, NULL, 0) != 0) {
>>           exit(1);
>> @@ -4154,10 +4158,6 @@ int main(int argc, char **argv, char **envp)
>>
>>       parse_numa_opts();
>>
>> -    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
>> -        exit(1);
>> -    }
>> -
>>       if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>>           exit(1);
>>       if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
>
>
> Errors after monitor initialization look ugly when a monitor is on
> stdio:
>
>      $ qemu -nodefaults -monitor stdio -vga xxx
>      QEMU 2.2.50 monitor - type 'help' for more information
>      (qemu) Unknown vga type: xxx
>      $
>
> This patch initializes monitors earlier, thus makes more errors look
> ugly.  Do we care?
>

Hmm, i don't think it will make any difference, no matter monitor or stdio,
it is all OK, it is just error reports, let people know what's wrong.

> If not: should we initialize them even earlier?  They depend on
> character devices, so what about right after loop calling
> chardev_init_func()?
>

This is reasonable, will fix it like that in v2, thanks.

> Our startup is a big happy ad hoc mess.  A more organizes program would

Yes, maybe some cleanup work should be done.

> read and check configuration first (quick, can fail), then allocate
> resources (quick, can fail), then initialize (somewhat slow, failure
> unlikely).
>
> .
>

Re: [Qemu-devel] [PATCH] vl: Adjust the place of processing '-mon'

[Peter Maydell]

On 5 March 2015 at 02:02, Markus Armbruster <armbru@redhat.com> wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> From: Xiangyou Xie <xiexiangyou@huawei.com>
>>
>> If VM is configured with large size of hugepage, when startup,
>> it will consume lots of time to zero the hugepage memory in the function
>> 'os_mem_prealloc'.
>> Libvirtd will wait 30 seconds for qemu to establish the monitor,
>> If the timeout triggers, libvirtd will send TERM signal to kill qemu.
>>
>> To solve the problem, adjust the processing of '-mon' to the ahead of '-object'.
>>
>> Signed-off-by: Xiangyou Xie <xiexiangyou@huawei.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>  vl.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 86bdce0..d0c03fe 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4000,6 +4000,10 @@ int main(int argc, char **argv, char **envp)
>>          exit(0);
>>      }
>>
>> +    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
>> +        exit(1);
>> +    }
>> +
>>      if (qemu_opts_foreach(qemu_find_opts("object"),
>>                            object_create, NULL, 0) != 0) {
>>          exit(1);
>> @@ -4154,10 +4158,6 @@ int main(int argc, char **argv, char **envp)
>>
>>      parse_numa_opts();
>>
>> -    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
>> -        exit(1);
>> -    }
>> -
>>      if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>>          exit(1);
>>      if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
>
>
> Errors after monitor initialization look ugly when a monitor is on
> stdio:
>
>     $ qemu -nodefaults -monitor stdio -vga xxx
>     QEMU 2.2.50 monitor - type 'help' for more information
>     (qemu) Unknown vga type: xxx
>     $
>
> This patch initializes monitors earlier, thus makes more errors look
> ugly.  Do we care?

Yeah, this doesn't seem great. Surely the actual problem here
is that we're doing something that takes 30 seconds to initialise
as part of our command line option parsing phase ?

-- PMM

Re: [Qemu-devel] [PATCH] vl: Adjust the place of processing '-mon'

[zhanghailiang]

On 2015/3/6 17:51, Peter Maydell wrote:
> On 5 March 2015 at 02:02, Markus Armbruster <armbru@redhat.com> wrote:
>> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>>
>>> From: Xiangyou Xie <xiexiangyou@huawei.com>
>>>
>>> If VM is configured with large size of hugepage, when startup,
>>> it will consume lots of time to zero the hugepage memory in the function
>>> 'os_mem_prealloc'.
>>> Libvirtd will wait 30 seconds for qemu to establish the monitor,
>>> If the timeout triggers, libvirtd will send TERM signal to kill qemu.
>>>
>>> To solve the problem, adjust the processing of '-mon' to the ahead of '-object'.
>>>
>>> Signed-off-by: Xiangyou Xie <xiexiangyou@huawei.com>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>>   vl.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 86bdce0..d0c03fe 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -4000,6 +4000,10 @@ int main(int argc, char **argv, char **envp)
>>>           exit(0);
>>>       }
>>>
>>> +    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
>>> +        exit(1);
>>> +    }
>>> +
>>>       if (qemu_opts_foreach(qemu_find_opts("object"),
>>>                             object_create, NULL, 0) != 0) {
>>>           exit(1);
>>> @@ -4154,10 +4158,6 @@ int main(int argc, char **argv, char **envp)
>>>
>>>       parse_numa_opts();
>>>
>>> -    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
>>> -        exit(1);
>>> -    }
>>> -
>>>       if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>>>           exit(1);
>>>       if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
>>
>>
>> Errors after monitor initialization look n a monitor is on
>> stdio:
>>
>>      $ qemu -nodefaults -monitor stdio -vga xxx
>>      QEMU 2.2.50 monitor - type 'help' for more information
>>      (qemu) Unknown vga type: xxx
>>      $
>>
>> This patch initializes monitors earlier, thus makes more errors look
>> ugly.  Do we care?
>
> Yeah, this doesn't seem great. Surely the actual problem here
> is that we're doing something that takes 30 seconds to initialise
> as part of our command line option parsing phase ?
>

Yes, it is the problem of memset() in os_mem_prealloc,
if we configure VM with large size memory (prealloc is also configured),
it will consume lots of time. we can certainly do this consuming time work later,
after command line option parsing phase, but this scenario is more easy.

> -- PMM
>
> .
>

Re: [Qemu-devel] [PATCH] vl: Adjust the place of processing '-mon'

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 March 2015 at 02:02, Markus Armbruster <armbru@redhat.com> wrote:
>> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>>
>>> From: Xiangyou Xie <xiexiangyou@huawei.com>
>>>
>>> If VM is configured with large size of hugepage, when startup,
>>> it will consume lots of time to zero the hugepage memory in the function
>>> 'os_mem_prealloc'.
>>> Libvirtd will wait 30 seconds for qemu to establish the monitor,
>>> If the timeout triggers, libvirtd will send TERM signal to kill qemu.
>>>
>>> To solve the problem, adjust the processing of '-mon' to the ahead of '-object'.
>>>
>>> Signed-off-by: Xiangyou Xie <xiexiangyou@huawei.com>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>>  vl.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 86bdce0..d0c03fe 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -4000,6 +4000,10 @@ int main(int argc, char **argv, char **envp)
>>>          exit(0);
>>>      }
>>>
>>> +    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
>>> +        exit(1);
>>> +    }
>>> +
>>>      if (qemu_opts_foreach(qemu_find_opts("object"),
>>>                            object_create, NULL, 0) != 0) {
>>>          exit(1);
>>> @@ -4154,10 +4158,6 @@ int main(int argc, char **argv, char **envp)
>>>
>>>      parse_numa_opts();
>>>
>>> -    if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 0) {
>>> -        exit(1);
>>> -    }
>>> -
>>>      if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>>>          exit(1);
>>>      if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
>>
>>
>> Errors after monitor initialization look ugly when a monitor is on
>> stdio:
>>
>>     $ qemu -nodefaults -monitor stdio -vga xxx
>>     QEMU 2.2.50 monitor - type 'help' for more information
>>     (qemu) Unknown vga type: xxx
>>     $
>>
>> This patch initializes monitors earlier, thus makes more errors look
>> ugly.  Do we care?
>
> Yeah, this doesn't seem great. Surely the actual problem here
> is that we're doing something that takes 30 seconds to initialise
> as part of our command line option parsing phase ?

Yep.  Quoting myself:

    Our startup is a big happy ad hoc mess.  A more organizes [sic]
    program would read and check configuration first (quick, can fail),
    then allocate resources (quick, can fail), then initialize (somewhat
    slow, failure unlikely).

Reorganizing everything to work like that is a huge task.

Until we get that (if ever!), all we can do is delay the most
egregiously slow initializations until after configuration checking and
monitor creation.

Re: [Qemu-devel] [PATCH] vl: Adjust the place of processing '-mon'

[Peter Maydell]

On 6 March 2015 at 21:05, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 5 March 2015 at 02:02, Markus Armbruster <armbru@redhat.com> wrote:
>>> This patch initializes monitors earlier, thus makes more errors look
>>> ugly.  Do we care?
>>
>> Yeah, this doesn't seem great. Surely the actual problem here
>> is that we're doing something that takes 30 seconds to initialise
>> as part of our command line option parsing phase ?
>
> Yep.  Quoting myself:
>
>     Our startup is a big happy ad hoc mess.  A more organizes [sic]
>     program would read and check configuration first (quick, can fail),
>     then allocate resources (quick, can fail), then initialize (somewhat
>     slow, failure unlikely).
>
> Reorganizing everything to work like that is a huge task.
>
> Until we get that (if ever!), all we can do is delay the most
> egregiously slow initializations until after configuration checking and
> monitor creation.

Yep, so we should delay the memset() til after all our init and
option parsing, rather than just move the monitor parsing/init
early.

-- PMM