From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:58374)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1WkqY2-0000rk-1W
	for qemu-devel@nongnu.org; Thu, 15 May 2014 03:57:56 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1WkqXv-0006Lg-T6
	for qemu-devel@nongnu.org; Thu, 15 May 2014 03:57:49 -0400
Received: from mx1.redhat.com ([209.132.183.28]:26505)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1WkqXv-0006LV-IE
	for qemu-devel@nongnu.org; Thu, 15 May 2014 03:57:43 -0400
Date: Thu, 15 May 2014 09:57:37 +0200
From: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20140515075737.GC1111@stefanha-thinkpad.redhat.com>
References: <1400077367-23409-1-git-send-email-stefanha@redhat.com>
	<1400077367-23409-2-git-send-email-stefanha@redhat.com>
	<20140514150558.GA5955@irqsave.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
In-Reply-To: <20140514150558.GA5955@irqsave.net>
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH 1/3] throttle: add
	throttle_detach/attach_aio_context()
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: =?iso-8859-1?Q?Beno=EEt?= Canet <benoit.canet@irqsave.net>
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org

On Wed, May 14, 2014 at 05:05:58PM +0200, Beno=EEt Canet wrote:
> The Wednesday 14 May 2014 =E0 16:22:45 (+0200), Stefan Hajnoczi wrote :
> > Block I/O throttling uses timers and currently always adds them to th=
e
> > main loop.  Throttling will break if bdrv_set_aio_context() is used t=
o
> > move a BlockDriverState to a different AioContext.
> >=20
> > This patch adds throttle_detach/attach_aio_context() interfaces so th=
e
> s/so the/to the/ ?=20

Thanks, let me know how you feel about my replies below.  If there is no
other need to respin then let's let this commit description typo slide.

> > diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> > index ab29b0b..b890613 100644
> > --- a/include/qemu/throttle.h
> > +++ b/include/qemu/throttle.h
> > @@ -67,6 +67,11 @@ typedef struct ThrottleState {
>=20
> Can we make sure this header knows about AioContext in case
> there is another throttle user not block related ?

It already does because qemu-common.h includes qemu-typedefs.h.  We get
an opaque AioContext.

The point of the typedefs.h is to avoid pulling in all the headers when
callers just pass around pointers and don't need the actual definition.

In this case, I think the split makes sense because callers might pass
AioContext without actually using the AioContext API themselves.

> > diff --git a/tests/test-throttle.c b/tests/test-throttle.c
> > index 1d4ffd3..5fa5000 100644
> > --- a/tests/test-throttle.c
> > +++ b/tests/test-throttle.c
> > @@ -12,8 +12,10 @@
> > =20
> >  #include <glib.h>
> >  #include <math.h>
>=20
> > +#include "block/aio.h"
>=20
> And remove this one eventually.

This include pulls in the AioContext API because the test case itself
manipulates the AioContext.