Re: [Qemu-devel] [PATCH v4 6/7] qom: replace object property list with GHashTable

[Andreas Färber]

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 == ri->hash_table->version' failed
>>>>>
>>>>> (process:4102): GLib-CRITICAL **: g_hash_table_iter_next: assertion
>>>>> 'ri->version == ri->hash_table->version' failed
>>>>>
>>>>> (process:4102): GLib-CRITICAL **: iter_remove_or_steal: assertion
>>>>> 'ri->version == 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 immediately.
>>
>>  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 <address@hidden>
>>
>> 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 duplicates.
>> Traversing the list becomes very slow, therefore qemu initialization 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_recursive()
>> cannot modify their objects during traversal, since GHashTableIter does not
>> have modify-safe version. However, the code seems not to modify objects via
>> these functions.
>>
>> Signed-off-by: Daniel P. Berrange <address@hidden>
>> Signed-off-by: Pavel Fedin <address@hidden>
> 
> 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?

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 searching 
>> 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 them. 
>> 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 the 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

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