From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48639) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyN1i-00073i-TD for qemu-devel@nongnu.org; Mon, 16 Nov 2015 11:53:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZyN1g-0007ID-5f for qemu-devel@nongnu.org; Mon, 16 Nov 2015 11:53:10 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53788) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyN1f-0007I0-SG for qemu-devel@nongnu.org; Mon, 16 Nov 2015 11:53:08 -0500 Date: Mon, 16 Nov 2015 16:53:01 +0000 From: "Daniel P. Berrange" Message-ID: <20151116165300.GF20157@redhat.com> 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> <5649A418.2030107@suse.de> <564A07F3.4040200@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <564A07F3.4040200@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: 'Peter Maydell' , David Hildenbrand , Pavel Fedin , qemu-devel@nongnu.org, 'Markus Armbruster' , Christian Borntraeger , Cornelia Huck , 'Paolo Bonzini' On Mon, Nov 16, 2015 at 05:44:35PM +0100, Andreas F=C3=A4rber wrote: > Am 16.11.2015 um 10:38 schrieb Andreas F=C3=A4rber: > > 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: assert= ion > >>>>>> 'ri->version =3D=3D ri->hash_table->version' failed > >>>>>> > >>>>>> (process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assert= ion > >>>>>> 'ri->version =3D=3D ri->hash_table->version' failed > >>>>>> > >>>>>> (process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertio= n > >>>>>> 'ri->version =3D=3D ri->hash_table->version' failed > >>> > >>> Wow... Actually this may come from attempts to modify the tree ins= ide iteration. > >>> > >>>> Thanks! sclp_init() seems to violate several QOM design principles= in > >>>> that it uses object_new() during TypeInfo::instance_init() and use= s a > >>>> TYPE_... constant as property name. But nothing else stands out im= mediately. > >>> > >>> I think we should refactor this and retry. If not all problems go = away, then we are indeed modifying the tree during iteration, and > >>> we have to find some solution. > >> > >> David, Conny, > >> > >> the current tree of afaerber > >> > >> https://github.com/afaerber/qemu-cpu/commits/qom-next > >> > >> has this patch: > >> > >>> From: Pavel Fedin > >>> > >>> ARM GICv3 systems with large number of CPUs create lots of IRQ pins= . Since > >>> every pin is represented as a property, number of these properties = becomes > >>> very large. Every property add first makes sure there's no duplicat= es. > >>> Traversing the list becomes very slow, therefore qemu initializatio= n takes > >>> 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_re= cursive() > >>> cannot modify their objects during traversal, since GHashTableIter = does not > >>> have modify-safe version. However, the code seems not to modify obj= ects via > >>> these functions. > >>> > >>> Signed-off-by: Daniel P. Berrange > >>> Signed-off-by: Pavel Fedin > >> > >> which causes failures in make check. A simple reproducer is > >> > >> qemu-system-s390x -device sclp,help > >> > >> > >> 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? > >=20 > > 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 i= t. > >=20 > > In this case it seems to be about the bus child<> on the event facili= ty. > >=20 > >>> I wonder... Could we have both list and hashtable? hashtable for s= earching by name and list for iteration. In this case we would > >>> not have to use glib's iterators, and would be free of problems wit= h them. Just keep the list and hashtable in sync. > >>> Or, is there any hashtable implementation out there which would ke= ep iterators valid during modification? > >>> OTOH, glib has a function "remove the element at iterator's positi= on", and we could postpone addition. So, perhaps, using both > >>> containers would be an overkill, just refactor the code to adapt to= the new behavior. > >=20 > > My idea, which I wanted to investigate after the weekend, is iteratin= g > > 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 releas= ed > > properties to disappear after prop->release(). >=20 > I went with the latter and squashed the attached fixup (without last tw= o > hunks, preparing a separate patch for that), interrupting each iteratio= n > after prop->release() to be safe. That seems to fix it. >=20 > Will prepend and test Dan's unit test next. > diff --git a/qom/object.c b/qom/object.c > index 0ac3bc1..284fa38 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -377,14 +377,22 @@ static void object_property_del_all(Object *obj) > ObjectProperty *prop; > GHashTableIter iter; > gpointer key, value; > + bool released; > =20 > - g_hash_table_iter_init(&iter, obj->properties); > - while (g_hash_table_iter_next(&iter, &key, &value)) { > - prop =3D value; > - if (prop->release) { > - prop->release(obj, prop->name, prop->opaque); > + do { > + released =3D false; > + g_hash_table_iter_init(&iter, obj->properties); > + while (g_hash_table_iter_next(&iter, &key, &value)) { > + prop =3D value; > + if (prop->release) { > + prop->release(obj, prop->name, prop->opaque); > + prop->release =3D NULL; > + released =3D true; > + break; > + } > + g_hash_table_iter_remove(&iter); > } > - } > + } while (released); > =20 > g_hash_table_unref(obj->properties); > } > @@ -401,7 +409,15 @@ static void object_property_del_child(Object *obj,= Object *child, Error **errp) > if (object_property_is_child(prop) && prop->opaque =3D=3D chil= d) { > if (prop->release) { > prop->release(obj, prop->name, prop->opaque); > + prop->release =3D NULL; > } > + break; > + } > + } > + g_hash_table_iter_init(&iter, obj->properties); > + while (g_hash_table_iter_next(&iter, &key, &value)) { > + prop =3D value; > + if (object_property_is_child(prop) && prop->opaque =3D=3D chil= d) { > g_hash_table_iter_remove(&iter); > break; > } > @@ -856,7 +872,7 @@ void object_ref(Object *obj) > if (!obj) { > return; > } > - atomic_inc(&obj->ref); > + atomic_inc(&obj->ref); > } > =20 > void object_unref(Object *obj) > @@ -864,7 +880,7 @@ void object_unref(Object *obj) > if (!obj) { > return; > } > - g_assert(obj->ref > 0); > + g_assert_cmpint(obj->ref, >, 0); > =20 > /* parent always holds a reference to its children */ > if (atomic_fetch_dec(&obj->ref) =3D=3D 1) { This looks good to me so can add Signed-off-by: Daniel P. Berrange to this change. Regards, Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|