From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42601) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QKucS-0002mj-44 for qemu-devel@nongnu.org; Fri, 13 May 2011 11:49:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QKucQ-00075f-UK for qemu-devel@nongnu.org; Fri, 13 May 2011 11:49:36 -0400 Received: from mail-gx0-f173.google.com ([209.85.161.173]:48581) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QKucQ-00075Y-Qf for qemu-devel@nongnu.org; Fri, 13 May 2011 11:49:34 -0400 Received: by gxk26 with SMTP id 26so1092221gxk.4 for ; Fri, 13 May 2011 08:49:33 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4DCD527C.6070907@linux.vnet.ibm.com> References: <1305233867-4367-1-git-send-email-jvrao@linux.vnet.ibm.com> <1305233867-4367-2-git-send-email-jvrao@linux.vnet.ibm.com> <4DCD527C.6070907@linux.vnet.ibm.com> Date: Fri, 13 May 2011 16:49:32 +0100 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: Venkateswararao Jujjuri Cc: Arun R Bharadwaj , Harsh Prateek Bora , qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com, aliguori@us.ibm.com On Fri, May 13, 2011 at 4:47 PM, Venkateswararao Jujjuri wrote: > On 05/12/2011 10:48 PM, Stefan Hajnoczi wrote: >> >> On Thu, May 12, 2011 at 9:57 PM, Venkateswararao Jujjuri (JV) >> =A0wrote: >>> >>> +/* v9fs glib thread pool */ >>> +V9fsThPool v9fs_pool; >> >> This should be static and an init function in this file could perform >> initialization. =A0Right now the initialization is inlined in >> virtio-9p-device.c. >> >>> +void v9fs_qemu_submit_request(V9fsRequest *req) >>> +{ >>> + =A0 =A0V9fsThPool *p =3D&v9fs_pool; >>> + >>> + =A0 =A0req->done =3D 0; >>> + =A0 =A0p->requests =3D g_list_append(p->requests, req); >>> + =A0 =A0g_thread_pool_push(v9fs_pool.pool, req, NULL); >>> +} >>> + >>> +void v9fs_qemu_process_req_done(void *arg) >>> +{ >>> + =A0 =A0struct V9fsThPool *p =3D&v9fs_pool; >>> + =A0 =A0char byte; >>> + =A0 =A0ssize_t len; >>> + =A0 =A0GList *cur_req, *next_req; >>> + >>> + =A0 =A0do { >>> + =A0 =A0 =A0 =A0len =3D read(p->rfd,&byte, sizeof(byte)); >>> + =A0 =A0} while (len =3D=3D -1&& =A0errno =3D=3D EINTR); >>> + >>> + =A0 =A0for (cur_req =3D p->requests; cur_req !=3D NULL; cur_req =3D n= ext_req) { >>> + =A0 =A0 =A0 =A0V9fsRequest *req =3D cur_req->data; >>> + =A0 =A0 =A0 =A0next_req =3D g_list_next(cur_req); >>> + >>> + =A0 =A0 =A0 =A0if (!req->done) { >>> + =A0 =A0 =A0 =A0 =A0 =A0continue; >>> + =A0 =A0 =A0 =A0} >> >> The requests list is only used for completion, why not enqueue only >> completed requests and get rid of the done field. =A0Then this function >> doesn't have to skip over in-flight requests. >> >>> + =A0 =A0 =A0 =A0Coroutine *entry =3D req->coroutine; >>> + =A0 =A0 =A0 =A0qemu_coroutine_enter(entry, NULL); >>> + >>> + =A0 =A0 =A0 =A0p->requests =3D g_list_delete_link(p->requests, cur_re= q); >> >> Does this actually free the req? >> >>> +static int notifier_fds[2]; >> >> Why global? >> >>> + =A0 =A0if (!g_thread_supported()) { >>> + =A0 =A0 =A0 =A0g_thread_init(NULL); >>> + =A0 =A0} >> >> The logic looks inverted. =A0But 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 thre= ad > 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-initial= ized Stefan