[v2,2/3] cpus: Convert cpu_index into a bitmap

Currently CPUState.cpu_index is monotonically increasing and a newly
created CPU always gets the next higher index. The next available
index is calculated by counting the existing number of CPUs. This is
fine as long as we only add CPUs, but there are architectures which
are starting to support CPU removal too. For an architecture like PowerPC
which derives its CPU identifier (device tree ID) from cpu_index, the
existing logic of generating cpu_index values causes problems.

With the currently proposed method of handling vCPU removal by parking
the vCPU fd in QEMU
(Ref: http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02604.html),
generating cpu_index this way will not work for PowerPC.

This patch changes the way cpu_index is handed out by maintaining
a bit map of the CPUs that tracks both addition and removal of CPUs.

The CPU bitmap allocation logic is part of cpu_exec_init() which is
called by instance_init routines of various CPU targets. Newly added
cpu_exec_exit() API handles the deallocation part and this routine is
called from generic CPU::instance_finalize().

Note: This new CPU enumeration is for !CONFIG_USER_ONLY only.
CONFIG_USER_ONLY continues to have the old enumeration logic.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 exec.c            | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 include/qom/cpu.h |  1 +
 qom/cpu.c         |  7 +++++++
 3 files changed, 58 insertions(+), 5 deletions(-)

On Tue, May 12, 2015 at 11:06:25AM +0530, Bharata B Rao wrote:
> Currently CPUState.cpu_index is monotonically increasing and a newly
> created CPU always gets the next higher index. The next available
> index is calculated by counting the existing number of CPUs. This is
> fine as long as we only add CPUs, but there are architectures which
> are starting to support CPU removal too. For an architecture like PowerPC
> which derives its CPU identifier (device tree ID) from cpu_index, the
> existing logic of generating cpu_index values causes problems.
> 
> With the currently proposed method of handling vCPU removal by parking
> the vCPU fd in QEMU
> (Ref: http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02604.html),
> generating cpu_index this way will not work for PowerPC.
> 
> This patch changes the way cpu_index is handed out by maintaining
> a bit map of the CPUs that tracks both addition and removal of CPUs.
> 
> The CPU bitmap allocation logic is part of cpu_exec_init() which is
> called by instance_init routines of various CPU targets. Newly added
> cpu_exec_exit() API handles the deallocation part and this routine is
> called from generic CPU::instance_finalize().
> 
> Note: This new CPU enumeration is for !CONFIG_USER_ONLY only.
> CONFIG_USER_ONLY continues to have the old enumeration logic.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Looks good to me, minor comments below:

