All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] softmmu/vl.c: fix too slow TCG regression
@ 2020-02-26 16:35 Igor Mammedov
  2020-02-26 18:21 ` Alex Bennée
  2020-02-27  9:15 ` Peter Maydell
  0 siblings, 2 replies; 5+ messages in thread
From: Igor Mammedov @ 2020-02-26 16:35 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, f4bug, nieklinnenbank, hsp.cat7, pbonzini,
	alex.bennee, rth

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



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] softmmu/vl.c: fix too slow TCG regression
  2020-02-26 16:35 [PATCH] softmmu/vl.c: fix too slow TCG regression Igor Mammedov
@ 2020-02-26 18:21 ` Alex Bennée
  2020-02-26 19:51   ` Niek Linnenbank
  2020-02-27  9:15 ` Peter Maydell
  1 sibling, 1 reply; 5+ messages in thread
From: Alex Bennée @ 2020-02-26 18:21 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: peter.maydell, qemu-devel, f4bug, nieklinnenbank, hsp.cat7,
	pbonzini, rth


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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] softmmu/vl.c: fix too slow TCG regression
  2020-02-26 18:21 ` Alex Bennée
@ 2020-02-26 19:51   ` Niek Linnenbank
  0 siblings, 0 replies; 5+ messages in thread
From: Niek Linnenbank @ 2020-02-26 19:51 UTC (permalink / raw)
  To: Alex Bennée
  Cc: Peter Maydell, Philippe Mathieu-Daudé,
	QEMU Developers, Paolo Bonzini, hsp.cat7, Igor Mammedov, rth

[-- 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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] softmmu/vl.c: fix too slow TCG regression
  2020-02-26 16:35 [PATCH] softmmu/vl.c: fix too slow TCG regression Igor Mammedov
  2020-02-26 18:21 ` Alex Bennée
@ 2020-02-27  9:15 ` Peter Maydell
  2020-02-27  9:55   ` Igor Mammedov
  1 sibling, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2020-02-27  9:15 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: QEMU Developers, Philippe Mathieu-Daudé,
	Niek Linnenbank, Howard Spoelstra, Paolo Bonzini,
	Alex Bennée, Richard Henderson

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] softmmu/vl.c: fix too slow TCG regression
  2020-02-27  9:15 ` Peter Maydell
@ 2020-02-27  9:55   ` Igor Mammedov
  0 siblings, 0 replies; 5+ messages in thread
From: Igor Mammedov @ 2020-02-27  9:55 UTC (permalink / raw)
  To: Peter Maydell
  Cc: QEMU Developers, Philippe Mathieu-Daudé,
	Niek Linnenbank, Howard Spoelstra, Paolo Bonzini,
	Alex Bennée, Richard Henderson

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
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-02-27  9:56 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26 16:35 [PATCH] softmmu/vl.c: fix too slow TCG regression Igor Mammedov
2020-02-26 18:21 ` Alex Bennée
2020-02-26 19:51   ` Niek Linnenbank
2020-02-27  9:15 ` Peter Maydell
2020-02-27  9:55   ` Igor Mammedov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.