[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V3 2/3] qemu: Generic asynchronous threading fra
From: |
Gautham R Shenoy |
Subject: |
Re: [Qemu-devel] [PATCH V3 2/3] qemu: Generic asynchronous threading framework to offload tasks |
Date: |
Thu, 10 Jun 2010 17:07:45 +0530 |
User-agent: |
Mutt/1.5.17 (2007-11-01) |
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<address@hidden>
>> + *
>> + * 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
- Re: [Qemu-devel] Re: [PATCH V3 1/3] qemu: Add qemu-wrappers for pthread_attr_t, (continued)
[Qemu-devel] [PATCH V3 3/3] qemu: Convert AIO code to use the generic threading infrastructure., Gautham R Shenoy, 2010/06/03
[Qemu-devel] [PATCH V3 2/3] qemu: Generic asynchronous threading framework to offload tasks, Gautham R Shenoy, 2010/06/03