[Qemu-devel] [PATCH v2] Do not use slow [*] expansion for GPIO creation

[Qemu-devel] [PATCH v2] Do not use slow [*] expansion for GPIO creation

[Pavel Fedin]

Expansion of [*] suffix is very slow because index expansion is done using
trial and error strategy, starting every time from zero and retrying with
the next index until insertion succeeds. With large number of already added
properties this process takes huge amount of time (O(n^2) complexity).

Some architectures (like ARM) use very large amount of IRQ pins in interrupt
controller models. This flaw makes machine startup extremely slow
(~20 seconds for ARM64 with 32 CPUs. This patch decreases this time down to
~10 seconds.

Also in qdev_init_gpio_out_named() memset() is now called only once for the
whole array instead of per-cell cleaning

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/core/qdev.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index b2f404a..1d15736 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -417,17 +417,21 @@ void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
 {
     int i;
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
-    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-in");
 
     assert(gpio_list->num_out == 0 || !name);
     gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
                                      dev, n);
 
+    if (!name) {
+        name = "unnamed-gpio-in";
+    }
     for (i = gpio_list->num_in; i < gpio_list->num_in + n; i++) {
+        gchar *propname = g_strdup_printf("%s[%u]", name, i);
+
         object_property_add_child(OBJECT(dev), propname,
                                   OBJECT(gpio_list->in[i]), &error_abort);
+        g_free(propname);
     }
-    g_free(propname);
 
     gpio_list->num_in += n;
 }
@@ -442,20 +446,25 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
 {
     int i;
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
-    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
 
     assert(gpio_list->num_in == 0 || !name);
-    gpio_list->num_out += n;
 
+    if (!name) {
+        name = "unnamed-gpio-out";
+    }
+    memset(pins, 0, sizeof(*pins) * n);
     for (i = 0; i < n; ++i) {
-        memset(&pins[i], 0, sizeof(*pins));
+        gchar *propname = g_strdup_printf("%s[%u]", name,
+                                          gpio_list->num_out + i);
+
         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
                                  (Object **)&pins[i],
                                  object_property_allow_set_link,
                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
                                  &error_abort);
+        g_free(propname);
     }
-    g_free(propname);
+    gpio_list->num_out += n;
 }
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
-- 
1.9.5.msysgit.0

Re: [Qemu-devel] [PATCH v2] Do not use slow [*] expansion for GPIO creation

[Daniel P. Berrange]

On Fri, Jul 31, 2015 at 03:23:22PM +0300, Pavel Fedin wrote:
> Expansion of [*] suffix is very slow because index expansion is done using
> trial and error strategy, starting every time from zero and retrying with
> the next index until insertion succeeds. With large number of already added
> properties this process takes huge amount of time (O(n^2) complexity).
> 
> Some architectures (like ARM) use very large amount of IRQ pins in interrupt
> controller models. This flaw makes machine startup extremely slow
> (~20 seconds for ARM64 with 32 CPUs. This patch decreases this time down to
> ~10 seconds.
> 
> Also in qdev_init_gpio_out_named() memset() is now called only once for the
> whole array instead of per-cell cleaning
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/core/qdev.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2] Do not use slow [*] expansion for GPIO creation

[Andreas Färber]

Am 31.07.2015 um 14:26 schrieb Daniel P. Berrange:
> On Fri, Jul 31, 2015 at 03:23:22PM +0300, Pavel Fedin wrote:
>> Expansion of [*] suffix is very slow because index expansion is done using
>> trial and error strategy, starting every time from zero and retrying with
>> the next index until insertion succeeds. With large number of already added
>> properties this process takes huge amount of time (O(n^2) complexity).
>>
>> Some architectures (like ARM) use very large amount of IRQ pins in interrupt
>> controller models. This flaw makes machine startup extremely slow
>> (~20 seconds for ARM64 with 32 CPUs. This patch decreases this time down to
>> ~10 seconds.
>>
>> Also in qdev_init_gpio_out_named() memset() is now called only once for the
>> whole array instead of per-cell cleaning
>>
>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
>> ---
>>  hw/core/qdev.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

Thanks, queued on qom-next now:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2] Do not use slow [*] expansion for GPIO creation

[Paolo Bonzini]

On 31/07/2015 14:23, Pavel Fedin wrote:
> Expansion of [*] suffix is very slow because index expansion is done using
> trial and error strategy, starting every time from zero and retrying with
> the next index until insertion succeeds. With large number of already added
> properties this process takes huge amount of time (O(n^2) complexity).
> 
> Some architectures (like ARM) use very large amount of IRQ pins in interrupt
> controller models. This flaw makes machine startup extremely slow
> (~20 seconds for ARM64 with 32 CPUs. This patch decreases this time down to
> ~10 seconds.
> 
> Also in qdev_init_gpio_out_named() memset() is now called only once for the
> whole array instead of per-cell cleaning
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/core/qdev.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index b2f404a..1d15736 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -417,17 +417,21 @@ void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
>  {
>      int i;
>      NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
> -    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-in");
>  
>      assert(gpio_list->num_out == 0 || !name);
>      gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
>                                       dev, n);
>  
> +    if (!name) {
> +        name = "unnamed-gpio-in";
> +    }
>      for (i = gpio_list->num_in; i < gpio_list->num_in + n; i++) {
> +        gchar *propname = g_strdup_printf("%s[%u]", name, i);
> +
>          object_property_add_child(OBJECT(dev), propname,
>                                    OBJECT(gpio_list->in[i]), &error_abort);
> +        g_free(propname);
>      }
> -    g_free(propname);
>  
>      gpio_list->num_in += n;
>  }
> @@ -442,20 +446,25 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>  {
>      int i;
>      NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
> -    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
>  
>      assert(gpio_list->num_in == 0 || !name);
> -    gpio_list->num_out += n;
>  
> +    if (!name) {
> +        name = "unnamed-gpio-out";
> +    }
> +    memset(pins, 0, sizeof(*pins) * n);
>      for (i = 0; i < n; ++i) {
> -        memset(&pins[i], 0, sizeof(*pins));
> +        gchar *propname = g_strdup_printf("%s[%u]", name,
> +                                          gpio_list->num_out + i);
> +
>          object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
>                                   (Object **)&pins[i],
>                                   object_property_allow_set_link,
>                                   OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                                   &error_abort);
> +        g_free(propname);
>      }
> -    g_free(propname);
> +    gpio_list->num_out += n;
>  }
>  
>  void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
> 

Thanks, this looks good.  I'm not the maintainer, but I've queued it
anyway locally so that it's not forgotten.

Paolo