Re: [Qemu-devel] [RFC 1/3] using CPUMASK bitmaps to calculate cpu index

[Igor Mammedov]

On Tue, 27 May 2014 08:39:00 +0000
"address@hidden" <address@hidden> wrote:

> On Thu, 2014-05-22 at 15:26 +0200, Igor Mammedov wrote: 
> > On Tue, 13 May 2014 18:08:47 +0800
> > Chen Fan <address@hidden> wrote:
> > 
> > > instead of seeking the number of CPUs, using CPUMASK bitmaps to
> > > calculate the cpu index, also would be a gread benefit to remove
> > > cpu index.
> > How would it help to remove cpu_index?
> I think the cpu_index is just a CPU counter，if we remove one sparse CPU
> by setting corresponding bit to zero. then adding a new one we could
> easily get this vacancy bit by find_first_zero_bit() rather than get a 
> duplicate index by calculating the number of CPU.
> 
> > What if there is only one CPU at start nad there is a need to add
> > CPU with cpu_index==10?
> > 
> > Bitmap hardly changes here anything, it's just another way of
> > doing:
> >    cpu_index = 0;
> >    CPU_FOREACH(some_cpu) {
> >        cpu_index++;
> >    }
> > 
> > 
> > What would help however is replacing cpu_index at read points with
> > CPUClass->get_arch_id()
> > 
> > Then targets that need sparse index could override default
> > get_arch_id() to suite their needs and use their own properties
> > to set arch_id value.
> > 
> > Caution: this change would cause migration breakage for target-i386,
> > so x86_cpu_get_arch_id() should take care about it when used with old
> > machine types.
> yes, but for migration, I think we should need to override the cpu index
> in function vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu)
> with an unique instance_id while hot-add/hot-remove CPU arbitrarily.
> maybe this unique instance_id can be 'apic-id'.
you can use just get_arch_id() there instead. For target-i386 it returns
apic id for other targets it's cpu_index. Just make sure that compat machine
will use cpu_index so that migration for old machine types would work.

> 
> Thanks,
> Chen
> 
> > 
> > > 
> > > Signed-off-by: Chen Fan <address@hidden>
> > > ---
> > >  exec.c                  | 9 ++++-----
> > >  include/qom/cpu.h       | 9 +++++++++
> > >  include/sysemu/sysemu.h | 7 -------
> > >  3 files changed, 13 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/exec.c b/exec.c
> > > index cf12049..2948841 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -473,16 +473,15 @@ void cpu_exec_init(CPUArchState *env)
> > >  {
> > >      CPUState *cpu = ENV_GET_CPU(env);
> > >      CPUClass *cc = CPU_GET_CLASS(cpu);
> > > -    CPUState *some_cpu;
> > >      int cpu_index;
> > >  
> > >  #if defined(CONFIG_USER_ONLY)
> > >      cpu_list_lock();
> > >  #endif
> > > -    cpu_index = 0;
> > > -    CPU_FOREACH(some_cpu) {
> > > -        cpu_index++;
> > > -    }
> > > +    cpu_index = find_first_zero_bit(cc->cpu_present_mask,
> > > +                                    MAX_CPUMASK_BITS);
> > > +    set_bit(cpu_index, cc->cpu_present_mask);
> > > +
> > >      cpu->cpu_index = cpu_index;
> > >      cpu->numa_node = 0;
> > >      QTAILQ_INIT(&cpu->breakpoints);
> > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > > index df977c8..b8f46b1 100644
> > > --- a/include/qom/cpu.h
> > > +++ b/include/qom/cpu.h
> > > @@ -70,6 +70,13 @@ typedef void (*CPUUnassignedAccess)(CPUState *cpu, 
> > > hwaddr addr,
> > >  
> > >  struct TranslationBlock;
> > >  
> > > +/* The following shall be true for all CPUs:
> > > + *   cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS
> > > + *
> > > + * Note that cpu->get_arch_id() may be larger than MAX_CPUMASK_BITS.
> > > + */
> > > +#define MAX_CPUMASK_BITS 255
> > > +
> > >  /**
> > >   * CPUClass:
> > >   * @class_by_name: Callback to map -cpu command line model name to an
> > > @@ -142,6 +149,8 @@ typedef struct CPUClass {
> > >      const struct VMStateDescription *vmsd;
> > >      int gdb_num_core_regs;
> > >      const char *gdb_core_xml_file;
> > > +
> > > +    DECLARE_BITMAP(cpu_present_mask, MAX_CPUMASK_BITS);
> > >  } CPUClass;
> > >  
> > >  #ifdef HOST_WORDS_BIGENDIAN
> > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > > index ba5c7f8..04edb8b 100644
> > > --- a/include/sysemu/sysemu.h
> > > +++ b/include/sysemu/sysemu.h
> > > @@ -134,13 +134,6 @@ extern QEMUClockType rtc_clock;
> > >  
> > >  #define MAX_NODES 64
> > >  
> > > -/* The following shall be true for all CPUs:
> > > - *   cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS
> > > - *
> > > - * Note that cpu->get_arch_id() may be larger than MAX_CPUMASK_BITS.
> > > - */
> > > -#define MAX_CPUMASK_BITS 255
> > > -
> > >  extern int nb_numa_nodes;
> > >  extern uint64_t node_mem[MAX_NODES];
> > >  extern unsigned long *node_cpumask[MAX_NODES];
> > 
>