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<aneesh.ku...@linux.vnet.ibm.com> >> + * >> + * 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