> ---
>  exec.c            | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  include/qom/cpu.h |  1 +
>  qom/cpu.c         |  7 +++++++
>  3 files changed, 58 insertions(+), 5 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 5cf821e..c8c4e53 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -518,21 +518,66 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
>  }
>  #endif
>  
> +#ifndef CONFIG_USER_ONLY
> +static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> +
> +static int cpu_get_free_index(Error **errp)
> +{
> +    int cpu = find_first_zero_bit(cpu_index_map, max_cpus);
> +
> +    if (cpu >= max_cpus) {
> +        error_setg(errp, "Trying to use more CPUs than allowed max of %d\n",
> +                    max_cpus);
> +        return -1;
> +    } else {
> +        bitmap_set(cpu_index_map, cpu, 1);
> +        return cpu;
> +    }

I prefer the following style, to avoid indenting the non-error path:

     if (error_condition) {
        error_setg(...);
        return;
     }

     bitmap_set(...)
     return cpu;

(Which is exactly the style you used in cpu_exec_exit() below, by the
way).

> +}
> +
> +void cpu_exec_exit(CPUState *cpu)
> +{
> +    if (cpu->cpu_index == -1) {
> +        /* cpu_index was never allocated by this @cpu or was already freed. */
> +        return;
> +    }
> +
> +    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> +    cpu->cpu_index = -1;
> +}
> +#else
> +
> +static int cpu_get_free_index(Error **errp)
> +{
> +    CPUState *some_cpu;
> +    int cpu_index = 0;
> +
> +    CPU_FOREACH(some_cpu) {
> +        cpu_index++;
> +    }
> +    return cpu_index;
> +}
> +
> +void cpu_exec_exit(CPUState *cpu)
> +{
> +}
> +#endif
> +
>  void cpu_exec_init(CPUArchState *env, Error **errp)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> -    CPUState *some_cpu;
>      int cpu_index;
> +    Error *local_err = NULL;
>  
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
>  #endif
> -    cpu_index = 0;
> -    CPU_FOREACH(some_cpu) {
> -        cpu_index++;
> +    cpu_index = cpu->cpu_index = cpu_get_free_index(&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
>      }
> -    cpu->cpu_index = cpu_index;
>      cpu->numa_node = 0;
>      QTAILQ_INIT(&cpu->breakpoints);
>      QTAILQ_INIT(&cpu->watchpoints);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 39f0f19..7db310e 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -672,6 +672,7 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
>  
>  void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
> +void cpu_exec_exit(CPUState *cpu);

Why don't we keep both cpu_exec_init() and cpu_exec_exit() in the same
header file?

>
>  #ifdef CONFIG_SOFTMMU
>  extern const struct VMStateDescription vmstate_cpu_common;
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 108bfa2..061a0c3 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -312,9 +312,15 @@ static void cpu_common_initfn(Object *obj)
>      CPUState *cpu = CPU(obj);
>      CPUClass *cc = CPU_GET_CLASS(obj);
>  
> +    cpu->cpu_index = -1;
>      cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
>  }
>  
> +static void cpu_common_finalize(Object *obj)
> +{
> +    cpu_exec_exit(CPU(obj));
> +}
> +
>  static int64_t cpu_common_get_arch_id(CPUState *cpu)
>  {
>      return cpu->cpu_index;
> @@ -356,6 +362,7 @@ static const TypeInfo cpu_type_info = {
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(CPUState),
>      .instance_init = cpu_common_initfn,
> +    .instance_finalize = cpu_common_finalize,
>      .abstract = true,
>      .class_size = sizeof(CPUClass),
>      .class_init = cpu_class_init,
> -- 
> 2.1.0
>

On Mon, May 18, 2015 at 03:36:54PM -0300, Eduardo Habkost wrote:
> On Tue, May 12, 2015 at 11:06:25AM +0530, Bharata B Rao wrote:
> > Currently CPUState.cpu_index is monotonically increasing and a newly
> > created CPU always gets the next higher index. The next available
> > index is calculated by counting the existing number of CPUs. This is
> > fine as long as we only add CPUs, but there are architectures which
> > are starting to support CPU removal too. For an architecture like PowerPC
> > which derives its CPU identifier (device tree ID) from cpu_index, the
> > existing logic of generating cpu_index values causes problems.
> > 
> > With the currently proposed method of handling vCPU removal by parking
> > the vCPU fd in QEMU
> > (Ref: http://lists.gnu.org/archive/html/qemu-devel/2015-02/msg02604.html),
> > generating cpu_index this way will not work for PowerPC.
> > 
> > This patch changes the way cpu_index is handed out by maintaining
> > a bit map of the CPUs that tracks both addition and removal of CPUs.
> > 
> > The CPU bitmap allocation logic is part of cpu_exec_init() which is
> > called by instance_init routines of various CPU targets. Newly added
> > cpu_exec_exit() API handles the deallocation part and this routine is
> > called from generic CPU::instance_finalize().
> > 
> > Note: This new CPU enumeration is for !CONFIG_USER_ONLY only.
> > CONFIG_USER_ONLY continues to have the old enumeration logic.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> Looks good to me, minor comments below:
> 
> > ---
> >  exec.c            | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  include/qom/cpu.h |  1 +
> >  qom/cpu.c         |  7 +++++++
> >  3 files changed, 58 insertions(+), 5 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index 5cf821e..c8c4e53 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -518,21 +518,66 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
> >  }
> >  #endif
> >  
> > +#ifndef CONFIG_USER_ONLY
> > +static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
> > +
> > +static int cpu_get_free_index(Error **errp)
> > +{
> > +    int cpu = find_first_zero_bit(cpu_index_map, max_cpus);
> > +
> > +    if (cpu >= max_cpus) {
> > +        error_setg(errp, "Trying to use more CPUs than allowed max of %d\n",
> > +                    max_cpus);
> > +        return -1;
> > +    } else {
> > +        bitmap_set(cpu_index_map, cpu, 1);
> > +        return cpu;
> > +    }
> 
> I prefer the following style, to avoid indenting the non-error path:
> 
>      if (error_condition) {
>         error_setg(...);
>         return;
>      }
> 
>      bitmap_set(...)
>      return cpu;
> 
> (Which is exactly the style you used in cpu_exec_exit() below, by the
> way).

Sure, I can change this in next post.

> 
> > +}
> > +
> > +void cpu_exec_exit(CPUState *cpu)
> > +{
> > +    if (cpu->cpu_index == -1) {
> > +        /* cpu_index was never allocated by this @cpu or was already freed. */
> > +        return;
> > +    }
> > +
> > +    bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
> > +    cpu->cpu_index = -1;
> > +}
> > +#else
> > +
> > +static int cpu_get_free_index(Error **errp)
> > +{
> > +    CPUState *some_cpu;
> > +    int cpu_index = 0;
> > +
> > +    CPU_FOREACH(some_cpu) {
> > +        cpu_index++;
> > +    }
> > +    return cpu_index;
> > +}
> > +
> > +void cpu_exec_exit(CPUState *cpu)
> > +{
> > +}
> > +#endif
> > +
> >  void cpu_exec_init(CPUArchState *env, Error **errp)
> >  {
> >      CPUState *cpu = ENV_GET_CPU(env);
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > -    CPUState *some_cpu;
> >      int cpu_index;
> > +    Error *local_err = NULL;
> >  
> >  #if defined(CONFIG_USER_ONLY)
> >      cpu_list_lock();
> >  #endif
> > -    cpu_index = 0;
> > -    CPU_FOREACH(some_cpu) {
> > -        cpu_index++;
> > +    cpu_index = cpu->cpu_index = cpu_get_free_index(&local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> >      }
> > -    cpu->cpu_index = cpu_index;
> >      cpu->numa_node = 0;
> >      QTAILQ_INIT(&cpu->breakpoints);
> >      QTAILQ_INIT(&cpu->watchpoints);
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 39f0f19..7db310e 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -672,6 +672,7 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
> >  
> >  void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
> >      GCC_FMT_ATTR(2, 3);
> > +void cpu_exec_exit(CPUState *cpu);
> 
> Why don't we keep both cpu_exec_init() and cpu_exec_exit() in the same
> header file?

Currently cpu_exec_init() is in exec-all.h.

1. If I put cpu_exec_exit() also there, qom/cpu.c doesn't like it since
many definitions (like ram_addr_t etc) aren't known in qom/cpu.c.

2. I can't move cpu_exec_init() declaration to qom/cpu.h since it results
in the use of poisoned definition of CPUArchState from qom/cpu.c

Hence I separated them into different hearder files.

Regards,
Bharata.

On Tue, May 19, 2015 at 11:49:36AM +0530, Bharata B Rao wrote:
> On Mon, May 18, 2015 at 03:36:54PM -0300, Eduardo Habkost wrote:
> > On Tue, May 12, 2015 at 11:06:25AM +0530, Bharata B Rao wrote:
[...]
> > >  void cpu_exec_init(CPUArchState *env, Error **errp)
[...]
> > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > > index 39f0f19..7db310e 100644
> > > --- a/include/qom/cpu.h
> > > +++ b/include/qom/cpu.h
> > > @@ -672,6 +672,7 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
> > >  
> > >  void QEMU_NORETURN cpu_abort(CPUState *cpu, const char *fmt, ...)
> > >      GCC_FMT_ATTR(2, 3);
> > > +void cpu_exec_exit(CPUState *cpu);
> > 
> > Why don't we keep both cpu_exec_init() and cpu_exec_exit() in the same
> > header file?
> 
> Currently cpu_exec_init() is in exec-all.h.
> 
> 1. If I put cpu_exec_exit() also there, qom/cpu.c doesn't like it since
> many definitions (like ram_addr_t etc) aren't known in qom/cpu.c.
> 
> 2. I can't move cpu_exec_init() declaration to qom/cpu.h since it results
> in the use of poisoned definition of CPUArchState from qom/cpu.c

OK, so we can change cpu_exec_init() to use CPUState in a follow-up
patch later.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>