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

[Paolo Bonzini]

On 02/13/2012 03:42 PM, Alex Barcelo wrote:
> This file is based in both coroutine-ucontext.c and
> pth_mctx.c (from the GNU Portable Threads library).
>
> The mechanism used to change stacks is the sigaltstack
> function (variant 2 of the pth library).
>
> Signed-off-by: Alex Barcelo <address@hidden>
> ---
>  coroutine-sigaltstack.c |  337 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 337 insertions(+), 0 deletions(-)
>  create mode 100644 coroutine-sigaltstack.c
>
> diff --git a/coroutine-sigaltstack.c b/coroutine-sigaltstack.c
> new file mode 100644
> index 0000000..1d4f26d
> --- /dev/null
> +++ b/coroutine-sigaltstack.c
> @@ -0,0 +1,337 @@
> +/*
> + * sigaltstack coroutine initialization code
> + *
> + * Copyright (C) 2006  Anthony Liguori <address@hidden>
> + * Copyright (C) 2011  Kevin Wolf <address@hidden>
> + * Copyright (C) 2012  Alex Barcelo <address@hidden>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.0 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> +** This file is partly based on pth_mctx.c, from the GNU Portable Threads
> +**  Copyright (c) 1999-2006 Ralf S. Engelschall <address@hidden>
> +**  Same license (version 2.1 or later)
> +*/
> +
> +/* XXX Is there a nicer way to disable glibc's stack check for longjmp? */
> +#ifdef _FORTIFY_SOURCE
> +#undef _FORTIFY_SOURCE
> +#endif
> +#include <stdlib.h>
> +#include <setjmp.h>
> +#include <stdint.h>
> +#include <pthread.h>
> +#include <signal.h>
> +#include "qemu-common.h"
> +#include "qemu-coroutine-int.h"
> +
> +enum {
> +    /* Maximum free pool size prevents holding too many freed coroutines */
> +    POOL_MAX_SIZE = 64,
> +};
> +
> +/** Free list to speed up creation */
> +static QLIST_HEAD(, Coroutine) pool = QLIST_HEAD_INITIALIZER(pool);
> +static unsigned int pool_size;
> +
> +typedef struct {
> +    Coroutine base;
> +    void *stack;
> +    jmp_buf env;
> +} CoroutineUContext;
> +
> +/**
> + * Per-thread coroutine bookkeeping
> + */
> +typedef struct {
> +    /** Currently executing coroutine */
> +    Coroutine *current;
> +
> +    /** The default coroutine */
> +    CoroutineUContext leader;
> +} CoroutineThreadState;
> +
> +static pthread_key_t thread_state_key;
> +
> +/*
> + * the way to pass information to the signal handler (trampoline)
> + * It's not thread-safe, as can be seen, but there is no other simple way.
> + */
> +static volatile jmp_buf      tr_reenter;
> +static volatile sig_atomic_t tr_called;

Unlike pth, we can assume thread-local storage:   these should be placed 
in CoroutineThreadState and coroutine_get_thread_state() used to access 
them.

> +    /*
> +     * Preserve the SIGUSR1 signal state, block SIGUSR1,
> +     * and establish our signal handler. The signal will
> +     * later transfer control onto the signal stack.
> +     */

We're already using SIGUSR1.  Can you switch to SIGUSR2?

> +    sigemptyset(&sigs);
> +    sigaddset(&sigs, SIGUSR1);
> +    sigprocmask(SIG_BLOCK, &sigs, &osigs);

This should be pthread_sigmask.

> +    /*
> +     * Restore the old SIGUSR1 signal handler and mask
> +     */
> +    sigaction(SIGUSR1, &osa, NULL);
> +    sigprocmask(SIG_SETMASK, &osigs, NULL);
> +
> +    /*
> +     * Now enter the trampoline again, but this time not as a signal
> +     * handler. Instead we jump into it directly. The functionally
> +     * redundant ping-pong pointer arithmentic is neccessary to avoid
> +     * type-conversion warnings related to the `volatile' qualifier and
> +     * the fact that `jmp_buf' usually is an array type.
> +     */
> +    if (!setjmp(old_env)) {
> +        longjmp(*((jmp_buf *)&tr_reenter), 1);
> +    }

Use thread-local storage and you'll be able to remove this ugliness,
too.

Overall it looks good, however I think if it is good it should replace 
coroutine-ucontext.c altogether.  Other thoughts?

Paolo