[Qemu-devel] Race condition between signal handler and cpu_exec()

[Qemu-devel] Race condition between signal handler and cpu_exec()

[Aurelien Jarno]

Hi,

I have just found the root of a long standing but rare problem, the 
freeze of QEMU for a few seconds when using -clock dynticks. Laurent
Vivier, while working on the DMA support in MACIO remarked that running
'dbench 1' in the PPC target almost always trigger the bug after some
time. This helped finding the problem.

The problem is caused by a race condition between the signal handler and
cpu_exec():
- host_alarm_handler() calls cpu_interrupt(env, CPU_INTERRUPT_EXIT);
- cpu_interrupt(CPUState *env, int mask) does env->interrupt_request |= mask;
- it can happen while cpu_exec() does:

|  if (env->interrupt_request & CPU_INTERRUPT_EXITTB) {
|      env->interrupt_request &= ~CPU_INTERRUPT_EXITTB; 

- the corresponding x86_64 assembly code is:
|	movl    624(%r14), %eax
|	testb   $4, %al
|	je      .L26
|	andl    $-5, %eax
|	movl    %eax, 624(%r14)

That is a non atomic operation. This cause an endless loop in cpu_exec().
I guess it can happens at other place modifying env->interrupt_request,
but this seems to occurs that as this code is executed very often.

This does not happens with clocks other than dynticks, as they trigger
SIGALRM again and again which at the end sets the correct value.

I am currently too tired to find a proper solution (which should only
use read/write to a variable to keep the operations atomic), I'll look
at that tomorrow, but patches are welcome in the meanwhile.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Race condition between signal handler and cpu_exec()

[Jamie Lokier]

Aurelien Jarno wrote:
> I am currently too tired to find a proper solution (which should only
> use read/write to a variable to keep the operations atomic), I'll look
> at that tomorrow, but patches are welcome in the meanwhile.

The theoretically right thing in C is read/write a "volatile
sig_atomic_t".  The GNU/Linux libc manual has a section "Atomic Data
Access and Signal Handling", where it says you can assume read/writing
an "int" and a pointer are also atomic in this respect on all machines
which matter.

Note that writing to a "char" is not atomic on old Alphas.

-- Jamie

Re: [Qemu-devel] Race condition between signal handler and cpu_exec()

[Julian Seward]

On Friday 06 March 2009, Jamie Lokier wrote:
> Aurelien Jarno wrote:
> > I am currently too tired to find a proper solution (which should only
> > use read/write to a variable to keep the operations atomic), I'll look
> > at that tomorrow, but patches are welcome in the meanwhile.
>
> The theoretically right thing in C is read/write a "volatile
> sig_atomic_t".

It looks to me like this requires to atomically test that a bit in a
byte is set, and if so clear it.  That would require a lock;cmpxchg
sequence on x86 and lwarx/stwcx on ppc.  I wonder if it can be done
with gcc's __sync_bool_compare_and_swap builtin, in order to make
it portable.

J

Re: [Qemu-devel] Race condition between signal handler and cpu_exec()

[malc]

On Fri, 6 Mar 2009, Julian Seward wrote:

> On Friday 06 March 2009, Jamie Lokier wrote:
> > Aurelien Jarno wrote:
> > > I am currently too tired to find a proper solution (which should only
> > > use read/write to a variable to keep the operations atomic), I'll look
> > > at that tomorrow, but patches are welcome in the meanwhile.
> >
> > The theoretically right thing in C is read/write a "volatile
> > sig_atomic_t".
> 
> It looks to me like this requires to atomically test that a bit in a
> byte is set, and if so clear it.  That would require a lock;cmpxchg
> sequence on x86 and lwarx/stwcx on ppc.  I wonder if it can be done
> with gcc's __sync_bool_compare_and_swap builtin, in order to make
> it portable.

Just for the record GCCs sync builtins involve full memory barriers which
are unnecessary here.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Race condition between signal handler and cpu_exec()

[Jamie Lokier]

Julian Seward wrote:
> On Friday 06 March 2009, Jamie Lokier wrote:
> > Aurelien Jarno wrote:
> > > I am currently too tired to find a proper solution (which should only
> > > use read/write to a variable to keep the operations atomic), I'll look
> > > at that tomorrow, but patches are welcome in the meanwhile.
> >
> > The theoretically right thing in C is read/write a "volatile
> > sig_atomic_t".
> 
> It looks to me like this requires to atomically test that a bit in a
> byte is set, and if so clear it.

No, it only requires that if you keep with the same representation.

If you move that bit into its very own variable, just for the one bit,
you can read/write the whole variable.  _That's_ portable (assuming a
suitable variable type is used).

-- Jamie