From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57351) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QKuaw-00025S-NU for qemu-devel@nongnu.org; Fri, 13 May 2011 11:48:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QKuau-0006p3-65 for qemu-devel@nongnu.org; Fri, 13 May 2011 11:48:02 -0400 Received: from e38.co.us.ibm.com ([32.97.110.159]:47670) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QKuat-0006ob-Qq for qemu-devel@nongnu.org; Fri, 13 May 2011 11:48:00 -0400 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e38.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p4DFdth6004585 for ; Fri, 13 May 2011 09:39:55 -0600 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p4DFmkqs091916 for ; Fri, 13 May 2011 09:48:46 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p4D9khAI023536 for ; Fri, 13 May 2011 03:46:43 -0600 Message-ID: <4DCD527C.6070907@linux.vnet.ibm.com> Date: Fri, 13 May 2011 08:47:08 -0700 From: Venkateswararao Jujjuri MIME-Version: 1.0 References: <1305233867-4367-1-git-send-email-jvrao@linux.vnet.ibm.com> <1305233867-4367-2-git-send-email-jvrao@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/25] [virtio-9p] Add infrastructure to support glib threads and coroutines. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Arun R Bharadwaj , Harsh Prateek Bora , qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com, aliguori@us.ibm.com On 05/12/2011 10:48 PM, Stefan Hajnoczi wrote: > On Thu, May 12, 2011 at 9:57 PM, Venkateswararao Jujjuri (JV) > 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 We will address all your comments. Thanks for the review. - JV > Stefan