From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43036) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyGFD-00008s-Qe for qemu-devel@nongnu.org; Mon, 16 Nov 2015 04:38:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZyGFA-0007Fk-KZ for qemu-devel@nongnu.org; Mon, 16 Nov 2015 04:38:39 -0500 Received: from mx2.suse.de ([195.135.220.15]:58174) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyGFA-0007FY-Ae for qemu-devel@nongnu.org; Mon, 16 Nov 2015 04:38:36 -0500 References: <1444739866-14798-1-git-send-email-berrange@redhat.com> <1444739866-14798-7-git-send-email-berrange@redhat.com> <5646286B.2030307@suse.de> <56464F8A.3070709@de.ibm.com> <56465533.3030501@suse.de> <008001d1203e$51838510$f48a8f30$@samsung.com> <564990C9.1060801@de.ibm.com> From: =?UTF-8?Q?Andreas_F=c3=a4rber?= Message-ID: <5649A418.2030107@suse.de> Date: Mon, 16 Nov 2015 10:38:32 +0100 MIME-Version: 1.0 In-Reply-To: <564990C9.1060801@de.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christian Borntraeger , Pavel Fedin Cc: 'Peter Maydell' , qemu-devel@nongnu.org, 'Markus Armbruster' , David Hildenbrand , Cornelia Huck , 'Paolo Bonzini' Am 16.11.2015 um 09:16 schrieb Christian Borntraeger: > On 11/16/2015 08:13 AM, Pavel Fedin wrote: >>>>> (process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion >>>>> 'ri->version =3D=3D ri->hash_table->version' failed >>>>> >>>>> (process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion >>>>> 'ri->version =3D=3D ri->hash_table->version' failed >>>>> >>>>> (process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertion >>>>> 'ri->version =3D=3D ri->hash_table->version' failed >> >> Wow... Actually this may come from attempts to modify the tree inside= iteration. >> >>> Thanks! sclp_init() seems to violate several QOM design principles in >>> that it uses object_new() during TypeInfo::instance_init() and uses a >>> TYPE_... constant as property name. But nothing else stands out immed= iately. >> >> I think we should refactor this and retry. If not all problems go awa= y, then we are indeed modifying the tree during iteration, and >> we have to find some solution. >=20 > David, Conny, >=20 > the current tree of afaerber >=20 > https://github.com/afaerber/qemu-cpu/commits/qom-next >=20 > has this patch: >=20 >> From: Pavel Fedin >> >> ARM GICv3 systems with large number of CPUs create lots of IRQ pins. S= ince >> every pin is represented as a property, number of these properties bec= omes >> very large. Every property add first makes sure there's no duplicates. >> Traversing the list becomes very slow, therefore qemu initialization t= akes >> significant time (several seconds for e. g. 16 CPUs). >> >> This patch replaces list with GHashTable, making lookup very fast. The= only >> drawback is that object_child_foreach() and object_child_foreach_recur= sive() >> cannot modify their objects during traversal, since GHashTableIter doe= s not >> have modify-safe version. However, the code seems not to modify object= s via >> these functions. >> >> Signed-off-by: Daniel P. Berrange >> Signed-off-by: Pavel Fedin >=20 > which causes failures in make check. A simple reproducer is >=20 > qemu-system-s390x -device sclp,help >=20 >=20 > any idea what would be the most simple fix? > Can we refactor this to create the event facility and the bus in the > machine or whatever? I believe it is rather a very general problem with the new object_property_del_all() implementation. It iterates through properties, releasing child<> and link<> properties, which results in an unref, which at some point unparents that device, removing it in the parent's properties hashtable while the parent is iterating through it. In this case it seems to be about the bus child<> on the event facility. >> I wonder... Could we have both list and hashtable? hashtable for sear= ching by name and list for iteration. In this case we would >> not have to use glib's iterators, and would be free of problems with t= hem. Just keep the list and hashtable in sync. >> Or, is there any hashtable implementation out there which would keep = iterators valid during modification? >> OTOH, glib has a function "remove the element at iterator's position"= , and we could postpone addition. So, perhaps, using both >> containers would be an overkill, just refactor the code to adapt to th= e new behavior. My idea, which I wanted to investigate after the weekend, is iterating through the hashtable to create a list of prop->release functions and call them only after finishing the iteration. That might not work either, so we may need to loop over the releasing to allow for released properties to disappear after prop->release(). Regards, Andreas --=20 SUSE Linux GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Felix Imend=F6rffer, Jane Smithard, Graham Norton; HRB 21284 (AG N=FC= rnberg)