Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source)

[Stefan Hajnoczi]

On Mon, Feb 13, 2012 at 03:42:28PM +0100, Alex Barcelo wrote:
> +/*
> + * This is used as the signal handler. This is called with the brand new 
> stack
> + * (thanks to sigaltstack). We have to return, given that this is a signal
> + * handler and the sigmask and some other things are changed.
> + */
> +static void coroutine_trampoline(int signal)
> +{
> +    CoroutineUContext *self;
> +    Coroutine *co;
> +
> +    /* This will break on multithread or in any race condition */
> +    self = ptr_for_handler;
> +    tr_called = 1;
> +    co = &self->base;
> +
> +    /*
> +     * Here we have to do a bit of a ping pong between the caller, given that
> +     * this is a signal handler and we have to do a return "soon". Then the
> +     * caller can reestablish everything and do a longjmp here again.
> +     */
> +    if (!setjmp(*((jmp_buf *)&tr_reenter))) {
> +        return;
> +    }

setjmp() followed by return is usually bad.  We're relying on the fact
that the return code path here does not clobber local variables 'self'
and 'co'.  Can't we longjmp out back to the coroutine_new() function
instead?

> +static Coroutine *coroutine_new(void)
> +{
> +    const size_t stack_size = 1 << 20;

This reminds me of a recent observation that QEMU is using a lot of
memory in a bursty pattern.  I wonder if coroutine stacks are the cause
for this behavior - they are pretty big.  Not a specific problem with
your implementation since we do the same for other coroutine
implementations.

> +    /*
> +     * Preserve the SIGUSR1 signal state, block SIGUSR1,
> +     * and establish our signal handler. The signal will
> +     * later transfer control onto the signal stack.
> +     */
> +    sigemptyset(&sigs);
> +    sigaddset(&sigs, SIGUSR1);
> +    sigprocmask(SIG_BLOCK, &sigs, &osigs);
> +    sa.sa_handler = coroutine_trampoline;
> +    sigfillset(&sa.sa_mask);
> +    sa.sa_flags = SA_ONSTACK;
> +    if (sigaction(SIGUSR1, &sa, &osa) != 0) {
> +        abort();
> +    }
> +
> +    /*
> +     * Set the new stack.
> +     */
> +    ss.ss_sp = co->stack;
> +    ss.ss_size = stack_size;
> +    ss.ss_flags = 0;
> +    if (sigaltstack(&ss, &oss) < 0) {
> +        abort();
> +    }
> +
> +    /*
> +     * Now transfer control onto the signal stack and set it up.
> +     * It will return immediately via "return" after the setjmp()
> +     * was performed. Be careful here with race conditions.  The
> +     * signal can be delivered the first time sigsuspend() is
> +     * called.
> +     */
> +    tr_called = 0;
> +    kill(getpid(), SIGUSR1);
> +    sigfillset(&sigs);
> +    sigdelset(&sigs, SIGUSR1);
> +    while (!tr_called) {
> +        sigsuspend(&sigs);
> +    }
> +
> +    /*
> +     * Inform the system that we are back off the signal stack by
> +     * removing the alternative signal stack. Be careful here: It
> +     * first has to be disabled, before it can be removed.
> +     */
> +    sigaltstack(NULL, &ss);

What happens when a vcpu thread creates a coroutine while another QEMU
thread raises SIG_IPI?  The SIG_IPI will be handled on the alternate
signal stack and could corrupt the coroutine if the signal is handled
between sigsuspend(2) and sigaltstack(2).

Stefan