Patchwork [04/22] Leave inner main_loop faster on pending requests

login
register
mail settings
Submitter Jan Kiszka
Date Jan. 27, 2011, 1:09 p.m.
Message ID <34720dd344455d0abee575d399caedebcc099e5a.1296133797.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/80680/
State New
Headers show

Comments

Jan Kiszka - Jan. 27, 2011, 1:09 p.m.
If there is any pending request that requires us to leave the inner loop
if main_loop, makes sure we do this as soon as possible by enforcing
non-blocking IO processing.

At this change, move variable definitions out of the inner loop to
improve readability.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 vl.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)
Avi Kivity - Jan. 31, 2011, 9:52 a.m.
On 01/27/2011 03:09 PM, Jan Kiszka wrote:
> If there is any pending request that requires us to leave the inner loop
> if main_loop, makes sure we do this as soon as possible by enforcing
> non-blocking IO processing.
>
> At this change, move variable definitions out of the inner loop to
> improve readability.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>   vl.c |   11 +++++++----
>   1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 5fad700..2ebc55b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1384,18 +1384,21 @@ qemu_irq qemu_system_powerdown;
>
>   static void main_loop(void)
>   {
> +    bool nonblocking = false;
> +#ifdef CONFIG_PROFILER
> +    int64_t ti;
> +#endif
>       int r;
>
>       qemu_main_loop_start();
>
>       for (;;) {
>           do {
> -            bool nonblocking = false;
> -#ifdef CONFIG_PROFILER
> -            int64_t ti;
> -#endif
>   #ifndef CONFIG_IOTHREAD
>               nonblocking = cpu_exec_all();
> +            if (!vm_can_run()) {
> +                nonblocking = true;
> +            }

Doesn't this cause vmstop to spin?  We'll never execute 
main_loop_wait(false) if I read the code correctly?
Jan Kiszka - Jan. 31, 2011, 11:22 a.m.
On 2011-01-31 10:52, Avi Kivity wrote:
> On 01/27/2011 03:09 PM, Jan Kiszka wrote:
>> If there is any pending request that requires us to leave the inner loop
>> if main_loop, makes sure we do this as soon as possible by enforcing
>> non-blocking IO processing.
>>
>> At this change, move variable definitions out of the inner loop to
>> improve readability.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>   vl.c |   11 +++++++----
>>   1 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 5fad700..2ebc55b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1384,18 +1384,21 @@ qemu_irq qemu_system_powerdown;
>>
>>   static void main_loop(void)
>>   {
>> +    bool nonblocking = false;
>> +#ifdef CONFIG_PROFILER
>> +    int64_t ti;
>> +#endif
>>       int r;
>>
>>       qemu_main_loop_start();
>>
>>       for (;;) {
>>           do {
>> -            bool nonblocking = false;
>> -#ifdef CONFIG_PROFILER
>> -            int64_t ti;
>> -#endif
>>   #ifndef CONFIG_IOTHREAD
>>               nonblocking = cpu_exec_all();
>> +            if (!vm_can_run()) {
>> +                nonblocking = true;
>> +            }
> 
> Doesn't this cause vmstop to spin?  We'll never execute 
> main_loop_wait(false) if I read the code correctly?
> 

The code path is not changed, we just poll instead of wait in
main_loop_wait.

Also, I didn't get your error scenario yet. Even if we left the loop
here, what magic would main_loop_wait do to vmstop processing? The stop
request is handled outside the loop, that's why we should leave ASAP.

Jan
Avi Kivity - Jan. 31, 2011, 1:17 p.m.
On 01/31/2011 01:22 PM, Jan Kiszka wrote:
> On 2011-01-31 10:52, Avi Kivity wrote:
> >  On 01/27/2011 03:09 PM, Jan Kiszka wrote:
> >>  If there is any pending request that requires us to leave the inner loop
> >>  if main_loop, makes sure we do this as soon as possible by enforcing
> >>  non-blocking IO processing.
> >>
> >>  At this change, move variable definitions out of the inner loop to
> >>  improve readability.
> >>
> >>  Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >>  ---
> >>    vl.c |   11 +++++++----
> >>    1 files changed, 7 insertions(+), 4 deletions(-)
> >>
> >>  diff --git a/vl.c b/vl.c
> >>  index 5fad700..2ebc55b 100644
> >>  --- a/vl.c
> >>  +++ b/vl.c
> >>  @@ -1384,18 +1384,21 @@ qemu_irq qemu_system_powerdown;
> >>
> >>    static void main_loop(void)
> >>    {
> >>  +    bool nonblocking = false;
> >>  +#ifdef CONFIG_PROFILER
> >>  +    int64_t ti;
> >>  +#endif
> >>        int r;
> >>
> >>        qemu_main_loop_start();
> >>
> >>        for (;;) {
> >>            do {
> >>  -            bool nonblocking = false;
> >>  -#ifdef CONFIG_PROFILER
> >>  -            int64_t ti;
> >>  -#endif
> >>    #ifndef CONFIG_IOTHREAD
> >>                nonblocking = cpu_exec_all();
> >>  +            if (!vm_can_run()) {
> >>  +                nonblocking = true;
> >>  +            }
> >
> >  Doesn't this cause vmstop to spin?  We'll never execute
> >  main_loop_wait(false) if I read the code correctly?
> >
>
> The code path is not changed, we just poll instead of wait in
> main_loop_wait.

Where do we wait then?

> Also, I didn't get your error scenario yet. Even if we left the loop
> here, what magic would main_loop_wait do to vmstop processing? The stop
> request is handled outside the loop, that's why we should leave ASAP.

I'm just missing the alternate place where we sleep.  If there's no such 
place, we spin.
Jan Kiszka - Jan. 31, 2011, 2:32 p.m.
On 2011-01-31 14:17, Avi Kivity wrote:
> On 01/31/2011 01:22 PM, Jan Kiszka wrote:
>> On 2011-01-31 10:52, Avi Kivity wrote:
>>>  On 01/27/2011 03:09 PM, Jan Kiszka wrote:
>>>>  If there is any pending request that requires us to leave the inner loop
>>>>  if main_loop, makes sure we do this as soon as possible by enforcing
>>>>  non-blocking IO processing.
>>>>
>>>>  At this change, move variable definitions out of the inner loop to
>>>>  improve readability.
>>>>
>>>>  Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>>  ---
>>>>    vl.c |   11 +++++++----
>>>>    1 files changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>>  diff --git a/vl.c b/vl.c
>>>>  index 5fad700..2ebc55b 100644
>>>>  --- a/vl.c
>>>>  +++ b/vl.c
>>>>  @@ -1384,18 +1384,21 @@ qemu_irq qemu_system_powerdown;
>>>>
>>>>    static void main_loop(void)
>>>>    {
>>>>  +    bool nonblocking = false;
>>>>  +#ifdef CONFIG_PROFILER
>>>>  +    int64_t ti;
>>>>  +#endif
>>>>        int r;
>>>>
>>>>        qemu_main_loop_start();
>>>>
>>>>        for (;;) {
>>>>            do {
>>>>  -            bool nonblocking = false;
>>>>  -#ifdef CONFIG_PROFILER
>>>>  -            int64_t ti;
>>>>  -#endif
>>>>    #ifndef CONFIG_IOTHREAD
>>>>                nonblocking = cpu_exec_all();
>>>>  +            if (!vm_can_run()) {
>>>>  +                nonblocking = true;
>>>>  +            }
>>>
>>>  Doesn't this cause vmstop to spin?  We'll never execute
>>>  main_loop_wait(false) if I read the code correctly?
>>>
>>
>> The code path is not changed, we just poll instead of wait in
>> main_loop_wait.
> 
> Where do we wait then?
> 
>> Also, I didn't get your error scenario yet. Even if we left the loop
>> here, what magic would main_loop_wait do to vmstop processing? The stop
>> request is handled outside the loop, that's why we should leave ASAP.
> 
> I'm just missing the alternate place where we sleep.  If there's no such 
> place, we spin.
> 

Probably you are misled by the name of vm_can_run. It should better be
renamed to requests_pending or something like that - iow, it is only
true if some request is pending and not if just the vm is in stop mode.

Jan

Patch

diff --git a/vl.c b/vl.c
index 5fad700..2ebc55b 100644
--- a/vl.c
+++ b/vl.c
@@ -1384,18 +1384,21 @@  qemu_irq qemu_system_powerdown;
 
 static void main_loop(void)
 {
+    bool nonblocking = false;
+#ifdef CONFIG_PROFILER
+    int64_t ti;
+#endif
     int r;
 
     qemu_main_loop_start();
 
     for (;;) {
         do {
-            bool nonblocking = false;
-#ifdef CONFIG_PROFILER
-            int64_t ti;
-#endif
 #ifndef CONFIG_IOTHREAD
             nonblocking = cpu_exec_all();
+            if (!vm_can_run()) {
+                nonblocking = true;
+            }
 #endif
 #ifdef CONFIG_PROFILER
             ti = profile_getclock();