| Message ID | 1448024079-20808-3-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 20, 2015 at 06:24:31PM +0530, 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 > 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. It's not clear to me if you're intending this just as an interim step or not. Surely we should fix the existing code to be consistent about where the QTAILQ_REMOVE is done, rather than using a special -1 flag? > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > exec.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/exec.c b/exec.c > index b09f18b..fbac85b 100644 > --- a/exec.c > +++ b/exec.c > @@ -593,6 +593,7 @@ void cpu_exec_exit(CPUState *cpu) > return; > } > > + QTAILQ_REMOVE(&cpus, cpu, node); > bitmap_clear(cpu_index_map, cpu->cpu_index, 1); > cpu->cpu_index = -1; > } > @@ -611,6 +612,15 @@ static int cpu_get_free_index(Error **errp) > > void cpu_exec_exit(CPUState *cpu) > { > + cpu_list_lock(); > + if (cpu->cpu_index == -1) { .. especially since as far as I can tell you're only testing for the -1 value here, which is in the USER_ONLY version of this function. > + cpu_list_unlock(); > + return; > + } > + > + QTAILQ_REMOVE(&cpus, cpu, node); Again, as the user only version of the function, is this redundant with the removals in linux-user/{main.c,syscall.c}? > + cpu->cpu_index = -1; > + cpu_list_unlock(); > } > #endif >
On Tue, Dec 01, 2015 at 11:44:03AM +1100, David Gibson wrote: > On Fri, Nov 20, 2015 at 06:24:31PM +0530, 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 > > 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. > > It's not clear to me if you're intending this just as an interim step > or not. Surely we should fix the existing code to be consistent about > where the QTAILQ_REMOVE is done, rather than using a special -1 flag? cpu_index for a CPU starts with -1 and comes back to -1 when the CPU is destroyed. So -1 value is already being used to prevent double freeing of this particular CPU bit from the cpu_index_map. In this patch, I am depending on the same (-1 value) to guard against double removal of the CPU from the list. Setting cpu_index to -1 wasn't being done for CONFIG_USER_ONLY and this patch adds that bit too. This check against double freeing from bitmap and double removal from list is needed since we call cpu_exec_init() from CPU::instance_finalize for all archs and archs like PowerPC call it from unrealizefn. If and when all archs switch to calling cpu_exec_init()/_exit() from realizefn/unrealizefn, we wouldn't have to worry about this double freeing. Regards, Bharata.
diff --git a/exec.c b/exec.c index b09f18b..fbac85b 100644 --- a/exec.c +++ b/exec.c @@ -593,6 +593,7 @@ void cpu_exec_exit(CPUState *cpu) return; } + QTAILQ_REMOVE(&cpus, cpu, node); bitmap_clear(cpu_index_map, cpu->cpu_index, 1); cpu->cpu_index = -1; } @@ -611,6 +612,15 @@ static int cpu_get_free_index(Error **errp) void cpu_exec_exit(CPUState *cpu) { + cpu_list_lock(); + if (cpu->cpu_index == -1) { + cpu_list_unlock(); + return; + } + + QTAILQ_REMOVE(&cpus, cpu, node); + cpu->cpu_index = -1; + cpu_list_unlock(); } #endif
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 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 | 10 ++++++++++ 1 file changed, 10 insertions(+)