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 <bhar...@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.
