Comment 10 for bug 668799

On 28 November 2010 11:24, Peter Maydell <email address hidden> wrote:
> 2010/11/28 Brian Harring <email address hidden>:
>> Additional note... it *looks* like the deadlock potential is there
>> already in 0.13, it's just heavily exacerbated by this patch- out of
>> about 600 builds I've seen 2 lockup in the same fashion (rate was far
>> higher with the patch on this ticket).

> (I think that running multithreaded programs under user-mode
> emulation is effectively hitting a lot of the same locking issues
> that you would get for emulating an MP core in multiple threads.)

Having looked in a bit more detail at the code, I definitely think
the locking is insufficient for the TCG TranslationBlock structures.
In particular:
 * cpu_exit() updates the linked lists in the TB ->jmp_next[] arrays
 * cpu_exit() is called:
   + by other threads, in linux-user mode
   + by signal handlers (both in linux user mode for taking signals
and in system mode via qemu_notify_event() when timers expire)
   + by normal generated code

At the moment nothing blocks signals when it is modifying the TB
jmp_next arrays (either via cpu_exit() or tb_add_jump()), so if you're
unlucky and you take a signal while you're in the middle of modifying
a jmp_next list you might end up with the list corrupted. This is
more likely to happen with multithreaded linux-user mode code I
think, but there's still a possibility there even in pure system mode.

I'm not sure what the best approach to solving this is. We could
add "block signals; take mutex" around all the places in the code
that touch the TB data structures. That seems a bit heavyweight
and it's also not totally clear to me what the best points in the
exec.c code to put the locking are; but it would fix the problem.

Alternatively we could try making cpu_exit() not have to actually
fiddle with the TB graph. To do that you'd need to do one of:
 * explicit checks for a "should we exit" flag at backwards and
indirect branches and every few hundred insns. This is extra
straight-line-code overhead, but on the other hand you get to
avoid having all your cached next-tb links trashed every time
something has to call cpu_exit(), so it's not totally obvious that
it would be a net loss
 * have cpu_exit() force itself to be running in the thread for that
virtual CPU by sending a signal, to avoid the "thread has executed
its way out of the TB" problem that currently requires us to trace
through the whole TB call graph. Then we could do something
simpler and atomic/reentrant to stop the cpu rather than chasing
and editing linked lists

I think on balance I maybe favour the last one, but I'm not
sure. Does anybody have an opinion?

-- PMM