From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33845 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OOrp5-0005gr-0D for qemu-devel@nongnu.org; Wed, 16 Jun 2010 08:34:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OOrp3-0000IL-Pc for qemu-devel@nongnu.org; Wed, 16 Jun 2010 08:34:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2092) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OOrp3-0000I7-Ga for qemu-devel@nongnu.org; Wed, 16 Jun 2010 08:34:25 -0400 Message-ID: <4C18C4C8.8090901@redhat.com> Date: Wed, 16 Jun 2010 14:34:16 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <20100616115404.10988.62371.stgit@localhost.localdomain> <20100616115656.10988.96529.stgit@localhost.localdomain> In-Reply-To: <20100616115656.10988.96529.stgit@localhost.localdomain> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH V4 2/3] qemu: Generic task offloading framework: threadlets List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gautham R Shenoy Cc: Anthony Liguori , Avi Kivity , Qemu-development List , Corentin Chary , "Aneesh Kumar K.V" > +block-obj-y += qemu-thread.o > +block-obj-y += async-work.o These should be (at least for now) block-obj-$(CONFIG_POSIX). > + while (QTAILQ_EMPTY(&(queue->request_list))&& > + (ret != ETIMEDOUT)) { > + ret = qemu_cond_timedwait(&(queue->cond), > + &(queue->lock), 10*100000); > + } Using qemu_cond_timedwait is a hack for not properly broadcasting the condvar in flush_threadlet_queue. > + if (QTAILQ_EMPTY(&(queue->request_list))) > + goto check_exit; What's the reason for the goto? {...} works just as well. > +/** > + * flush_threadlet_queue: Wait till completion of all the submitted tasks > + * @queue: Queue containing the tasks we're waiting on. > + */ > +void flush_threadlet_queue(ThreadletQueue *queue) > +{ > + qemu_mutex_lock(&queue->lock); > + queue->exit = 1; > + > + qemu_barrier_init(&queue->barr, queue->cur_threads + 1); > + qemu_mutex_unlock(&queue->lock); > + > + qemu_barrier_wait(&queue->barr); Can be implemented just as well with queue->cond and a loop waiting for queue->cur_threads == 0. This would remove the need to implement barriers in qemu-threads (especially for Win32). Anyway whoever will contribute Win32 qemu-threads can do it, since it's not hard. > +int cancel_threadlet_common(ThreadletWork *work) > +{ > + return cancel_threadlet(&globalqueue, work); > +} I would prefer *_threadlet to be the globalqueue function (and flush_threadlets) and queue_*_threadlet to be the special-queue function. I should have spoken earlier probably, but please consider this if there will be a v5. > + * Generalization based on posix-aio emulation code. No need to specify these as long as the original authors are attributed properly. > +static inline void threadlet_queue_init(ThreadletQueue *queue, > + int max_threads, int min_threads) > +{ > + queue->cur_threads = 0; > + queue->idle_threads = 0; > + queue->exit = 0; > + queue->max_threads = max_threads; > + queue->min_threads = min_threads; > + QTAILQ_INIT(&(queue->request_list)); > + QTAILQ_INIT(&(queue->threadlet_work_pool)); > + qemu_mutex_init(&(queue->lock)); > + qemu_cond_init(&(queue->cond)); > +} No need to make this inline. > +extern void threadlet_submit(ThreadletQueue *queue, > + ThreadletWork *work); > + > +extern void threadlet_submit_common(ThreadletWork *work); > + > +extern int cancel_threadlet(ThreadletQueue *queue, ThreadletWork *work); > +extern int cancel_threadlet_common(ThreadletWork *work); > + > + > +extern void flush_threadlet_queue(ThreadletQueue *queue); > +extern void flush_common_threadlet_queue(void); Please make the position of the verb consistent (e.g. "submit_threadlet"). Paolo