| Message ID | 1438838837-28504-2-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 06, 2015 at 10:57:07AM +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. > > Instead of introducing a new field CPUState.queued, I could have used > CPUState.cpu_index to check if the cpu is already dequeued from the list. > Since that doesn't work for CONFIG_USER_ONLY, I had to add a new field. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> This seems reasonable to me, but I'm wondering how x86 cpu hotplug / unplug is working without it. > --- > exec.c | 11 +++++++++++ > include/qom/cpu.h | 1 + > 2 files changed, 12 insertions(+) > > diff --git a/exec.c b/exec.c > index 0a4a0c5..b196d68 100644 > --- a/exec.c > +++ b/exec.c > @@ -550,6 +550,10 @@ void cpu_exec_exit(CPUState *cpu) > return; > } > > + if (cpu->queued) { > + QTAILQ_REMOVE(&cpus, cpu, node); > + cpu->queued = false; > + } > bitmap_clear(cpu_index_map, cpu->cpu_index, 1); > cpu->cpu_index = -1; > } > @@ -568,6 +572,12 @@ static int cpu_get_free_index(Error **errp) > > void cpu_exec_exit(CPUState *cpu) > { > + cpu_list_lock(); > + if (cpu->queued) { > + QTAILQ_REMOVE(&cpus, cpu, node); > + cpu->queued = false; > + } > + cpu_list_unlock(); > } > #endif > > @@ -595,6 +605,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) > return; > } > QTAILQ_INSERT_TAIL(&cpus, cpu, node); > + cpu->queued = true; > #if defined(CONFIG_USER_ONLY) > cpu_list_unlock(); > #endif > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 20aabc9..a00e3a8 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -284,6 +284,7 @@ struct CPUState { > int gdb_num_regs; > int gdb_num_g_regs; > QTAILQ_ENTRY(CPUState) node; > + bool queued; > > /* ice debug support */ > QTAILQ_HEAD(breakpoints_head, CPUBreakpoint) breakpoints;
On Fri, Sep 04, 2015 at 03:31:24PM +1000, David Gibson wrote: > On Thu, Aug 06, 2015 at 10:57:07AM +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. > > > > Instead of introducing a new field CPUState.queued, I could have used > > CPUState.cpu_index to check if the cpu is already dequeued from the list. > > Since that doesn't work for CONFIG_USER_ONLY, I had to add a new field. > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > This seems reasonable to me, but I'm wondering how x86 cpu hotplug / > unplug is working without it. x86 hotplug/unplug code currently resides in Zhu's git tree (git://github.com/zhugh/qemu). They are removing the CPU from the list explicitly in x86 CPU's instance_finalize routine. Since we add CPU to the list in cpu_exec_init(), I thought it makes sense to remove it in cpu_exec_exit(). May be it makes sense to separately purse this patch and the next one so that other archs are also taken into account correctly. Regards, Bharata.
On 09/09/2015 01:52 PM, Bharata B Rao wrote: > On Fri, Sep 04, 2015 at 03:31:24PM +1000, David Gibson wrote: >> On Thu, Aug 06, 2015 at 10:57:07AM +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. >>> >>> Instead of introducing a new field CPUState.queued, I could have used >>> CPUState.cpu_index to check if the cpu is already dequeued from the list. >>> Since that doesn't work for CONFIG_USER_ONLY, I had to add a new field. >>> >>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> >> This seems reasonable to me, but I'm wondering how x86 cpu hotplug / >> unplug is working without it. > x86 hotplug/unplug code currently resides in Zhu's git tree > (git://github.com/zhugh/qemu). They are removing the CPU from the list > explicitly in x86 CPU's instance_finalize routine. Sorry, my git tree is git://github.com/zhuguihua/qemu Now there was no progress about topology, so we don't know what will happen in x86. I am not sure whether we will take this method finally. Thanks, Zhu > > Since we add CPU to the list in cpu_exec_init(), I thought it makes > sense to remove it in cpu_exec_exit(). > > May be it makes sense to separately purse this patch and the next one > so that other archs are also taken into account correctly. > > Regards, > Bharata. > > >
On Wed, Sep 09, 2015 at 03:41:30PM +0800, Zhu Guihua wrote: > > On 09/09/2015 01:52 PM, Bharata B Rao wrote: > >On Fri, Sep 04, 2015 at 03:31:24PM +1000, David Gibson wrote: > >>On Thu, Aug 06, 2015 at 10:57:07AM +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. > >>> > >>>Instead of introducing a new field CPUState.queued, I could have used > >>>CPUState.cpu_index to check if the cpu is already dequeued from the list. > >>>Since that doesn't work for CONFIG_USER_ONLY, I had to add a new field. > >>> > >>>Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > >>This seems reasonable to me, but I'm wondering how x86 cpu hotplug / > >>unplug is working without it. > >x86 hotplug/unplug code currently resides in Zhu's git tree > >(git://github.com/zhugh/qemu). They are removing the CPU from the list > >explicitly in x86 CPU's instance_finalize routine. > > Sorry, my git tree is git://github.com/zhuguihua/qemu > > Now there was no progress about topology, so we don't know what will happen > in x86. I am not sure whether we will take this method finally. Andreas had a presentation on this topic in KVM forum recently. Andreas - do you have any updates on the topology and other aspects of CPU hotplug so that we can align the CPU hotplug work in different archs accordingly and hope to get it merged in 2.5 time frame ? Regards, Bharata.
Hi Bharata, On 09/09/2015 03:56 PM, Bharata B Rao wrote: > On Wed, Sep 09, 2015 at 03:41:30PM +0800, Zhu Guihua wrote: >> On 09/09/2015 01:52 PM, Bharata B Rao wrote: >>> On Fri, Sep 04, 2015 at 03:31:24PM +1000, David Gibson wrote: >>>> On Thu, Aug 06, 2015 at 10:57:07AM +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. >>>>> >>>>> Instead of introducing a new field CPUState.queued, I could have used >>>>> CPUState.cpu_index to check if the cpu is already dequeued from the list. >>>>> Since that doesn't work for CONFIG_USER_ONLY, I had to add a new field. >>>>> >>>>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> >>>> This seems reasonable to me, but I'm wondering how x86 cpu hotplug / >>>> unplug is working without it. >>> x86 hotplug/unplug code currently resides in Zhu's git tree >>> (git://github.com/zhugh/qemu). They are removing the CPU from the list >>> explicitly in x86 CPU's instance_finalize routine. >> Sorry, my git tree is git://github.com/zhuguihua/qemu >> >> Now there was no progress about topology, so we don't know what will happen >> in x86. I am not sure whether we will take this method finally. > Andreas had a presentation on this topic in KVM forum recently. > > Andreas - do you have any updates on the topology and other aspects > of CPU hotplug so that we can align the CPU hotplug work in different > archs accordingly and hope to get it merged in 2.5 time frame ? Do you update the patchset? My work in x86 has stopped for a while, Maybe I can get some ideas from another arch's worker. Thanks, Zhu
On Thu, Nov 12, 2015 at 05:11:02PM +0800, Zhu Guihua wrote: > Hi Bharata, > > On 09/09/2015 03:56 PM, Bharata B Rao wrote: > >On Wed, Sep 09, 2015 at 03:41:30PM +0800, Zhu Guihua wrote: > >>On 09/09/2015 01:52 PM, Bharata B Rao wrote: > >>>On Fri, Sep 04, 2015 at 03:31:24PM +1000, David Gibson wrote: > >>>>On Thu, Aug 06, 2015 at 10:57:07AM +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. > >>>>> > >>>>>Instead of introducing a new field CPUState.queued, I could have used > >>>>>CPUState.cpu_index to check if the cpu is already dequeued from the list. > >>>>>Since that doesn't work for CONFIG_USER_ONLY, I had to add a new field. > >>>>> > >>>>>Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > >>>>This seems reasonable to me, but I'm wondering how x86 cpu hotplug / > >>>>unplug is working without it. > >>>x86 hotplug/unplug code currently resides in Zhu's git tree > >>>(git://github.com/zhugh/qemu). They are removing the CPU from the list > >>>explicitly in x86 CPU's instance_finalize routine. > >>Sorry, my git tree is git://github.com/zhuguihua/qemu > >> > >>Now there was no progress about topology, so we don't know what will happen > >>in x86. I am not sure whether we will take this method finally. > >Andreas had a presentation on this topic in KVM forum recently. > > > >Andreas - do you have any updates on the topology and other aspects > >of CPU hotplug so that we can align the CPU hotplug work in different > >archs accordingly and hope to get it merged in 2.5 time frame ? > > Do you update the patchset? > > My work in x86 has stopped for a while, Maybe I can get some ideas from > another > arch's worker. My last version is here: https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00650.html I initally started with core level CPU hotplug, moved to socket level hotplug based on Andreas' patchset and then moved back again to core level hotplug. I was a bit confused about how the generic semantics would evovle and hence the work got delayed. I wil be posting the next version of my patchset based on core level semantics soon. I am hoping that I should be able to get CPU hotplug/unplug included in QEMU-2.6 timeframe. Regards, Bharata.
On 11/12/2015 05:30 PM, Bharata B Rao wrote: > On Thu, Nov 12, 2015 at 05:11:02PM +0800, Zhu Guihua wrote: >> Hi Bharata, >> >> On 09/09/2015 03:56 PM, Bharata B Rao wrote: >>> On Wed, Sep 09, 2015 at 03:41:30PM +0800, Zhu Guihua wrote: >>>> On 09/09/2015 01:52 PM, Bharata B Rao wrote: >>>>> On Fri, Sep 04, 2015 at 03:31:24PM +1000, David Gibson wrote: >>>>>> On Thu, Aug 06, 2015 at 10:57:07AM +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. >>>>>>> >>>>>>> Instead of introducing a new field CPUState.queued, I could have used >>>>>>> CPUState.cpu_index to check if the cpu is already dequeued from the list. >>>>>>> Since that doesn't work for CONFIG_USER_ONLY, I had to add a new field. >>>>>>> >>>>>>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> >>>>>> This seems reasonable to me, but I'm wondering how x86 cpu hotplug / >>>>>> unplug is working without it. >>>>> x86 hotplug/unplug code currently resides in Zhu's git tree >>>>> (git://github.com/zhugh/qemu). They are removing the CPU from the list >>>>> explicitly in x86 CPU's instance_finalize routine. >>>> Sorry, my git tree is git://github.com/zhuguihua/qemu >>>> >>>> Now there was no progress about topology, so we don't know what will happen >>>> in x86. I am not sure whether we will take this method finally. >>> Andreas had a presentation on this topic in KVM forum recently. >>> >>> Andreas - do you have any updates on the topology and other aspects >>> of CPU hotplug so that we can align the CPU hotplug work in different >>> archs accordingly and hope to get it merged in 2.5 time frame ? >> Do you update the patchset? >> >> My work in x86 has stopped for a while, Maybe I can get some ideas from >> another >> arch's worker. > My last version is here: > https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00650.html > > I initally started with core level CPU hotplug, moved to socket level hotplug > based on Andreas' patchset and then moved back again to core level hotplug. > > I was a bit confused about how the generic semantics would evovle and hence > the work got delayed. I wil be posting the next version of my patchset > based on core level semantics soon. > > I am hoping that I should be able to get CPU hotplug/unplug included > in QEMU-2.6 timeframe. > Thanks for your reply. Look forward to your next version. Regards, Zhu
Am 12.11.2015 um 10:30 schrieb Bharata B Rao: > On Thu, Nov 12, 2015 at 05:11:02PM +0800, Zhu Guihua wrote: >> Hi Bharata, >> >> On 09/09/2015 03:56 PM, Bharata B Rao wrote: >>> On Wed, Sep 09, 2015 at 03:41:30PM +0800, Zhu Guihua wrote: >>>> On 09/09/2015 01:52 PM, Bharata B Rao wrote: >>>>> On Fri, Sep 04, 2015 at 03:31:24PM +1000, David Gibson wrote: >>>>>> On Thu, Aug 06, 2015 at 10:57:07AM +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. >>>>>>> >>>>>>> Instead of introducing a new field CPUState.queued, I could have used >>>>>>> CPUState.cpu_index to check if the cpu is already dequeued from the list. >>>>>>> Since that doesn't work for CONFIG_USER_ONLY, I had to add a new field. >>>>>>> >>>>>>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> >>>>>> This seems reasonable to me, but I'm wondering how x86 cpu hotplug / >>>>>> unplug is working without it. >>>>> x86 hotplug/unplug code currently resides in Zhu's git tree >>>>> (git://github.com/zhugh/qemu). They are removing the CPU from the list >>>>> explicitly in x86 CPU's instance_finalize routine. >>>> Sorry, my git tree is git://github.com/zhuguihua/qemu >>>> >>>> Now there was no progress about topology, so we don't know what will happen >>>> in x86. I am not sure whether we will take this method finally. >>> Andreas had a presentation on this topic in KVM forum recently. >>> >>> Andreas - do you have any updates on the topology and other aspects >>> of CPU hotplug so that we can align the CPU hotplug work in different >>> archs accordingly and hope to get it merged in 2.5 time frame ? >> >> Do you update the patchset? >> >> My work in x86 has stopped for a while, Maybe I can get some ideas from >> another >> arch's worker. > > My last version is here: > https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00650.html > > I initally started with core level CPU hotplug, moved to socket level hotplug > based on Andreas' patchset and then moved back again to core level hotplug. > > I was a bit confused about how the generic semantics would evovle and hence > the work got delayed. I wil be posting the next version of my patchset > based on core level semantics soon. What I recall as conclusion from the KVM Forum session and previous discussions was that pseries would operate on core level (i.e., granularity of two SMT threads), whereas your first try was on thread level and then on socket level. Regards, Andreas > I am hoping that I should be able to get CPU hotplug/unplug included > in QEMU-2.6 timeframe. If there are preparatory patches ready for inclusion today, please point me to them urgently. Thanks, Andreas
On Thu, Nov 12, 2015 at 10:56:50AM +0100, Andreas Färber wrote: > <snip> > > I am hoping that I should be able to get CPU hotplug/unplug included > > in QEMU-2.6 timeframe. > > If there are preparatory patches ready for inclusion today, please point > me to them urgently. Thanks. I do have some generic changes, but I will push them during 2.6 development. Regards, Bharata.
diff --git a/exec.c b/exec.c index 0a4a0c5..b196d68 100644 --- a/exec.c +++ b/exec.c @@ -550,6 +550,10 @@ void cpu_exec_exit(CPUState *cpu) return; } + if (cpu->queued) { + QTAILQ_REMOVE(&cpus, cpu, node); + cpu->queued = false; + } bitmap_clear(cpu_index_map, cpu->cpu_index, 1); cpu->cpu_index = -1; } @@ -568,6 +572,12 @@ static int cpu_get_free_index(Error **errp) void cpu_exec_exit(CPUState *cpu) { + cpu_list_lock(); + if (cpu->queued) { + QTAILQ_REMOVE(&cpus, cpu, node); + cpu->queued = false; + } + cpu_list_unlock(); } #endif @@ -595,6 +605,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp) return; } QTAILQ_INSERT_TAIL(&cpus, cpu, node); + cpu->queued = true; #if defined(CONFIG_USER_ONLY) cpu_list_unlock(); #endif diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 20aabc9..a00e3a8 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -284,6 +284,7 @@ struct CPUState { int gdb_num_regs; int gdb_num_g_regs; QTAILQ_ENTRY(CPUState) node; + bool queued; /* ice debug support */ QTAILQ_HEAD(breakpoints_head, CPUBreakpoint) breakpoints;
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. Instead of introducing a new field CPUState.queued, I could have used CPUState.cpu_index to check if the cpu is already dequeued from the list. Since that doesn't work for CONFIG_USER_ONLY, I had to add a new field. Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- exec.c | 11 +++++++++++ include/qom/cpu.h | 1 + 2 files changed, 12 insertions(+)