Re: [Qemu-ppc] [PATCH v5 02/10] exec: Remove cpu from cpus list during cpu_exec_exit()

[David Gibson]

On Fri, Nov 20, 2015 at 06:24:31PM +0530, Bharata B Rao wrote:
> CPUState *cpu gets added to the cpus list during cpu_exec_init(). It
> should be removed from cpu_exec_exit().
> 
> cpu_exec_init() is called from generic CPU::instance_finalize and some
> archs like PowerPC call it from CPU unrealizefn. So ensure that we
> dequeue the cpu only once.
> 
> Now -1 value for cpu->cpu_index indicates that we have already dequeued
> the cpu for CONFIG_USER_ONLY case also.

It's not clear to me if you're intending this just as an interim step
or not.  Surely we should fix the existing code to be consistent about
where the QTAILQ_REMOVE is done, rather than using a special -1 flag?

> 
> Signed-off-by: Bharata B Rao <address@hidden>
> ---
>  exec.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/exec.c b/exec.c
> index b09f18b..fbac85b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -593,6 +593,7 @@ void cpu_exec_exit(CPUState *cpu)
>          return;
>      }
>  
> +    QTAILQ_REMOVE(&cpus, cpu, node);
>      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
>      cpu->cpu_index = -1;
>  }
> @@ -611,6 +612,15 @@ static int cpu_get_free_index(Error **errp)
>  
>  void cpu_exec_exit(CPUState *cpu)
>  {
> +    cpu_list_lock();
> +    if (cpu->cpu_index == -1) {

.. especially since as far as I can tell you're only testing for the
-1 value here, which is in the USER_ONLY version of this function.


> +        cpu_list_unlock();
> +        return;
> +    }
> +
> +    QTAILQ_REMOVE(&cpus, cpu, node);

Again, as the user only version of the function, is this redundant
with the removals in linux-user/{main.c,syscall.c}?

> +    cpu->cpu_index = -1;
> +    cpu_list_unlock();
>  }
>  #endif
>  

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature