[Qemu-devel] Re: [PATCH 22/32] add next_cpu_index

[Glauber Costa <glommer@redhat.com>]

On Thu, Oct 23, 2008 at 04:40:49PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Thu, Oct 23, 2008 at 09:21:45AM -0500, Anthony Liguori wrote:
> >> Glauber Costa wrote:
> >>> From: Glauber Costa <gcosta@redhat.com>
> >>>
> >>> separate the logic for calculating the next cpu index
> >>> from cpu creation. It will allow others to query what's
> >>> the next cpu index to be created before cpu creation.
> >>>   
> >> What is this useful for?
> > In earlier versions of the series, I was passing the cpu_index to init_env.
> > So I guess it's just a die hard patch that survived the reworks. It can be dropped
> > now (although I believe its clearer this way).
> 
> Why not keep track of the last assigned cpu_index in a global or static
> variable? That should be clearer and avoids the iteration (which is
> basically a count-how-many-cpus-we-already-have loop).
I like this. A "next_cpu_index()" function that increments it atomically would
do just fine. We don't have this kind of race yet for lack of smpbility, but it's
one less thing to fix in the future.

> 
> Jan
> 
> > 
> >> Regards,
> >>
> >> Anthony Liguori
> >>
> >>> Signed-off-by: Glauber Costa <gcosta@redhat.com>
> >>> ---
> >>>  exec.c |   22 ++++++++++++++--------
> >>>  1 files changed, 14 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/exec.c b/exec.c
> >>> index 80b8a78..7fe7eeb 100644
> >>> --- a/exec.c
> >>> +++ b/exec.c
> >>> @@ -526,25 +526,31 @@ static int cpu_common_load(QEMUFile *f, void *opaque, int version_id)
> >>>  }
> >>>  #endif
> >>>
> >>> -void cpu_exec_init(CPUState *env)
> >>> +int next_cpu_index(void)
> >>>  {
> >>>      CPUState **penv;
> >>> -    int cpu_index;
> >>> +    int cpu_index = 0;
> >>>
> >>> -    env->next_cpu = NULL;
> >>>      penv = &first_cpu;
> >>> -    cpu_index = 0;
> >>> +
> >>>      while (*penv != NULL) {
> >>>          penv = (CPUState **)&(*penv)->next_cpu;
> >>>          cpu_index++;
> >>>      }
> >>> -    env->cpu_index = cpu_index;
> >>> +    return cpu_index;
> >>> +}
> >>> +
> >>> +void cpu_exec_init(CPUState *env)
> >>> +{
> >>> +    env->next_cpu = NULL;
> >>> +    env->cpu_index = next_cpu_index();
> >>>      env->nb_watchpoints = 0;
> >>> -    *penv = env;
> >>> +    if (env->cpu_index == 0)
> >>> +        first_cpu = env;
> >>>  #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
> >>> -    register_savevm("cpu_common", cpu_index, CPU_COMMON_SAVE_VERSION,
> >>> +    register_savevm("cpu_common", env->cpu_index, CPU_COMMON_SAVE_VERSION,
> >>>                      cpu_common_save, cpu_common_load, env);
> >>> -    register_savevm("cpu", cpu_index, CPU_SAVE_VERSION,
> >>> +    register_savevm("cpu", env->cpu_index, CPU_SAVE_VERSION,
> >>>                      cpu_save, cpu_load, env);
> >>>  #endif
> >>>  }
> >>>   
> 
> 
> -- 
> Siemens AG, Corporate Technology, CT SE 2
> Corporate Competence Center Embedded Linux