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

[Thomas Huth <thuth@redhat.com>]

On 09.03.2016 08:57, Bharata B Rao wrote:
> 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.

It would be a nice way to get rid of some #if statements in the code,
but I don't insist on that change ... so feel free to keep the current
state, if you prefer it.

 Thomas