[PATCH] softmmu/vl.c: fix too slow TCG regression

[PATCH] softmmu/vl.c: fix too slow TCG regression

[Igor Mammedov]

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(-)

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;
-- 
2.18.1

Re: [PATCH] softmmu/vl.c: fix too slow TCG regression

[Alex Bennée]

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;


-- 
Alex Bennée

Re: [PATCH] softmmu/vl.c: fix too slow TCG regression

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 4904 bytes --]

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
>


-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 7010 bytes --]

Re: [PATCH] softmmu/vl.c: fix too slow TCG regression

[Peter Maydell]

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

Re: [PATCH] softmmu/vl.c: fix too slow TCG regression

[Igor Mammedov]

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
>