Re: [Qemu-devel] [PATCH] main: force enabling of I/O thread

[Peter Maydell]

On 22 August 2011 15:00, Paolo Bonzini <address@hidden> wrote:
> On 08/22/2011 03:50 PM, Peter Maydell wrote:
>> Even with iothread it's still signal based (and still racy) -- the only
>> way to get a thread currently executing TCG code to stop doing so is to
>> send it a signal.
>
> It's signal-based, but I'm not sure it's racy when single-threaded.  This:
>
>                /* ... tb_add_jump ... */
>                barrier();
>                if (likely(!env->exit_request)) {
>
> in cpu_exec, vs. this in the signal handler:
>
>        void cpu_exit(CPUState *env)
>        {
>            env->exit_request = 1;
>            cpu_unlink_tb(env);
>        }
>
> together will make sure that only a single basic block is executed after an
> exit request.

If the cpu thread is in the middle of tb_add_jump() (updating the
jmp_next[] circular list) when the signal fires, cpu_unlink_tb()
will also try to modify the same list and corrupt things.

(Also an arbitrary number of basic blocks may be executed because
it's (in theory) possible for the cpu thread to stay one jump ahead
of the thread running cpu_unlink_tb(). This is why cpu_unlink_tb()
has to traverse the entire tb graph from the current tb unlinking
things rather than only operating on the current tb. In practice
of course only one or two tbs will be executed.)

> The problems with user-level emulation arise from multiple threads
> concurrently execute the tb_add_jump or cpu_unlink_tb operations.  My
> knowledge of user-level emulation is approximately zero, but I think it
> should be possible to make the race outcome predictable.  That's because (1)
> cpu_unlink_tb is idempotent; (2) you don't really need to do anything in
> cpu_unlink_tb if the other thread is running tb_add_jump, because setting
> env->exit_request will avoid entering the CPU.

Yeah, but we don't actually do enough locking or other cleverness
at this point, which is why it's racy.

(I think that cpu_unlink_tb() is just fundamentally too much work to be
doing in a signal handler...)

Anyway, dropping non-iothread will reduce the number of configurations
we have to reason about in fixing this kind of thing, which is a good
thing.

-- PMM