From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34446) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKRVt-0004hT-Ee for qemu-devel@nongnu.org; Wed, 29 Jul 2015 09:35:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKRVq-0004Tg-8l for qemu-devel@nongnu.org; Wed, 29 Jul 2015 09:35:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57153) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKRVq-0004Rz-2H for qemu-devel@nongnu.org; Wed, 29 Jul 2015 09:35:14 -0400 Date: Wed, 29 Jul 2015 14:34:59 +0100 From: "Daniel P. Berrange" Message-ID: <20150729133459.GE16847@redhat.com> References: <019001d0c9fd$d3268410$79738c30$@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <019001d0c9fd$d3268410$79738c30$@samsung.com> Subject: Re: [Qemu-devel] [PATCH] Do not use slow [*] expansion for GPIO creation Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Fedin Cc: 'Paolo Bonzini' , 'QEMU Developers' , 'Markus Armbruster' , 'Andreas =?utf-8?Q?F=C3=A4rber'?= On Wed, Jul 29, 2015 at 03:55:14PM +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. > > Signed-off-by: Pavel Fedin > --- > hw/core/qdev.c | 29 ++++++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index b2f404a..d285784 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -25,6 +25,8 @@ > inherit from a particular bus (e.g. PCI or I2C) rather than > this API directly. */ > > +#include > + > #include "hw/qdev.h" > #include "hw/fw-path-provider.h" > #include "sysemu/sysemu.h" > @@ -415,15 +417,24 @@ static NamedGPIOList *qdev_get_named_gpio_list(DeviceState *dev, > void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler, > const char *name, int n) > { > - int i; > + int i, l; > NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name); > - char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-in"); > + char *propname; > > assert(gpio_list->num_out == 0 || !name); > + > + if (!name) { > + name = "unnamed-gpio-in"; > + } > + l = strlen(name); > + propname = g_malloc(l + 13); /* 10 characters for UINT_MAX plus "[]" */ > + memcpy(propname, name, l); Please don't do manual string length calculations in combination with unbounded sprintf calls. It is a recipe for future security bugs. > + > gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler, > dev, n); > > for (i = gpio_list->num_in; i < gpio_list->num_in + n; i++) { > + g_sprintf(&propname[l], "[%u]", i); Replace this with 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); > } > @@ -440,14 +451,21 @@ void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n) > void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins, > const char *name, int n) > { > - int i; > + int i, l; > NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name); > - char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out"); > + char *propname; > > assert(gpio_list->num_in == 0 || !name); > - gpio_list->num_out += n; > + > + if (!name) { > + name = "unnamed-gpio-out"; > + } > + l = strlen(name); > + propname = g_malloc(l + 13); /* 10 characters for UINT_MAX plus "[]" */ > + memcpy(propname, name, l); Same again here. > > for (i = 0; i < n; ++i) { > + g_sprintf(&propname[l], "[%u]", gpio_list->num_out + i); > memset(&pins[i], 0, sizeof(*pins)); > object_property_add_link(OBJECT(dev), propname, TYPE_IRQ, > (Object **)&pins[i], > @@ -456,6 +474,7 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins, > &error_abort); > } > g_free(propname); > + gpio_list->num_out += n; > } 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 :|