Re: [Qemu-devel] [PATCH 01/25] [virtio-9p] Add infrastructure to support glib threads and coroutines.

[Stefan Hajnoczi]

On Fri, May 13, 2011 at 4:47 PM, Venkateswararao Jujjuri
<address@hidden> wrote:
> On 05/12/2011 10:48 PM, Stefan Hajnoczi wrote:
>>
>> On Thu, May 12, 2011 at 9:57 PM, Venkateswararao Jujjuri (JV)
>> <address@hidden>  wrote:
>>>
>>> +/* v9fs glib thread pool */
>>> +V9fsThPool v9fs_pool;
>>
>> This should be static and an init function in this file could perform
>> initialization.  Right now the initialization is inlined in
>> virtio-9p-device.c.
>>
>>> +void v9fs_qemu_submit_request(V9fsRequest *req)
>>> +{
>>> +    V9fsThPool *p =&v9fs_pool;
>>> +
>>> +    req->done = 0;
>>> +    p->requests = g_list_append(p->requests, req);
>>> +    g_thread_pool_push(v9fs_pool.pool, req, NULL);
>>> +}
>>> +
>>> +void v9fs_qemu_process_req_done(void *arg)
>>> +{
>>> +    struct V9fsThPool *p =&v9fs_pool;
>>> +    char byte;
>>> +    ssize_t len;
>>> +    GList *cur_req, *next_req;
>>> +
>>> +    do {
>>> +        len = read(p->rfd,&byte, sizeof(byte));
>>> +    } while (len == -1&&  errno == EINTR);
>>> +
>>> +    for (cur_req = p->requests; cur_req != NULL; cur_req = next_req) {
>>> +        V9fsRequest *req = cur_req->data;
>>> +        next_req = g_list_next(cur_req);
>>> +
>>> +        if (!req->done) {
>>> +            continue;
>>> +        }
>>
>> The requests list is only used for completion, why not enqueue only
>> completed requests and get rid of the done field.  Then this function
>> doesn't have to skip over in-flight requests.
>>
>>> +        Coroutine *entry = req->coroutine;
>>> +        qemu_coroutine_enter(entry, NULL);
>>> +
>>> +        p->requests = g_list_delete_link(p->requests, cur_req);
>>
>> Does this actually free the req?
>>
>>> +static int notifier_fds[2];
>>
>> Why global?
>>
>>> +    if (!g_thread_supported()) {
>>> +        g_thread_init(NULL);
>>> +    }
>>
>> The logic looks inverted.  But if GThread isn't supported where in big
>> trouble so we should probably exit?
>>
> Name of the function is little weird .. it basically checking if the thread
> infrastructure
> is initialized or not. One can call g_thread_init() unconditionally.. but
> this may be good
> practice.
>
> http://developer.gnome.org/glib/2.28/glib-Threads.html#g-thread-supported

Thanks I was just checking it out and noticed they a function with a
clearer name since 2.20:
http://developer.gnome.org/glib/2.28/glib-Threads.html#g-thread-get-initialized

Stefan