From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:37639)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1QKlEe-00008z-6x
	for qemu-devel@nongnu.org; Fri, 13 May 2011 01:48:25 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1QKlEd-0000mk-6o
	for qemu-devel@nongnu.org; Fri, 13 May 2011 01:48:24 -0400
Received: from mail-gw0-f45.google.com ([74.125.83.45]:52291)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1QKlEd-0000md-35
	for qemu-devel@nongnu.org; Fri, 13 May 2011 01:48:23 -0400
Received: by gwb19 with SMTP id 19so924244gwb.4
	for <qemu-devel@nongnu.org>; Thu, 12 May 2011 22:48:22 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <1305233867-4367-2-git-send-email-jvrao@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>
Date: Fri, 13 May 2011 06:48:21 +0100
Message-ID: <BANLkTik8y5S9hBor2HqcfAUG+q=Hmf=LTQ@mail.gmail.com>
From: Stefan Hajnoczi <stefanha@gmail.com>
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: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: "Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com>
Cc: Arun R Bharadwaj <arun@linux.vnet.ibm.com>, Harsh Prateek Bora <harsh@linux.vnet.ibm.com>, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com, aliguori@us.ibm.com

On Thu, May 12, 2011 at 9:57 PM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> 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)
> +{
> + =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 && errno =3D=3D EINTR);
> +
> + =A0 =A0for (cur_req =3D p->requests; cur_req !=3D NULL; cur_req =3D nex=
t_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.  Then 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_req)=
;

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.  But if GThread isn't supported where in big
trouble so we should probably exit?

Stefan