On Tue, Feb 14, 2012 at 10:24, Stefan Hajnoczi <stefa...@gmail.com> 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
