| Message ID | 20200226163539.31960-1-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | softmmu/vl.c: fix too slow TCG regression | expand |
Igor Mammedov <imammedo@redhat.com> writes: > Commit a1b18df9a4 moved -m option parsing after configure_accelerators() > that broke TCG accelerator initialization which accesses global ram_size > from size_code_gen_buffer() which is equal to 0 at that moment. > > Partially revert a1b18df9a4, by returning set_memory_options() to its > original location and only keep 32-bit host VA check and 'memory-backend' > size check introduced by fe64d06afc at current place. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > > PS: > This should take care of regression and give more time to think about > how to remove size_code_gen_buffer() dependency on ram_size Tested-by: Alex Bennée <alex.bennee@linaro.org> FWIW I don't think it will take too long to fixup size_code_gen_buffer. See: Subject: [PATCH v1 0/4] Fix codegen translation cache size Date: Wed, 26 Feb 2020 18:10:16 +0000 Message-Id: <20200226181020.19592-1-alex.bennee@linaro.org> > --- > softmmu/vl.c | 49 +++++++++++++++++++++++++------------------------ > 1 file changed, 25 insertions(+), 24 deletions(-) > > diff --git a/softmmu/vl.c b/softmmu/vl.c > index a9cce78f45..da7577129c 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -2634,29 +2634,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size, > exit(EXIT_FAILURE); > } > > - if (current_machine->ram_memdev_id) { > - Object *backend; > - ram_addr_t backend_size; > - > - backend = object_resolve_path_type(current_machine->ram_memdev_id, > - TYPE_MEMORY_BACKEND, NULL); > - backend_size = object_property_get_uint(backend, "size", &error_abort); > - if (mem_str && backend_size != ram_size) { > - error_report("Size specified by -m option must match size of " > - "explicitly specified 'memory-backend' property"); > - exit(EXIT_FAILURE); > - } > - ram_size = backend_size; > - } > - > - if (!xen_enabled()) { > - /* On 32-bit hosts, QEMU is limited by virtual address space */ > - if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) { > - error_report("at most 2047 MB RAM can be simulated"); > - exit(1); > - } > - } > - > loc_pop(&loc); > } > > @@ -3821,6 +3798,8 @@ void qemu_init(int argc, char **argv, char **envp) > machine_class = select_machine(); > object_set_machine_compat_props(machine_class->compat_props); > > + set_memory_options(&ram_slots, &maxram_size, machine_class); > + > os_daemonize(); > > /* > @@ -4296,7 +4275,29 @@ void qemu_init(int argc, char **argv, char **envp) > current_machine->cpu_type = parse_cpu_option(cpu_option); > } > > - set_memory_options(&ram_slots, &maxram_size, machine_class); > + if (!xen_enabled()) { > + /* On 32-bit hosts, QEMU is limited by virtual address space */ > + if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) { > + error_report("at most 2047 MB RAM can be simulated"); > + exit(1); > + } > + } > + > + if (current_machine->ram_memdev_id) { > + Object *backend; > + ram_addr_t backend_size; > + > + backend = object_resolve_path_type(current_machine->ram_memdev_id, > + TYPE_MEMORY_BACKEND, NULL); > + backend_size = object_property_get_uint(backend, "size", &error_abort); > + if (backend_size != ram_size) { > + error_report("Size specified by -m option must match size of " > + "explicitly specified 'memory-backend' property"); > + exit(EXIT_FAILURE); > + } > + ram_size = backend_size; > + } > + > current_machine->ram_size = ram_size; > current_machine->maxram_size = maxram_size; > current_machine->ram_slots = ram_slots;
Hello Igor, Alex, On Wed, Feb 26, 2020 at 7:21 PM Alex Bennée <alex.bennee@linaro.org> wrote: > > Igor Mammedov <imammedo@redhat.com> writes: > > > Commit a1b18df9a4 moved -m option parsing after configure_accelerators() > > that broke TCG accelerator initialization which accesses global ram_size > > from size_code_gen_buffer() which is equal to 0 at that moment. > > > > Partially revert a1b18df9a4, by returning set_memory_options() to its > > original location and only keep 32-bit host VA check and 'memory-backend' > > size check introduced by fe64d06afc at current place. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > > > PS: > > This should take care of regression and give more time to think about > > how to remove size_code_gen_buffer() dependency on ram_size > Thanks for your quick response on this! I just applied this patch on top of latest master (db736e0437aa6fd7c1b7e4599c17f9619ab6b837) and indeed it seems performance is back to normal. The avocado tests for cubieboard are also passing again like before. So for me: Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com> > > Tested-by: Alex Bennée <alex.bennee@linaro.org> > > FWIW I don't think it will take too long to fixup size_code_gen_buffer. > See: > > Subject: [PATCH v1 0/4] Fix codegen translation cache size > Date: Wed, 26 Feb 2020 18:10:16 +0000 > Message-Id: <20200226181020.19592-1-alex.bennee@linaro.org> > > Yeah I saw your patch come by on the list as well Alex. I will try to look into it as well. Regards, Niek > > --- > > softmmu/vl.c | 49 +++++++++++++++++++++++++------------------------ > > 1 file changed, 25 insertions(+), 24 deletions(-) > > > > diff --git a/softmmu/vl.c b/softmmu/vl.c > > index a9cce78f45..da7577129c 100644 > > --- a/softmmu/vl.c > > +++ b/softmmu/vl.c > > @@ -2634,29 +2634,6 @@ static void set_memory_options(uint64_t > *ram_slots, ram_addr_t *maxram_size, > > exit(EXIT_FAILURE); > > } > > > > - if (current_machine->ram_memdev_id) { > > - Object *backend; > > - ram_addr_t backend_size; > > - > > - backend = > object_resolve_path_type(current_machine->ram_memdev_id, > > - TYPE_MEMORY_BACKEND, NULL); > > - backend_size = object_property_get_uint(backend, "size", > &error_abort); > > - if (mem_str && backend_size != ram_size) { > > - error_report("Size specified by -m option must match > size of " > > - "explicitly specified 'memory-backend' > property"); > > - exit(EXIT_FAILURE); > > - } > > - ram_size = backend_size; > > - } > > - > > - if (!xen_enabled()) { > > - /* On 32-bit hosts, QEMU is limited by virtual address space */ > > - if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) { > > - error_report("at most 2047 MB RAM can be simulated"); > > - exit(1); > > - } > > - } > > - > > loc_pop(&loc); > > } > > > > @@ -3821,6 +3798,8 @@ void qemu_init(int argc, char **argv, char **envp) > > machine_class = select_machine(); > > object_set_machine_compat_props(machine_class->compat_props); > > > > + set_memory_options(&ram_slots, &maxram_size, machine_class); > > + > > os_daemonize(); > > > > /* > > @@ -4296,7 +4275,29 @@ void qemu_init(int argc, char **argv, char **envp) > > current_machine->cpu_type = parse_cpu_option(cpu_option); > > } > > > > - set_memory_options(&ram_slots, &maxram_size, machine_class); > > + if (!xen_enabled()) { > > + /* On 32-bit hosts, QEMU is limited by virtual address space */ > > + if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) { > > + error_report("at most 2047 MB RAM can be simulated"); > > + exit(1); > > + } > > + } > > + > > + if (current_machine->ram_memdev_id) { > > + Object *backend; > > + ram_addr_t backend_size; > > + > > + backend = > object_resolve_path_type(current_machine->ram_memdev_id, > > + TYPE_MEMORY_BACKEND, NULL); > > + backend_size = object_property_get_uint(backend, "size", > &error_abort); > > + if (backend_size != ram_size) { > > + error_report("Size specified by -m option must match > size of " > > + "explicitly specified 'memory-backend' > property"); > > + exit(EXIT_FAILURE); > > + } > > + ram_size = backend_size; > > + } > > + > > current_machine->ram_size = ram_size; > > current_machine->maxram_size = maxram_size; > > current_machine->ram_slots = ram_slots; > > > -- > Alex Bennée >
On Wed, 26 Feb 2020 at 16:35, Igor Mammedov <imammedo@redhat.com> wrote: > > Commit a1b18df9a4 moved -m option parsing after configure_accelerators() > that broke TCG accelerator initialization which accesses global ram_size > from size_code_gen_buffer() which is equal to 0 at that moment. > > Partially revert a1b18df9a4, by returning set_memory_options() to its > original location and only keep 32-bit host VA check and 'memory-backend' > size check introduced by fe64d06afc at current place. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > > PS: > This should take care of regression and give more time to think about > how to remove size_code_gen_buffer() dependency on ram_size > + if (current_machine->ram_memdev_id) { > + Object *backend; > + ram_addr_t backend_size; > + > + backend = object_resolve_path_type(current_machine->ram_memdev_id, > + TYPE_MEMORY_BACKEND, NULL); > + backend_size = object_property_get_uint(backend, "size", &error_abort); > + if (backend_size != ram_size) { > + error_report("Size specified by -m option must match size of " > + "explicitly specified 'memory-backend' property"); > + exit(EXIT_FAILURE); > + } > + ram_size = backend_size; Why do we do this assignment? We've just checked that backend_size == ram_size so the assignment won't do anything, will it? In the version of this check in set_memory_options() the assignment was useful because the error check only happened if mem_str is not NULL, ie there was an explicitly specified 'size' option somewhere. It looks like now we require the backend size to match even if the size is not explicitly specified by the user but comes from some default somewhere? thanks -- PMM
On Thu, 27 Feb 2020 09:15:00 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On Wed, 26 Feb 2020 at 16:35, Igor Mammedov <imammedo@redhat.com> wrote: > > > > Commit a1b18df9a4 moved -m option parsing after configure_accelerators() > > that broke TCG accelerator initialization which accesses global ram_size > > from size_code_gen_buffer() which is equal to 0 at that moment. > > > > Partially revert a1b18df9a4, by returning set_memory_options() to its > > original location and only keep 32-bit host VA check and 'memory-backend' > > size check introduced by fe64d06afc at current place. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > > > PS: > > This should take care of regression and give more time to think about > > how to remove size_code_gen_buffer() dependency on ram_size > > > + if (current_machine->ram_memdev_id) { > > + Object *backend; > > + ram_addr_t backend_size; > > + > > + backend = object_resolve_path_type(current_machine->ram_memdev_id, > > + TYPE_MEMORY_BACKEND, NULL); > > + backend_size = object_property_get_uint(backend, "size", &error_abort); > > + if (backend_size != ram_size) { > > + error_report("Size specified by -m option must match size of " > > + "explicitly specified 'memory-backend' property"); > > + exit(EXIT_FAILURE); > > + } > > + ram_size = backend_size; > > Why do we do this assignment? We've just checked that > backend_size == ram_size so the assignment won't do > anything, will it? > > In the version of this check in set_memory_options() > the assignment was useful because the error check > only happened if mem_str is not NULL, ie there was > an explicitly specified 'size' option somewhere. It > looks like now we require the backend size to match > even if the size is not explicitly specified by the > user but comes from some default somewhere? Size might come from default and in that case mem_str was skipping the check and updating ram_size with backend's value. So this patch is not correct, as it is breaking the case where no "-m" was specified and explicit backend was provided. > > thanks > -- PMM >
diff --git a/softmmu/vl.c b/softmmu/vl.c index a9cce78f45..da7577129c 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2634,29 +2634,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size, exit(EXIT_FAILURE); } - if (current_machine->ram_memdev_id) { - Object *backend; - ram_addr_t backend_size; - - backend = object_resolve_path_type(current_machine->ram_memdev_id, - TYPE_MEMORY_BACKEND, NULL); - backend_size = object_property_get_uint(backend, "size", &error_abort); - if (mem_str && backend_size != ram_size) { - error_report("Size specified by -m option must match size of " - "explicitly specified 'memory-backend' property"); - exit(EXIT_FAILURE); - } - ram_size = backend_size; - } - - if (!xen_enabled()) { - /* On 32-bit hosts, QEMU is limited by virtual address space */ - if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) { - error_report("at most 2047 MB RAM can be simulated"); - exit(1); - } - } - loc_pop(&loc); } @@ -3821,6 +3798,8 @@ void qemu_init(int argc, char **argv, char **envp) machine_class = select_machine(); object_set_machine_compat_props(machine_class->compat_props); + set_memory_options(&ram_slots, &maxram_size, machine_class); + os_daemonize(); /* @@ -4296,7 +4275,29 @@ void qemu_init(int argc, char **argv, char **envp) current_machine->cpu_type = parse_cpu_option(cpu_option); } - set_memory_options(&ram_slots, &maxram_size, machine_class); + if (!xen_enabled()) { + /* On 32-bit hosts, QEMU is limited by virtual address space */ + if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) { + error_report("at most 2047 MB RAM can be simulated"); + exit(1); + } + } + + if (current_machine->ram_memdev_id) { + Object *backend; + ram_addr_t backend_size; + + backend = object_resolve_path_type(current_machine->ram_memdev_id, + TYPE_MEMORY_BACKEND, NULL); + backend_size = object_property_get_uint(backend, "size", &error_abort); + if (backend_size != ram_size) { + error_report("Size specified by -m option must match size of " + "explicitly specified 'memory-backend' property"); + exit(EXIT_FAILURE); + } + ram_size = backend_size; + } + current_machine->ram_size = ram_size; current_machine->maxram_size = maxram_size; current_machine->ram_slots = ram_slots;
Commit a1b18df9a4 moved -m option parsing after configure_accelerators() that broke TCG accelerator initialization which accesses global ram_size from size_code_gen_buffer() which is equal to 0 at that moment. Partially revert a1b18df9a4, by returning set_memory_options() to its original location and only keep 32-bit host VA check and 'memory-backend' size check introduced by fe64d06afc at current place. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- PS: This should take care of regression and give more time to think about how to remove size_code_gen_buffer() dependency on ram_size --- softmmu/vl.c | 49 +++++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 24 deletions(-)