Re: [Qemu-devel] [PATCH V3 2/3] qemu: Generic asynchronous threading framework to offload tasks

[Gautham R Shenoy <ego@in.ibm.com>]

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.kumar@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