| Message ID | 1463024905-28401-2-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 12/05/2016 05:48, 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_exit() is called from generic CPU::instance_finalize and some > archs like PowerPC call it from CPU unrealizefn. So ensure that we > dequeue the cpu only once. I think the better thing would be to call it from CPU::unrealize, but this patch is okay too. Thanks, Paolo > 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> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Reviewed-by: Thomas Huth <thuth@redhat.com>
On Thu, May 26, 2016 at 12:12:41PM +0200, Paolo Bonzini wrote: > > > On 12/05/2016 05:48, 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_exit() is called from generic CPU::instance_finalize and some > > archs like PowerPC call it from CPU unrealizefn. So ensure that we > > dequeue the cpu only once. > > I think the better thing would be to call it from CPU::unrealize, but > this patch is okay too. > > Thanks, > > Paolo Thanks for the review Paolo. However, what I'm really unclear on is what is the next step towards merging these. Will you take them through your tree? Should Bharata send a formal pull request with the prelim patches? If so, to whom? > > 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> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > Reviewed-by: Thomas Huth <thuth@redhat.com> >
On 27/05/2016 05:07, David Gibson wrote: > On Thu, May 26, 2016 at 12:12:41PM +0200, Paolo Bonzini wrote: >> >> >> On 12/05/2016 05:48, 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_exit() is called from generic CPU::instance_finalize and some >>> archs like PowerPC call it from CPU unrealizefn. So ensure that we >>> dequeue the cpu only once. >> >> I think the better thing would be to call it from CPU::unrealize, but >> this patch is okay too. >> >> Thanks, >> >> Paolo > > Thanks for the review Paolo. > > However, what I'm really unclear on is what is the next step towards > merging these. Will you take them through your tree? Should Bharata > send a formal pull request with the prelim patches? If so, to whom? Feel free to take them and add an Acked-by for me. The fewer patches I merge, the better. :) Thanks, Paolo
diff --git a/exec.c b/exec.c index c4f9036..da1f09a 100644 --- a/exec.c +++ b/exec.c @@ -610,15 +610,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 @@ -633,11 +627,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; } #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 +} + void cpu_exec_init(CPUState *cpu, Error **errp) { CPUClass *cc = CPU_GET_CLASS(cpu);