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

Submitted by Jan Kiszka on Jan. 27, 2011, 1:09 p.m.

Details

Message ID 34720dd344455d0abee575d399caedebcc099e5a.1296133797.git.jan.kiszka@siemens.com
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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