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

[Alex Barcelo]

On Tue, Feb 14, 2012 at 10:24, Stefan Hajnoczi <address@hidden> wrote:
> On Mon, Feb 13, 2012 at 03:42:28PM +0100, Alex Barcelo wrote:
>> +    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?

Paolo has answered this question. But some lighter reading: man sigreturn

Somebody has to call sigreturn. The easiest way is to do a return from
the signal handler. Calling manually sigreturn it's... tricky (and "It
should *never* be called directly.", man's page dixit). Knowing that
qemu has to work in lots of platforms, I wouldn't bet on that. Or on
any clever procmask direct manipulation.

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

Agree. But few coroutines are created (one or two in an average
qemu-system execution, as I have checked). So... well, not sure if
this is our guilty malloc. Anyway, as you say, the behaviour is the
same in ucontext than sigaltstack, so it should not be a matter for
this series of patches.

>> +    /*
>> +     * 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

mmm no, it won't. The sigaction is set for the SIGUSR1 only (yes I
have to change it to sigusr2, the V2 will have this changed). And only
this signal will be handled on an alternate stack (the sa.sa_flags is
the responsible).

I'm not really sure about that, did I miss something?

> and could corrupt the coroutine if the signal is handled
> between sigsuspend(2) and sigaltstack(2).
> Stefan