qemu-arm segfaults executing msgmerge (gettext)

We always see this in :

exec.c1662:

void cpu_exit(CPUState *env)
{
    cpu_unlink_tb(env);
    env->exit_request = 1;
}

A quick test with the statement cpu_unlink_tb(env) removed passed the test.

Alternative testcase:
compile and "export OMP_NUM_THREADS=6" before running.

/******************************************************************************
* FILE: omp_mm.c
* DESCRIPTION:
* OpenMp Example - Matrix Multiply - C Version
* Demonstrates a matrix multiply using OpenMP. Threads share row iterations
* according to a predefined chunk size.
* AUTHOR: Blaise Barney
* LAST REVISED: 06/28/05
******************************************************************************/
#include <omp.h>
#include <stdio.h>
#include <stdlib.h>

#define NRA 620 /* number of rows in matrix A */
#define NCA 150 /* number of columns in matrix A */
#define NCB 70 /* number of columns in matrix B */

int main (int argc, char *argv[])
{
int tid, nthreads, i, j, k, chunk;
double a[NRA][NCA], /* matrix A to be multiplied */
        b[NCA][NCB], /* matrix B to be multiplied */
        c[NRA][NCB]; /* result matrix C */

chunk = 10; /* set loop iteration chunk size */

/*** Spawn a parallel region explicitly scoping all variables ***/
#pragma omp parallel shared(a,b,c,nthreads,chunk) private(tid,i,j,k)
  {
  tid = omp_get_thread_num();
  if (tid == 0)
    {
    nthreads = omp_get_num_threads();
    printf("Starting matrix multiple example with %d threads\n",nthreads);
    printf("Initializing matrices...\n");
    }
  /*** Initialize matrices ***/
  #pragma omp for schedule (static, chunk)
  for (i=0; i<NRA; i++)
    for (j=0; j<NCA; j++)
      a[i][j]= i+j;
  #pragma omp for schedule (static, chunk)
  for (i=0; i<NCA; i++)
    for (j=0; j<NCB; j++)
      b[i][j]= i*j;
  #pragma omp for schedule (static, chunk)
  for (i=0; i<NRA; i++)
    for (j=0; j<NCB; j++)
      c[i][j]= 0;

  /*** Do matrix multiply sharing iterations on outer loop ***/
  /*** Display who does which iterations for demonstration purposes ***/
  printf("Thread %d starting matrix multiply...\n",tid);
  #pragma omp for schedule (static, chunk)
  for (i=0; i<NRA; i++)
    {
    printf("Thread=%d did row=%d\n",tid,i);
    for(j=0; j<NCB; j++)
      for (k=0; k<NCA; k++)
        c[i][j] += a[i][k] * b[k][j];
    }
  } /*** End of parallel region ***/

/*** Print results ***/
printf("******************************************************\n");
printf("Result Matrix:\n");
for (i=0; i<NRA; i++)
  {
  for (j=0; j<NCB; j++)
    printf("%6.2f ", c[i][j]);
  printf("\n");
  }
printf("******************************************************\n");
printf ("Done.\n");

}

Thanks for the test case. I can confirm that I can reproduce this. (Detail: compiled with "gcc -fopenmp 668799.c -o 668799 -static" on an Ubuntu maverick ARM system. qemu-arm-user on x86-64 then segfaults when run with "OMP_NUM_THREADS=6 ./arm-linux-user/qemu-arm /tmp/668799".) The point when it segfaults varies, and occasionally it deadlocks instead for variety...
Looks like a fun bug; I'll have a deeper look at this later this week.

To me it looks like racy/double lockings. We already lock meantime before some functions up the code-path of cpu_unlink_tb . IMHO the spinlock in cpu_unlink_tb is now unnecessary - at least in this code-path. Maybe we exit the cpu before the previous/upper lock is released. HTH, have phun!

See linux-user/main.c function start_exclusive.

The following patch stops the segfault (which happens because cpu_unlink_tb() is fiddling with the links between tbs without taking the tb_lock, so another thread can come in via eg tb_add_jump() and cause corruption of the linked lists). However, there are a number of comments in the TB handling code about things being non-thread-safe or not SMP safe, so I need to have a more careful think about the whole thing.

diff --git a/exec.c b/exec.c
index db9ff55..5f4a50b 100644
--- a/exec.c
+++ b/exec.c
@@ -1606,9 +1606,8 @@ static void cpu_unlink_tb(CPUState *env)
        emulation this often isn't actually as bad as it sounds. Often
        signals are used primarily to interrupt blocking syscalls. */
     TranslationBlock *tb;
- static spinlock_t interrupt_lock = SPIN_LOCK_UNLOCKED;

- spin_lock(&interrupt_lock);
+ spin_lock(&tb_lock);
     tb = env->current_tb;
     /* if the cpu is currently executing code, we must unlink it and
        all the potentially executing TB */
@@ -1616,7 +1615,7 @@ static void cpu_unlink_tb(CPUState *env)
         env->current_tb = NULL;
         tb_reset_jump_recursive(tb);
     }
- spin_unlock(&interrupt_lock);
+ spin_unlock(&tb_lock);
 }

 /* mask must never be zero, except for A20 change call */

@Peter, that patch, against 0.13 results in some odd deadlocks; specifically a racey deadlock during signal handling best I can tell.

Attached is an strac'ing of the make process- nothing special, just forking off some children, wait'ing on the results- if you look earlier in the log you'll see the fork/wait working fine (look for SIGCHLD delivery), then see the final one go out to lunch waiting on a futex.

At this point it takes me about 4-5 runs to trigger it, but it's proving fairly reproducible on the builds I've been attempting- cmake in particular seems to trigger it rather quickly.

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).

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).

Thanks for the testing. I had a nasty feeling this wouldn't
be the only problem, which is why I didn't propose that patch
as a real fix.

(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.)

-- PMM

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

Am Mittwoch, 1. Dezember 2010, 20:40:37 schrieb Peter Maydell:
> 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

Thanks for investigation this further!

> 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.

Adding locks everywhere is probably the "save but horribly slow" solution.

> 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?

Sounds reasonable to me.

Brainstorming:
Would per-thread data-structures make any sense ?

-- JSM

For information, I'm *not* able to reproduce this problem with:
    * QEMU user-mode v1.0.1
    * host system: Slackware64-current
    * guest system: Slackware/ARM 13.37
    * build command: gcc -fopenmp 668799.c -o 668799 -static
    * run commands:
          * ./668799
          * OMP_NUM_THREADS=2 ./668799
          * OMP_NUM_THREADS=6 ./668799
          * OMP_NUM_THREADS=16 ./668799
          * msgmerge /dev/null /dev/null (initial test-case)

Cédric.

Just a note.

We still have this issue when building unity on ARM. It crashed when running msgmerge.

The test I'm using in LP:1098729 hangs or segfaults nearly every single run.

Patches have now been committed to QEMU which fix the subset of "multithreaded guests crash" which this bug covers [ie ones where there was a race between tb_unlink_cpu() and the cpu thread using or modifying the TB graph], so I'm closing this bug.

Note that there are still other classes of QEMU bug which also manifest as "my multithreaded guest crashes" -- those are covered by LP:1098729.