Re: [Qemu-devel] [PATCH] linux-user: call cpu_copy under clone_lock

["Alex Bennée" <alex.bennee@linaro.org>]

Max Filippov <jcmvbkbc@gmail.com> writes:

> Hi Alex,
>
> On Tue, Apr 3, 2018 at 9:26 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>> Max Filippov <jcmvbkbc@gmail.com> writes:
>>
>>> cpu_copy adds newly created CPU object to container/machine/unattached,
>>> but does it w/o proper locking. As a result when multiple threads are
>>> created rapidly QEMU may abort with the following message:
>>>
>>>   GLib-CRITICAL **: g_hash_table_iter_next: assertion
>>>   'ri->version == ri->hash_table->version' failed
>>>
>>>   ERROR:qemu/qom/object.c:1663:object_get_canonical_path_component:
>>>   code should not be reached
>>>
>>> Move cpu_copy invocation under clone_lock to fix that.
>>
>> So my main concern is are we duplicating something already (should be?)
>> handled by fork_start/fork_end?
>
> clone_lock already exists, it protects state in case of thread creation,
> it just didn't protect enough of it.
>
> The work done by fork_start/fork_end appears to be heavier than
> what's needed for thread creation, because fork_start stops all
> other CPUs (to make sure that child process won't get locks owned
> by threads that no longer exist in the child process), which is not
> required for thread creation, hence thread creation uses clone_lock.

I'm wondering if it should be doing more. After all start/end_exclusive
rely on the cpu list and that isn't updated on thread creation - and
without that a bunch of other things fail like ld/st exclusive after
your first new thread is spawned.

This really needs some test cases to check.

So while I think clone_lock fixes this immediate problem I suspect there
is more to do for this case.

>
>> This serialises forks and ensures things like the cpu_list (which ~ a
>> thread list for linux-user) are updated safely.


--
Alex Bennée