From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=44353 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OMg59-0003ga-V2 for qemu-devel@nongnu.org; Thu, 10 Jun 2010 07:38:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OMg58-0005uj-RG for qemu-devel@nongnu.org; Thu, 10 Jun 2010 07:37:59 -0400 Received: from e28smtp01.in.ibm.com ([122.248.162.1]:47646) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OMg57-0005u5-Pw for qemu-devel@nongnu.org; Thu, 10 Jun 2010 07:37:58 -0400 Received: from d28relay05.in.ibm.com (d28relay05.in.ibm.com [9.184.220.62]) by e28smtp01.in.ibm.com (8.14.4/8.13.1) with ESMTP id o5ABbnQ1011556 for ; Thu, 10 Jun 2010 17:07:49 +0530 Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o5ABbncr1630360 for ; Thu, 10 Jun 2010 17:07:49 +0530 Received: from d28av02.in.ibm.com (loopback [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o5ABbmjB028736 for ; Thu, 10 Jun 2010 21:37:49 +1000 Date: Thu, 10 Jun 2010 17:07:45 +0530 From: Gautham R Shenoy Subject: Re: [Qemu-devel] [PATCH V3 2/3] qemu: Generic asynchronous threading framework to offload tasks Message-ID: <20100610113745.GC11253@in.ibm.com> References: <20100603085223.25546.88499.stgit@localhost.localdomain> <20100603085623.25546.14585.stgit@localhost.localdomain> <4C08FCA3.3020602@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4C08FCA3.3020602@codemonkey.ws> Reply-To: ego@in.ibm.com List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: "Aneesh Kumar K.V" , Qemu-development List , Corentin Chary , Avi Kivity On Fri, Jun 04, 2010 at 08:16:19AM -0500, Anthony Liguori wrote: >> --- /dev/null >> +++ b/async-work.c >> @@ -0,0 +1,136 @@ >> +/* >> + * Async work support >> + * >> + * Copyright IBM, Corp. 2010 >> + * >> + * Authors: >> + * Aneesh Kumar K.V >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2. See >> + * the COPYING file in the top-level directory. >> > > Please preserve the original copyright of the copied code. Will update the comment containing the Copyright. > >> + >> +struct work_item >> +{ >> + QTAILQ_ENTRY(work_item) node; >> + void (*func)(struct work_item *work); >> + void *private; >> +}; >> > > Structs are not named in accordance to CODING_STYLE. Will fix this. > >> +static inline void async_queue_init(struct async_queue *queue, >> + int max_threads, int min_threads) >> +{ >> + queue->cur_threads = 0; >> + queue->idle_threads = 0; >> + queue->max_threads = max_threads; >> + queue->min_threads = min_threads; >> + QTAILQ_INIT(&(queue->request_list)); >> + QTAILQ_INIT(&(queue->work_item_pool)); >> + qemu_mutex_init(&(queue->lock)); >> + qemu_cond_init(&(queue->cond)); >> +} >> > > I'd prefer there be a single queue that everything used verses multiple > queues. Otherwise, we'll end up having per device queues and my concern is > that we'll end up with thousands and thousands of threads with no central > place to tune the maximum thread number. Aah! So, the original idea was to have a single queue, but since we were making it generic, we thought that the subsystems might like the flexibility of having their own queue. I suppose we are not looking to differentiate between the worker threads belonging to different subsystems in terms of their relative importance/priorities, right ? > >> +static inline struct work_item *async_work_init(struct async_queue *queue, >> + void (*func)(struct work_item *), >> + void *data) >> > > I'd suggest actually using a Notifier as the worker or at least something > that looks exactly like it. There's no need to pass a void * because more > often than not, a caller just wants to pass a state structure anyway and > they can embed the Notifier within the structure. IOW: > > async_work_submit(queue, &s->worker); > > Then in the callback: > > DeviceState *s = container_of(worker, DeviceState, worker); > > I don't think the name makes the most sense either. I think something like: > > threadlet_submit() Makes sense. Will implement this. > > Would work best. It would be good for there to be a big comment warning > that the routine does not run with the qemu_mutex and therefore cannot make > use of any qemu functions without very special consideration. > > > There shouldn't need to be an explicit init vs. submit function either. Ok, will address these comments. > > Regards, > > Anthony Liguori -- Thanks and Regards gautham