From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LkKeq-0002Vm-P7 for qemu-devel@nongnu.org; Thu, 19 Mar 2009 11:59:48 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LkKel-0002Tz-RM for qemu-devel@nongnu.org; Thu, 19 Mar 2009 11:59:48 -0400 Received: from [199.232.76.173] (port=50549 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LkKel-0002Ts-JO for qemu-devel@nongnu.org; Thu, 19 Mar 2009 11:59:43 -0400 Received: from mx2.redhat.com ([66.187.237.31]:32895) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LkKek-0004zb-Ru for qemu-devel@nongnu.org; Thu, 19 Mar 2009 11:59:43 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n2JFxdRF022199 for ; Thu, 19 Mar 2009 11:59:39 -0400 Message-ID: <49C26BE9.4030905@redhat.com> Date: Thu, 19 Mar 2009 17:59:37 +0200 From: Avi Kivity MIME-Version: 1.0 Subject: Re: [Qemu-devel] [patch 1/7] qemu: mutex/thread/cond wrappers References: <20090319145705.988576615@localhost.localdomain> <20090319150537.801001000@localhost.localdomain> In-Reply-To: <20090319150537.801001000@localhost.localdomain> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org mtosatti@redhat.com wrote: > --- qemu.orig/vl.c > +++ qemu/vl.c > @@ -36,6 +36,7 @@ > #include "gdbstub.h" > #include "qemu-timer.h" > #include "qemu-char.h" > +#include "qemu-thread.h" > #include "cache-utils.h" > #include "block.h" > #include "audio/audio.h" > @@ -263,6 +264,8 @@ static QEMUTimer *nographic_timer; > > uint8_t qemu_uuid[16]; > > +QemuMutex qemu_global_mutex; > + > /***********************************************************/ > /* x86 ISA bus support */ > > @@ -3650,7 +3653,14 @@ void main_loop_wait(int timeout) > slirp_select_fill(&nfds, &rfds, &wfds, &xfds); > } > #endif > + > + /* > + * main_loop_wait() *must* not assume any global state is consistent across > + * select() invocations. > + */ > + qemu_mutex_unlock(&qemu_global_mutex); > ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv); > + qemu_mutex_lock(&qemu_global_mutex); > if (ret > 0) { > IOHandlerRecord **pioh; > > @@ -3708,6 +3718,9 @@ static int main_loop(void) > #endif > CPUState *env; > > + qemu_mutex_init(&qemu_global_mutex); > + qemu_mutex_lock(&qemu_global_mutex); > + > cur_cpu = first_cpu; > next_cpu = cur_cpu->next_cpu ?: first_cpu; > for(;;) { > All the bits above seem unnecessary for the this patch. > + > +int qemu_mutex_init(QemuMutex *mutex) > +{ > + return pthread_mutex_init(&mutex->lock, NULL); > +} > No one will check the return code, so better to check it here and abort() if it's bad. > +static void add_to_timespec(struct timespec *ts, unsigned int msecs) > +{ > + ts->tv_sec = ts->tv_sec + (long)(msecs / 1000); > + ts->tv_nsec = (ts->tv_nsec + ((long)msecs % 1000) * 1000000); > + if (ts->tv_nsec >= 1000000000) { > + ts->tv_nsec -= 1000000000; > + ts->tv_sec++; > + } > +} > timespec_add_msec()? and why milliseconds? Also, maybe uint64_t is a better type. > + > +int qemu_mutex_timedlock(QemuMutex *mutex, unsigned int msecs) > +{ > + struct timespec ts; > + > + clock_gettime(CLOCK_REALTIME, &ts); > + add_to_timespec(&ts, msecs); > + > + return pthread_mutex_timedlock(&mutex->lock, &ts); > +} > I would have preferred a deadline instead of a timeout, but we'll see on the next patches. > + > +int qemu_thread_create(QemuThread *thread, > + void *(*start_routine)(void*), > + void *arg) > +{ > + return pthread_create(&thread->thread, NULL, start_routine, arg); > +} > Don't return an error that no one will check. > + > +int qemu_thread_signal(QemuThread *thread, int sig) > +{ > + if (thread->thread != 0) > + return pthread_kill(thread->thread, sig); > + return -1; /* XXX: ESCHR */ > +} > Ditto. If the thread dies, qemu dies. > +int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2) > +{ > + return (thread1->thread == thread2->thread); > +} > Can compare thing1 to thing2 instead of ->thread. Not that it matters. -- error compiling committee.c: too many arguments to function