| Message ID | 1454586455-10202-6-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 04, 2016 at 12:47:32PM +0100, Igor Mammedov wrote: > do not assume that all lapics in range 0..apic_id_limit > are valid and do not create lapic entries for not > possible lapics in MADT. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> But there's one minor suggestion below: > --- > hw/i386/acpi-build.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index df13c7d..9eeeffa 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm, > } > > static void > -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, > - PcGuestInfo *guest_info) > +build_madt(GArray *table_data, GArray *linker, > + MachineState *machine, PcGuestInfo *guest_info) > { > + MachineClass *mc = MACHINE_GET_CLASS(machine); > + GArray *apic_id_list = mc->possible_cpu_arch_ids(); > int madt_start = table_data->len; > > AcpiMultipleApicTable *madt; > @@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, > madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS); > madt->flags = cpu_to_le32(1); > > - for (i = 0; i < guest_info->apic_id_limit; i++) { > + for (i = 0; i < apic_id_list->len; i++) { > AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic); > + CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i); > + int apic_id = id.arch_id; > + > apic->type = ACPI_APIC_PROCESSOR; > apic->length = sizeof(*apic); > - apic->processor_id = i; > - apic->local_apic_id = i; > - if (test_bit(i, cpu->found_cpus)) { > + apic->processor_id = apic_id; > + apic->local_apic_id = apic_id; > + if (id.cpu != NULL) { This seems to be the only place where CPUArchId.cpu is being used (see my previous suggestion about making possible_cpu_arch_ids() return just an uint64_t list). Also, using the existing found_cpus bitmap is more efficient than making multiple calls to qemu_get_cpu_by_arch_id(). I wouldn't mind keeping the bitmap. > apic->flags = cpu_to_le32(1); > } else { > apic->flags = cpu_to_le32(0); > } > } > + g_array_free(apic_id_list, true); > + > io_apic = acpi_data_push(table_data, sizeof *io_apic); > io_apic->type = ACPI_APIC_IO; > io_apic->length = sizeof(*io_apic); > @@ -2659,7 +2666,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables) > aml_len += tables_blob->len - fadt; > > acpi_add_table(table_offsets, tables_blob); > - build_madt(tables_blob, tables->linker, &cpu, guest_info); > + build_madt(tables_blob, tables->linker, machine, guest_info); > > if (misc.has_hpet) { > acpi_add_table(table_offsets, tables_blob); > -- > 1.8.3.1 >
On Fri, 5 Feb 2016 13:28:31 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Feb 04, 2016 at 12:47:32PM +0100, Igor Mammedov wrote: > > do not assume that all lapics in range 0..apic_id_limit > > are valid and do not create lapic entries for not > > possible lapics in MADT. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > But there's one minor suggestion below: > > > --- > > hw/i386/acpi-build.c | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index df13c7d..9eeeffa 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm, > > } > > > > static void > > -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, > > - PcGuestInfo *guest_info) > > +build_madt(GArray *table_data, GArray *linker, > > + MachineState *machine, PcGuestInfo *guest_info) > > { > > + MachineClass *mc = MACHINE_GET_CLASS(machine); > > + GArray *apic_id_list = mc->possible_cpu_arch_ids(); > > int madt_start = table_data->len; > > > > AcpiMultipleApicTable *madt; > > @@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, > > madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS); > > madt->flags = cpu_to_le32(1); > > > > - for (i = 0; i < guest_info->apic_id_limit; i++) { > > + for (i = 0; i < apic_id_list->len; i++) { > > AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic); > > + CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i); > > + int apic_id = id.arch_id; > > + > > apic->type = ACPI_APIC_PROCESSOR; > > apic->length = sizeof(*apic); > > - apic->processor_id = i; > > - apic->local_apic_id = i; > > - if (test_bit(i, cpu->found_cpus)) { > > + apic->processor_id = apic_id; > > + apic->local_apic_id = apic_id; > > + if (id.cpu != NULL) { > > This seems to be the only place where CPUArchId.cpu is being used > (see my previous suggestion about making possible_cpu_arch_ids() > return just an uint64_t list). > > Also, using the existing found_cpus bitmap is more efficient than > making multiple calls to qemu_get_cpu_by_arch_id(). I wouldn't > mind keeping the bitmap. found_cpus bitmap is not better than (id.cpu != NULL) check the cost of filling both is about the same. The issue I have with bitmap is that it's harder to generalize CPU hotplug code with it, while with possible_cpu_arch_ids() returned array I have a list of CPUs to work with without any assumptions on position in bitmap or array. Also bitmap scales worse than a list of CPUs if ID space is sparse and if ID is quite big. That's why I'm dropping bitmap and switching to a list of IDs which in worst case is upto max_cpus. > > > apic->flags = cpu_to_le32(1); > > } else { > > apic->flags = cpu_to_le32(0); > > } > > } > > + g_array_free(apic_id_list, true); > > + > > io_apic = acpi_data_push(table_data, sizeof *io_apic); > > io_apic->type = ACPI_APIC_IO; > > io_apic->length = sizeof(*io_apic); > > @@ -2659,7 +2666,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables) > > aml_len += tables_blob->len - fadt; > > > > acpi_add_table(table_offsets, tables_blob); > > - build_madt(tables_blob, tables->linker, &cpu, guest_info); > > + build_madt(tables_blob, tables->linker, machine, guest_info); > > > > if (misc.has_hpet) { > > acpi_add_table(table_offsets, tables_blob); > > -- > > 1.8.3.1 > > >
On Fri, Feb 05, 2016 at 05:14:41PM +0100, Igor Mammedov wrote: > On Fri, 5 Feb 2016 13:28:31 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Feb 04, 2016 at 12:47:32PM +0100, Igor Mammedov wrote: > > > do not assume that all lapics in range 0..apic_id_limit > > > are valid and do not create lapic entries for not > > > possible lapics in MADT. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > But there's one minor suggestion below: > > > > > --- > > > hw/i386/acpi-build.c | 21 ++++++++++++++------- > > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > index df13c7d..9eeeffa 100644 > > > --- a/hw/i386/acpi-build.c > > > +++ b/hw/i386/acpi-build.c > > > @@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm, > > > } > > > > > > static void > > > -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, > > > - PcGuestInfo *guest_info) > > > +build_madt(GArray *table_data, GArray *linker, > > > + MachineState *machine, PcGuestInfo *guest_info) > > > { > > > + MachineClass *mc = MACHINE_GET_CLASS(machine); > > > + GArray *apic_id_list = mc->possible_cpu_arch_ids(); > > > int madt_start = table_data->len; > > > > > > AcpiMultipleApicTable *madt; > > > @@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, > > > madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS); > > > madt->flags = cpu_to_le32(1); > > > > > > - for (i = 0; i < guest_info->apic_id_limit; i++) { > > > + for (i = 0; i < apic_id_list->len; i++) { > > > AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic); > > > + CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i); > > > + int apic_id = id.arch_id; > > > + > > > apic->type = ACPI_APIC_PROCESSOR; > > > apic->length = sizeof(*apic); > > > - apic->processor_id = i; > > > - apic->local_apic_id = i; > > > - if (test_bit(i, cpu->found_cpus)) { > > > + apic->processor_id = apic_id; > > > + apic->local_apic_id = apic_id; > > > + if (id.cpu != NULL) { > > > > This seems to be the only place where CPUArchId.cpu is being used > > (see my previous suggestion about making possible_cpu_arch_ids() > > return just an uint64_t list). > > > > Also, using the existing found_cpus bitmap is more efficient than > > making multiple calls to qemu_get_cpu_by_arch_id(). I wouldn't > > mind keeping the bitmap. > found_cpus bitmap is not better than (id.cpu != NULL) check > the cost of filling both is about the same. The cost doesn't look the same. Populating found_cpus should be O(smp_cpus)[1], and is being done only once. Filling id.cpu in pc_possible_cpu_arch_ids() is O(max_cpus*smp_cpus), and it is called multiple times. [1] I just noticed it is actually O(size_of_qom_tree), but it is still linear, and could be changed to O(smp_cpus). > The issue I have with bitmap is that it's harder to generalize > CPU hotplug code with it, while with possible_cpu_arch_ids() > returned array I have a list of CPUs to work with without any > assumptions on position in bitmap or array. > Also bitmap scales worse than a list of CPUs if ID space > is sparse and if ID is quite big. Yes, I agree that a list is better depending on how the arch ID space is used. > That's why I'm dropping bitmap and switching to a list of > IDs which in worst case is upto max_cpus. I think the possible_cpu_arch_ids() interface looks good, maybe we just need to optimize it. But I'm not sure if we need to optimize it now, or if we can live with the inefficient code and optimize it later. I won't complain if we do it later, if we warn about it in the commit message or comments.
On Thu, 11 Feb 2016 14:11:34 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Feb 05, 2016 at 05:14:41PM +0100, Igor Mammedov wrote: > > On Fri, 5 Feb 2016 13:28:31 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Thu, Feb 04, 2016 at 12:47:32PM +0100, Igor Mammedov wrote: > > > > do not assume that all lapics in range 0..apic_id_limit > > > > are valid and do not create lapic entries for not > > > > possible lapics in MADT. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > > > But there's one minor suggestion below: > > > > > > > --- > > > > hw/i386/acpi-build.c | 21 ++++++++++++++------- > > > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > > index df13c7d..9eeeffa 100644 > > > > --- a/hw/i386/acpi-build.c > > > > +++ b/hw/i386/acpi-build.c > > > > @@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm, > > > > } > > > > > > > > static void > > > > -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, > > > > - PcGuestInfo *guest_info) > > > > +build_madt(GArray *table_data, GArray *linker, > > > > + MachineState *machine, PcGuestInfo *guest_info) > > > > { > > > > + MachineClass *mc = MACHINE_GET_CLASS(machine); > > > > + GArray *apic_id_list = mc->possible_cpu_arch_ids(); > > > > int madt_start = table_data->len; > > > > > > > > AcpiMultipleApicTable *madt; > > > > @@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, > > > > madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS); > > > > madt->flags = cpu_to_le32(1); > > > > > > > > - for (i = 0; i < guest_info->apic_id_limit; i++) { > > > > + for (i = 0; i < apic_id_list->len; i++) { > > > > AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic); > > > > + CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i); > > > > + int apic_id = id.arch_id; > > > > + > > > > apic->type = ACPI_APIC_PROCESSOR; > > > > apic->length = sizeof(*apic); > > > > - apic->processor_id = i; > > > > - apic->local_apic_id = i; > > > > - if (test_bit(i, cpu->found_cpus)) { > > > > + apic->processor_id = apic_id; > > > > + apic->local_apic_id = apic_id; > > > > + if (id.cpu != NULL) { > > > > > > This seems to be the only place where CPUArchId.cpu is being used > > > (see my previous suggestion about making possible_cpu_arch_ids() > > > return just an uint64_t list). > > > > > > Also, using the existing found_cpus bitmap is more efficient than > > > making multiple calls to qemu_get_cpu_by_arch_id(). I wouldn't > > > mind keeping the bitmap. > > found_cpus bitmap is not better than (id.cpu != NULL) check > > the cost of filling both is about the same. > > The cost doesn't look the same. Populating found_cpus should be > O(smp_cpus)[1], and is being done only once. Filling id.cpu in > pc_possible_cpu_arch_ids() is O(max_cpus*smp_cpus), and it is > called multiple times. I've refactored patch to make pc_possible_cpu_arch_ids() linear, pls see v2: https://www.mail-archive.com/qemu-devel@nongnu.org/msg351298.html > > [1] I just noticed it is actually O(size_of_qom_tree), but it > is still linear, and could be changed to O(smp_cpus). > > > The issue I have with bitmap is that it's harder to generalize > > CPU hotplug code with it, while with possible_cpu_arch_ids() > > returned array I have a list of CPUs to work with without any > > assumptions on position in bitmap or array. > > Also bitmap scales worse than a list of CPUs if ID space > > is sparse and if ID is quite big. > > Yes, I agree that a list is better depending on how the arch ID > space is used. > > > That's why I'm dropping bitmap and switching to a list of > > IDs which in worst case is upto max_cpus. > > I think the possible_cpu_arch_ids() interface looks good, maybe > we just need to optimize it. > > But I'm not sure if we need to optimize it now, or if we can live > with the inefficient code and optimize it later. I won't complain > if we do it later, if we warn about it in the commit message or > comments. >
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index df13c7d..9eeeffa 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm, } static void -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, - PcGuestInfo *guest_info) +build_madt(GArray *table_data, GArray *linker, + MachineState *machine, PcGuestInfo *guest_info) { + MachineClass *mc = MACHINE_GET_CLASS(machine); + GArray *apic_id_list = mc->possible_cpu_arch_ids(); int madt_start = table_data->len; AcpiMultipleApicTable *madt; @@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS); madt->flags = cpu_to_le32(1); - for (i = 0; i < guest_info->apic_id_limit; i++) { + for (i = 0; i < apic_id_list->len; i++) { AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic); + CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i); + int apic_id = id.arch_id; + apic->type = ACPI_APIC_PROCESSOR; apic->length = sizeof(*apic); - apic->processor_id = i; - apic->local_apic_id = i; - if (test_bit(i, cpu->found_cpus)) { + apic->processor_id = apic_id; + apic->local_apic_id = apic_id; + if (id.cpu != NULL) { apic->flags = cpu_to_le32(1); } else { apic->flags = cpu_to_le32(0); } } + g_array_free(apic_id_list, true); + io_apic = acpi_data_push(table_data, sizeof *io_apic); io_apic->type = ACPI_APIC_IO; io_apic->length = sizeof(*io_apic); @@ -2659,7 +2666,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables) aml_len += tables_blob->len - fadt; acpi_add_table(table_offsets, tables_blob); - build_madt(tables_blob, tables->linker, &cpu, guest_info); + build_madt(tables_blob, tables->linker, machine, guest_info); if (misc.has_hpet) { acpi_add_table(table_offsets, tables_blob);
do not assume that all lapics in range 0..apic_id_limit are valid and do not create lapic entries for not possible lapics in MADT. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/i386/acpi-build.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)