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

[Stefan Hajnoczi]

On Tue, Feb 14, 2012 at 11:53 AM, Alex Barcelo <address@hidden> wrote:
> 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:
>>> +    /*
>>> +     * 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?

What I meant is that there are other signals handlers installed and
the signals will be unblocked between the time when sigsuspend() has
returned and before sigaltstack(NULL, &ss) is executed.  This seems
like a race condition to me.

Stefan