From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60969) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YStvL-000759-8h for qemu-devel@nongnu.org; Tue, 03 Mar 2015 16:00:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YStvH-0005ax-Rk for qemu-devel@nongnu.org; Tue, 03 Mar 2015 16:00:15 -0500 Received: from mail-wg0-x232.google.com ([2a00:1450:400c:c00::232]:46900) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YStvH-0005ac-IX for qemu-devel@nongnu.org; Tue, 03 Mar 2015 16:00:11 -0500 Received: by wggy19 with SMTP id y19so42493698wgg.13 for ; Tue, 03 Mar 2015 13:00:10 -0800 (PST) Date: Tue, 3 Mar 2015 15:00:06 -0600 From: Stefan Hajnoczi Message-ID: <20150303210006.GG15846@stefanha-thinkpad.redhat.com> References: <142929b6b5adbf371309043d23864c22b0afdec4.1423842044.git.berto@igalia.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="AGZzQgpsuUlWC1xT" Content-Disposition: inline In-Reply-To: <142929b6b5adbf371309043d23864c22b0afdec4.1423842044.git.berto@igalia.com> Subject: Re: [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi --AGZzQgpsuUlWC1xT Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Feb 13, 2015 at 06:06:15PM +0200, Alberto Garcia wrote: > The throttle group support use a cooperative round robin scheduling algor= ithm. >=20 > The principles of the algorithm are simple: > - Each BDS of the group is used as a token in a circular way. > - The active BDS compute if a wait must be done and arm the right timer. > - If a wait must be done the token timer will be armed so the token will = become > the next active BDS. >=20 > Signed-off-by: Alberto Garcia > --- > block.c | 212 +++++++++++++++++++++++++++++++++++++++-= ------ > block/qapi.c | 7 +- > block/throttle-groups.c | 2 +- > blockdev.c | 19 ++++- > hmp.c | 4 +- > include/block/block.h | 3 +- > include/block/block_int.h | 9 +- > qapi/block-core.json | 5 +- > qemu-options.hx | 1 + > qmp-commands.hx | 3 +- > 10 files changed, 220 insertions(+), 45 deletions(-) >=20 > diff --git a/block.c b/block.c > index 642ef04..625f1c8 100644 > --- a/block.c > +++ b/block.c > @@ -36,6 +36,7 @@ > #include "qmp-commands.h" > #include "qemu/timer.h" > #include "qapi-event.h" > +#include "block/throttle-groups.h" > =20 > #ifdef CONFIG_BSD > #include > @@ -130,7 +131,9 @@ void bdrv_set_io_limits(BlockDriverState *bs, > { > int i; > =20 > - throttle_config(&bs->throttle_state, &bs->throttle_timers, cfg); > + throttle_group_lock(bs->throttle_state); > + throttle_config(bs->throttle_state, &bs->throttle_timers, cfg); > + throttle_group_unlock(bs->throttle_state); > =20 > for (i =3D 0; i < 2; i++) { > qemu_co_enter_next(&bs->throttled_reqs[i]); > @@ -157,34 +160,99 @@ static bool bdrv_start_throttled_reqs(BlockDriverSt= ate *bs) > return drained; > } > =20 > +static void bdrv_throttle_group_add(BlockDriverState *bs) > +{ > + int i; > + BlockDriverState *token; > + > + for (i =3D 0; i < 2; i++) { > + /* Get the BlockDriverState having the round robin token */ > + token =3D throttle_group_token(bs->throttle_state, i); > + > + /* If the ThrottleGroup is new set the current BlockDriverState = as > + * token > + */ > + if (!token) { > + throttle_group_set_token(bs->throttle_state, bs, i); > + } > + > + } Does it make sense to move this inside throttle_group_register_bs()? Then bdrv_throttle_group_add() could be dropped and throttle_group_register_bs() is called directly. > + > + throttle_group_register_bs(bs->throttle_state, bs); > +} > + > +static void bdrv_throttle_group_remove(BlockDriverState *bs) The name is a hint that this doesn't belong in block.c. This function should be throttle_group_unregister_bs(). > +{ > + BlockDriverState *token; > + int i; > + > + for (i =3D 0; i < 2; i++) { > + /* Get the BlockDriverState having the round robin token */ > + token =3D throttle_group_token(bs->throttle_state, i); > + /* if this bs is the current token set the next bs as token */ > + if (token =3D=3D bs) { > + token =3D throttle_group_next_bs(token); > + /* take care of the case where bs is the only bs of the grou= p */ > + if (token =3D=3D bs) { > + token =3D NULL; > + } > + throttle_group_set_token(bs->throttle_state, token, i); > + } > + } > + > + /* remove the current bs from the list */ > + QLIST_REMOVE(bs, round_robin); > +} > + > void bdrv_io_limits_disable(BlockDriverState *bs) > { > + > + throttle_group_lock(bs->throttle_state); > bs->io_limits_enabled =3D false; > + throttle_group_unlock(bs->throttle_state); What are the locking rules? I don't understand why throttle_state must be locked to modify bs->io_limits_enabled. > =20 > bdrv_start_throttled_reqs(bs); > =20 > + throttle_group_lock(bs->throttle_state); > + bdrv_throttle_group_remove(bs); > + throttle_group_unlock(bs->throttle_state); > + > + throttle_group_unref(bs->throttle_state); > + bs->throttle_state =3D NULL; > + > throttle_timers_destroy(&bs->throttle_timers); > } > =20 > static void bdrv_throttle_read_timer_cb(void *opaque) > { > BlockDriverState *bs =3D opaque; > - throttle_timer_fired(&bs->throttle_state, false); > + > + throttle_group_lock(bs->throttle_state); > + throttle_timer_fired(bs->throttle_state, false); > + throttle_group_unlock(bs->throttle_state); This pattern suggests throttle_timer_fired() should acquire the lock internally instead. --AGZzQgpsuUlWC1xT Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJU9iDWAAoJEJykq7OBq3PIJ+YH/3lO6LkiObn+3L7maT4MTEru ZWWAxCzIJaJWIYvmFAh+0GkCwD5Oc4mIBOe0Yo7gl5PGIDQWIed+ACNC+2pHEKrq Bw63khktZTXxzgaQJSbp8I0E35p8HdqC8I1fJe1zbc9Od7FaDHhyfKXA8iz1PABR Ze/8S4j2Xa0kYxpGlmfs23CoEKJ7KPjLmEMM2Mwa4r/vJexhegcS0SacKOYV+PxA Rg1BPaPO6Rnu9xeAOctv1f2wdpALlVY8CmJeq3/xo9lL9LRajE3mX4DxCH38DCk1 kD+u593Y8nA0QLLHhtXpm5crRRgbM1K6iGxvNRDDKNlIMcnq19SG9eZDsdpMQz4= =hE26 -----END PGP SIGNATURE----- --AGZzQgpsuUlWC1xT--