| Message ID | 1371398269-6213-25-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On Sun, Jun 16, 2013 at 05:57:44PM +0200, Andreas Färber wrote: > Revert commit d6b9e0d60cc511eca210834428bb74508cff3d33 (cpu: Add > qemu_for_each_cpu()) and its usage in favor of open-coding CPU loops, > now that they are based on CPUState. > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Andreas Färber <afaerber@suse.de> Open-coding is kind of nasty though. How about #define qemu_for_each_cpu(cpu) \ for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) > --- > cpus.c | 11 ++++------- > exec.c | 11 ----------- > hw/acpi/piix4.c | 20 +++++++++----------- > include/qom/cpu.h | 9 --------- > qom/cpu.c | 30 +++++++++--------------------- > 5 files changed, 22 insertions(+), 59 deletions(-) > > diff --git a/cpus.c b/cpus.c > index ec38644..691c435 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -803,12 +803,6 @@ static void *qemu_dummy_cpu_thread_fn(void *arg) > > static void tcg_exec_all(void); > > -static void tcg_signal_cpu_creation(CPUState *cpu, void *data) > -{ > - cpu->thread_id = qemu_get_thread_id(); > - cpu->created = true; > -} > - > static void *qemu_tcg_cpu_thread_fn(void *arg) > { > CPUState *cpu = arg; > @@ -817,7 +811,10 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) > qemu_thread_get_self(cpu->thread); > > qemu_mutex_lock(&qemu_global_mutex); > - qemu_for_each_cpu(tcg_signal_cpu_creation, NULL); > + for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) { > + cpu->thread_id = qemu_get_thread_id(); > + cpu->created = true; > + } > qemu_cond_signal(&qemu_cpu_cond); > > /* wait for initial kick-off after machine start */ > diff --git a/exec.c b/exec.c > index 191eb4e..5f3aba1 100644 > --- a/exec.c > +++ b/exec.c > @@ -276,17 +276,6 @@ CPUState *qemu_get_cpu(int index) > return cpu; > } > > -void qemu_for_each_cpu(void (*func)(CPUState *cpu, void *data), void *data) > -{ > - CPUState *cpu; > - > - cpu = first_cpu; > - while (cpu) { > - func(cpu, data); > - cpu = cpu->next_cpu; > - } > -} > - > void cpu_exec_init(CPUArchState *env) > { > CPUState *cpu = ENV_GET_CPU(env); > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > index 756df3b..3234452 100644 > --- a/hw/acpi/piix4.c > +++ b/hw/acpi/piix4.c > @@ -654,22 +654,14 @@ static void piix4_cpu_added_req(Notifier *n, void *opaque) > piix4_cpu_hotplug_req(s, CPU(opaque), PLUG); > } > > -static void piix4_init_cpu_status(CPUState *cpu, void *data) > -{ > - CPUStatus *g = (CPUStatus *)data; > - CPUClass *k = CPU_GET_CLASS(cpu); > - int64_t id = k->get_arch_id(cpu); > - > - g_assert((id / 8) < PIIX4_PROC_LEN); > - g->sts[id / 8] |= (1 << (id % 8)); > -} > - > static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > PCIHotplugState state); > > static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > PCIBus *bus, PIIX4PMState *s) > { > + CPUState *cpu; > + > memory_region_init_io(&s->io_gpe, &piix4_gpe_ops, s, "apci-gpe0", > GPE_LEN); > memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe); > @@ -680,7 +672,13 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > &s->io_pci); > pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev); > > - qemu_for_each_cpu(piix4_init_cpu_status, &s->gpe_cpu); > + for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) { > + CPUClass *cc = CPU_GET_CLASS(cpu); > + int64_t id = cc->get_arch_id(cpu); > + > + g_assert((id / 8) < PIIX4_PROC_LEN); > + s->gpe_cpu.sts[id / 8] |= (1 << (id % 8)); > + } > memory_region_init_io(&s->io_cpu, &cpu_hotplug_ops, s, "apci-cpu-hotplug", > PIIX4_PROC_LEN); > memory_region_add_subregion(parent, PIIX4_PROC_BASE, &s->io_cpu); > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 2a64af2..467896c 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -356,15 +356,6 @@ bool cpu_is_stopped(CPUState *cpu); > void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data); > > /** > - * qemu_for_each_cpu: > - * @func: The function to be executed. > - * @data: Data to pass to the function. > - * > - * Executes @func for each CPU. > - */ > -void qemu_for_each_cpu(void (*func)(CPUState *cpu, void *data), void *data); > - > -/** > * qemu_get_cpu: > * @index: The CPUState@cpu_index value of the CPU to obtain. > * > diff --git a/qom/cpu.c b/qom/cpu.c > index ee8f632..94fa2b8 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -24,30 +24,18 @@ > #include "qemu/notify.h" > #include "sysemu/sysemu.h" > > -typedef struct CPUExistsArgs { > - int64_t id; > - bool found; > -} CPUExistsArgs; > - > -static void cpu_exist_cb(CPUState *cpu, void *data) > -{ > - CPUClass *klass = CPU_GET_CLASS(cpu); > - CPUExistsArgs *arg = data; > - > - if (klass->get_arch_id(cpu) == arg->id) { > - arg->found = true; > - } > -} > - > bool cpu_exists(int64_t id) > { > - CPUExistsArgs data = { > - .id = id, > - .found = false, > - }; > + CPUState *cpu; > + > + for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) { > + CPUClass *cc = CPU_GET_CLASS(cpu); > > - qemu_for_each_cpu(cpu_exist_cb, &data); > - return data.found; > + if (cc->get_arch_id(cpu) == id) { > + return true; > + } > + } > + return false; > } > > bool cpu_paging_enabled(const CPUState *cpu) > -- > 1.8.1.4
On 16 June 2013 21:26, Michael S. Tsirkin <mst@redhat.com> wrote: > On Sun, Jun 16, 2013 at 05:57:44PM +0200, Andreas Färber wrote: >> Revert commit d6b9e0d60cc511eca210834428bb74508cff3d33 (cpu: Add >> qemu_for_each_cpu()) and its usage in favor of open-coding CPU loops, >> now that they are based on CPUState. >> >> Suggested-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Andreas Färber <afaerber@suse.de> > > Open-coding is kind of nasty though. > > How about > #define qemu_for_each_cpu(cpu) \ > for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) Personally for an iteration that simple I would prefer the opencoded version -- it's more immediately obvious what it's doing and that it's not doing anything weird. Precedent isn't everything, but a quick grep for 'foreach' and 'for_each' suggests that we generally use the callback-fn style rather than iterator macros. thanks -- PMM
On Sun, Jun 16, 2013 at 09:30:29PM +0100, Peter Maydell wrote: > On 16 June 2013 21:26, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Sun, Jun 16, 2013 at 05:57:44PM +0200, Andreas Färber wrote: > >> Revert commit d6b9e0d60cc511eca210834428bb74508cff3d33 (cpu: Add > >> qemu_for_each_cpu()) and its usage in favor of open-coding CPU loops, > >> now that they are based on CPUState. > >> > >> Suggested-by: Markus Armbruster <armbru@redhat.com> > >> Signed-off-by: Andreas Färber <afaerber@suse.de> > > > > Open-coding is kind of nasty though. > > > > How about > > #define qemu_for_each_cpu(cpu) \ > > for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) > > Personally for an iteration that simple I would prefer > the opencoded version -- it's more immediately > obvious what it's doing and that it's not doing anything > weird. Only if you have memorized how is the list of CPUs put together. You probably did, most other readers probably didn't, and have to waste time looking it up. > Precedent isn't everything, but a quick grep for 'foreach' > and 'for_each' suggests that we generally use the callback-fn > style rather than iterator macros. > > thanks > -- PMM Callback is fine too.
Am 16.06.2013 22:26, schrieb Michael S. Tsirkin: > On Sun, Jun 16, 2013 at 05:57:44PM +0200, Andreas Färber wrote: >> Revert commit d6b9e0d60cc511eca210834428bb74508cff3d33 (cpu: Add >> qemu_for_each_cpu()) and its usage in favor of open-coding CPU loops, >> now that they are based on CPUState. >> >> Suggested-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Andreas Färber <afaerber@suse.de> > > Open-coding is kind of nasty though. > > How about > #define qemu_for_each_cpu(cpu) \ > for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) I've been investigating QTAILQ_FOREACH(), but I'd rather do things in two steps - CPUState type (21/29) in this series and then further macro wizardry. I'd be fine to defer this patch and replace it with the final helper of our choice. Andreas
diff --git a/cpus.c b/cpus.c index ec38644..691c435 100644 --- a/cpus.c +++ b/cpus.c @@ -803,12 +803,6 @@ static void *qemu_dummy_cpu_thread_fn(void *arg) static void tcg_exec_all(void); -static void tcg_signal_cpu_creation(CPUState *cpu, void *data) -{ - cpu->thread_id = qemu_get_thread_id(); - cpu->created = true; -} - static void *qemu_tcg_cpu_thread_fn(void *arg) { CPUState *cpu = arg; @@ -817,7 +811,10 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) qemu_thread_get_self(cpu->thread); qemu_mutex_lock(&qemu_global_mutex); - qemu_for_each_cpu(tcg_signal_cpu_creation, NULL); + for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) { + cpu->thread_id = qemu_get_thread_id(); + cpu->created = true; + } qemu_cond_signal(&qemu_cpu_cond); /* wait for initial kick-off after machine start */ diff --git a/exec.c b/exec.c index 191eb4e..5f3aba1 100644 --- a/exec.c +++ b/exec.c @@ -276,17 +276,6 @@ CPUState *qemu_get_cpu(int index) return cpu; } -void qemu_for_each_cpu(void (*func)(CPUState *cpu, void *data), void *data) -{ - CPUState *cpu; - - cpu = first_cpu; - while (cpu) { - func(cpu, data); - cpu = cpu->next_cpu; - } -} - void cpu_exec_init(CPUArchState *env) { CPUState *cpu = ENV_GET_CPU(env); diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 756df3b..3234452 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -654,22 +654,14 @@ static void piix4_cpu_added_req(Notifier *n, void *opaque) piix4_cpu_hotplug_req(s, CPU(opaque), PLUG); } -static void piix4_init_cpu_status(CPUState *cpu, void *data) -{ - CPUStatus *g = (CPUStatus *)data; - CPUClass *k = CPU_GET_CLASS(cpu); - int64_t id = k->get_arch_id(cpu); - - g_assert((id / 8) < PIIX4_PROC_LEN); - g->sts[id / 8] |= (1 << (id % 8)); -} - static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, PCIHotplugState state); static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, PCIBus *bus, PIIX4PMState *s) { + CPUState *cpu; + memory_region_init_io(&s->io_gpe, &piix4_gpe_ops, s, "apci-gpe0", GPE_LEN); memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe); @@ -680,7 +672,13 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, &s->io_pci); pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev); - qemu_for_each_cpu(piix4_init_cpu_status, &s->gpe_cpu); + for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) { + CPUClass *cc = CPU_GET_CLASS(cpu); + int64_t id = cc->get_arch_id(cpu); + + g_assert((id / 8) < PIIX4_PROC_LEN); + s->gpe_cpu.sts[id / 8] |= (1 << (id % 8)); + } memory_region_init_io(&s->io_cpu, &cpu_hotplug_ops, s, "apci-cpu-hotplug", PIIX4_PROC_LEN); memory_region_add_subregion(parent, PIIX4_PROC_BASE, &s->io_cpu); diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 2a64af2..467896c 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -356,15 +356,6 @@ bool cpu_is_stopped(CPUState *cpu); void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data); /** - * qemu_for_each_cpu: - * @func: The function to be executed. - * @data: Data to pass to the function. - * - * Executes @func for each CPU. - */ -void qemu_for_each_cpu(void (*func)(CPUState *cpu, void *data), void *data); - -/** * qemu_get_cpu: * @index: The CPUState@cpu_index value of the CPU to obtain. * diff --git a/qom/cpu.c b/qom/cpu.c index ee8f632..94fa2b8 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -24,30 +24,18 @@ #include "qemu/notify.h" #include "sysemu/sysemu.h" -typedef struct CPUExistsArgs { - int64_t id; - bool found; -} CPUExistsArgs; - -static void cpu_exist_cb(CPUState *cpu, void *data) -{ - CPUClass *klass = CPU_GET_CLASS(cpu); - CPUExistsArgs *arg = data; - - if (klass->get_arch_id(cpu) == arg->id) { - arg->found = true; - } -} - bool cpu_exists(int64_t id) { - CPUExistsArgs data = { - .id = id, - .found = false, - }; + CPUState *cpu; + + for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) { + CPUClass *cc = CPU_GET_CLASS(cpu); - qemu_for_each_cpu(cpu_exist_cb, &data); - return data.found; + if (cc->get_arch_id(cpu) == id) { + return true; + } + } + return false; } bool cpu_paging_enabled(const CPUState *cpu)
Revert commit d6b9e0d60cc511eca210834428bb74508cff3d33 (cpu: Add qemu_for_each_cpu()) and its usage in favor of open-coding CPU loops, now that they are based on CPUState. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Andreas Färber <afaerber@suse.de> --- cpus.c | 11 ++++------- exec.c | 11 ----------- hw/acpi/piix4.c | 20 +++++++++----------- include/qom/cpu.h | 9 --------- qom/cpu.c | 30 +++++++++--------------------- 5 files changed, 22 insertions(+), 59 deletions(-)