From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=35399 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OPAqs-0000pn-Bv for qemu-devel@nongnu.org; Thu, 17 Jun 2010 04:53:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OPAqr-0001Wm-4h for qemu-devel@nongnu.org; Thu, 17 Jun 2010 04:53:34 -0400 Received: from e28smtp08.in.ibm.com ([122.248.162.8]:49335) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OPAqq-0001W7-DH for qemu-devel@nongnu.org; Thu, 17 Jun 2010 04:53:33 -0400 Received: from d28relay05.in.ibm.com (d28relay05.in.ibm.com [9.184.220.62]) by e28smtp08.in.ibm.com (8.14.4/8.13.1) with ESMTP id o5H7iu85004668 for ; Thu, 17 Jun 2010 13:14:56 +0530 Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o5H8rRrx2986196 for ; Thu, 17 Jun 2010 14:23:27 +0530 Received: from d28av05.in.ibm.com (loopback [127.0.0.1]) by d28av05.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o5H8rRkS001063 for ; Thu, 17 Jun 2010 18:53:27 +1000 Date: Thu, 17 Jun 2010 14:23:25 +0530 From: Gautham R Shenoy Message-ID: <20100617085325.GA2849@in.ibm.com> References: <20100616115404.10988.62371.stgit@localhost.localdomain> <20100616115656.10988.96529.stgit@localhost.localdomain> <4C18C4C8.8090901@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4C18C4C8.8090901@redhat.com> Subject: [Qemu-devel] Re: [PATCH V4 2/3] qemu: Generic task offloading framework: threadlets Reply-To: ego@in.ibm.com List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Anthony Liguori , Avi Kivity , Qemu-development List , Corentin Chary , "Aneesh Kumar K.V" On Wed, Jun 16, 2010 at 02:34:16PM +0200, Paolo Bonzini wrote: >> +block-obj-y += qemu-thread.o >> +block-obj-y += async-work.o > > These should be (at least for now) block-obj-$(CONFIG_POSIX). Right. Will fix that. > >> + 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. I think Anthony answered this one. > >> + if (QTAILQ_EMPTY(&(queue->request_list))) >> + goto check_exit; > > What's the reason for the goto? {...} works just as well. Yes {...} works. Besides, this two step condition checking is broken and can cause the threads to exit even in the presence of unprocessed queued ThreadletWork items. Will fix this in the v5 (hopefully there will be one :-)) > >> +/** >> + * 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. That was the other option I had considered before going for barriers, for no particular reason. Now, considering that barriers are not welcome, I will implement this method. > >> +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. Sure, will do that. > >> + * Generalization based on posix-aio emulation code. > > No need to specify these as long as the original authors are attributed > properly. Ok! > >> +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. Will fix this. > >> +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"). Overlooked threadlet_submit() in the rename process. It has to be submit_threadlet(). Will fix. Thanks for the detailed review. Regards gautham. > > Paolo