[02/15] Refactor cpu_has_work/any_cpu_has_work in cpus.c

Submitted by Jan Kiszka on Feb. 7, 2011, 11:19 a.m.

Details

Message ID 1e7a4f5097aafb5da844c1b9ff8661539d0d10e8.1297077506.git.jan.kiszka@siemens.com
State New
Headers show

Commit Message

Jan Kiszka Feb. 7, 2011, 11:19 a.m.
Avoid duplicate use of the function name cpu_has_work, it's confusing.
Refactor cpu_has_work to cpu_is_idle and do the same with
any_cpu_has_work.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cpus.c |   43 +++++++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 20 deletions(-)

Comments

Marcelo Tosatti Feb. 8, 2011, 6:50 p.m.
On Mon, Feb 07, 2011 at 12:19:13PM +0100, Jan Kiszka wrote:
> Avoid duplicate use of the function name cpu_has_work, it's confusing.
> Refactor cpu_has_work to cpu_is_idle and do the same with
> any_cpu_has_work.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  cpus.c |   43 +++++++++++++++++++++++--------------------
>  1 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index d54ec7d..cd764f2 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -137,29 +137,30 @@ static int cpu_can_run(CPUState *env)
>      return 1;
>  }
>  
> -static int cpu_has_work(CPUState *env)
> +static bool cpu_is_idle(CPUState *env)
>  {
> -    if (env->stop)
> -        return 1;
> -    if (env->queued_work_first)
> -        return 1;
> -    if (env->stopped || !vm_running)
> -        return 0;
> -    if (!env->halted)
> -        return 1;
> -    if (qemu_cpu_has_work(env))
> -        return 1;
> -    return 0;
> +    if (env->stop || env->queued_work_first) {
> +        return false;
> +    }
> +    if (env->stopped || !vm_running) {
> +        return true;
> +    }
> +    if (!env->halted || qemu_cpu_has_work(env)) {
> +        return false;
> +    }
> +    return true;
>  }

Do you really find it easier to read evaluations grouped with || ? I
don't.

Sorry but the name change does not feel right either: CPU is still idle
if the vm is not running.
Jan Kiszka Feb. 9, 2011, 8:07 a.m.
On 2011-02-08 19:50, Marcelo Tosatti wrote:
> On Mon, Feb 07, 2011 at 12:19:13PM +0100, Jan Kiszka wrote:
>> Avoid duplicate use of the function name cpu_has_work, it's confusing.
>> Refactor cpu_has_work to cpu_is_idle and do the same with
>> any_cpu_has_work.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  cpus.c |   43 +++++++++++++++++++++++--------------------
>>  1 files changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index d54ec7d..cd764f2 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -137,29 +137,30 @@ static int cpu_can_run(CPUState *env)
>>      return 1;
>>  }
>>  
>> -static int cpu_has_work(CPUState *env)
>> +static bool cpu_is_idle(CPUState *env)
>>  {
>> -    if (env->stop)
>> -        return 1;
>> -    if (env->queued_work_first)
>> -        return 1;
>> -    if (env->stopped || !vm_running)
>> -        return 0;
>> -    if (!env->halted)
>> -        return 1;
>> -    if (qemu_cpu_has_work(env))
>> -        return 1;
>> -    return 0;
>> +    if (env->stop || env->queued_work_first) {
>> +        return false;
>> +    }
>> +    if (env->stopped || !vm_running) {
>> +        return true;
>> +    }
>> +    if (!env->halted || qemu_cpu_has_work(env)) {
>> +        return false;
>> +    }
>> +    return true;
>>  }
> 
> Do you really find it easier to read evaluations grouped with || ? I
> don't.

I do, specifically as the old version was even more confusing in that
important detail "return 0" vs. "return 1". But even the new benefits
from the grouping IMHO.

> 
> Sorry but the name change does not feel right either: CPU is still idle
> if the vm is not running.

But that's exactly what the function returns. Or is it confusing if we
are talking about the vcpu or the whole thread here? What about
"cpu_thread_is_idle" then?

Jan
Marcelo Tosatti Feb. 9, 2011, 1:54 p.m.
On Wed, Feb 09, 2011 at 09:07:50AM +0100, Jan Kiszka wrote:
> > Do you really find it easier to read evaluations grouped with || ? I
> > don't.
> 
> I do, specifically as the old version was even more confusing in that
> important detail "return 0" vs. "return 1". But even the new benefits
> from the grouping IMHO.

Well alright.

> > Sorry but the name change does not feel right either: CPU is still idle
> > if the vm is not running.
> 
> But that's exactly what the function returns. Or is it confusing if we
> are talking about the vcpu or the whole thread here? What about
> "cpu_thread_is_idle" then?

Yes thats better.

Patch hide | download patch | download mbox

diff --git a/cpus.c b/cpus.c
index d54ec7d..cd764f2 100644
--- a/cpus.c
+++ b/cpus.c
@@ -137,29 +137,30 @@  static int cpu_can_run(CPUState *env)
     return 1;
 }
 
-static int cpu_has_work(CPUState *env)
+static bool cpu_is_idle(CPUState *env)
 {
-    if (env->stop)
-        return 1;
-    if (env->queued_work_first)
-        return 1;
-    if (env->stopped || !vm_running)
-        return 0;
-    if (!env->halted)
-        return 1;
-    if (qemu_cpu_has_work(env))
-        return 1;
-    return 0;
+    if (env->stop || env->queued_work_first) {
+        return false;
+    }
+    if (env->stopped || !vm_running) {
+        return true;
+    }
+    if (!env->halted || qemu_cpu_has_work(env)) {
+        return false;
+    }
+    return true;
 }
 
-static int any_cpu_has_work(void)
+static bool all_cpus_idle(void)
 {
     CPUState *env;
 
-    for (env = first_cpu; env != NULL; env = env->next_cpu)
-        if (cpu_has_work(env))
-            return 1;
-    return 0;
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        if (!cpu_is_idle(env)) {
+            return false;
+        }
+    }
+    return true;
 }
 
 static void cpu_debug_handler(CPUState *env)
@@ -743,8 +744,9 @@  static void qemu_tcg_wait_io_event(void)
 {
     CPUState *env;
 
-    while (!any_cpu_has_work())
+    while (all_cpus_idle()) {
         qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
+    }
 
     qemu_mutex_unlock(&qemu_global_mutex);
 
@@ -765,8 +767,9 @@  static void qemu_tcg_wait_io_event(void)
 
 static void qemu_kvm_wait_io_event(CPUState *env)
 {
-    while (!cpu_has_work(env))
+    while (cpu_is_idle(env)) {
         qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
+    }
 
     qemu_kvm_eat_signals(env);
     qemu_wait_io_event_common(env);
@@ -1070,7 +1073,7 @@  bool cpu_exec_all(void)
         }
     }
     exit_request = 0;
-    return any_cpu_has_work();
+    return !all_cpus_idle();
 }
 
 void set_numa_modes(void)