Re: [Qemu-devel] [RFC PATCH v1 01/10] exec: Remove cpu from cpus list during cpu_exec_exit()

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Mon, Mar 07, 2016 at 05:23:48PM +0100, Thomas Huth wrote:
> On 04.03.2016 07:54, 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
> 
> s/cpu_exec_init/cpu_exec_exit/
> 
> > 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.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  exec.c | 32 ++++++++++++++++++++++++--------
> >  1 file changed, 24 insertions(+), 8 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index c62c439..7c3f747 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -588,15 +588,9 @@ static int cpu_get_free_index(Error **errp)
> >      return cpu;
> >  }
> >  
> > -void cpu_exec_exit(CPUState *cpu)
> > +static void cpu_release_index(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
> >  
> > @@ -611,11 +605,33 @@ static int cpu_get_free_index(Error **errp)
> >      return cpu_index;
> >  }
> >  
> > -void cpu_exec_exit(CPUState *cpu)
> > +static void cpu_release_index(CPUState *cpu)
> >  {
> > +    return;
> 
> You could also simply leave that return statement away, I think.
> 
> >  }
> >  #endif
> >  
> > +void cpu_exec_exit(CPUState *cpu)
> > +{
> > +#if defined(CONFIG_USER_ONLY)
> > +    cpu_list_lock();
> > +#endif
> > +    if (cpu->cpu_index == -1) {
> > +        /* cpu_index was never allocated by this @cpu or was already freed. */
> > +#if defined(CONFIG_USER_ONLY)
> > +        cpu_list_unlock();
> > +#endif
> > +        return;
> > +    }
> > +
> > +    QTAILQ_REMOVE(&cpus, cpu, node);
> > +    cpu_release_index(cpu);
> > +    cpu->cpu_index = -1;
> > +#if defined(CONFIG_USER_ONLY)
> > +    cpu_list_unlock();
> > +#endif
> > +}
> 
> Since there are a couple of these
> 
> #if defined(CONFIG_USER_ONLY)
>     cpu_list_[un]lock();
> #endif
> 
> in exec.c already, it might be somewhat nices to declare them at the
> beginning of the file as empty functions, somewhat like:
> 
> #if !defined(CONFIG_USER_ONLY)
> static inline void cpu_list_lock(void)
> {
> }
> static inline void cpu_list_unlock(void)
> {
> }
> #endif
> 
> What do you think about that?

If you and/or the maintainer insist/prefer, I can make the change.

Regards,
Bharata.