From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42106) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RwxLv-0007Dt-3R for qemu-devel@nongnu.org; Mon, 13 Feb 2012 09:58:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RwxLr-0007v7-I7 for qemu-devel@nongnu.org; Mon, 13 Feb 2012 09:58:02 -0500 Received: from mail-pz0-f45.google.com ([209.85.210.45]:57599) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RwxLr-0007uo-4c for qemu-devel@nongnu.org; Mon, 13 Feb 2012 09:57:59 -0500 Received: by dadp14 with SMTP id p14so5253298dad.4 for ; Mon, 13 Feb 2012 06:57:57 -0800 (PST) Sender: Paolo Bonzini Message-ID: <4F3924EF.5010002@redhat.com> Date: Mon, 13 Feb 2012 15:57:51 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1329144150-7720-1-git-send-email-abarcelo@ac.upc.edu> <1329144150-7720-2-git-send-email-abarcelo@ac.upc.edu> In-Reply-To: <1329144150-7720-2-git-send-email-abarcelo@ac.upc.edu> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] coroutine: adding sigaltstack method (.c source) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Barcelo Cc: Kevin Wolf , qemu-devel@nongnu.org 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 > --- > 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 > + * Copyright (C) 2011 Kevin Wolf > + * Copyright (C) 2012 Alex Barcelo > + * > + * 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 . > + */ > + > +/* > +** This file is partly based on pth_mctx.c, from the GNU Portable Threads > +** Copyright (c) 1999-2006 Ralf S. Engelschall > +** 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 > +#include > +#include > +#include > +#include > +#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