From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33291) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaRJ4-000220-W0 for qemu-devel@nongnu.org; Tue, 24 Mar 2015 12:04:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaRJ0-0005b5-Tg for qemu-devel@nongnu.org; Tue, 24 Mar 2015 12:03:54 -0400 Received: from mail-wg0-x229.google.com ([2a00:1450:400c:c00::229]:35061) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaRIy-0005aK-Nn for qemu-devel@nongnu.org; Tue, 24 Mar 2015 12:03:50 -0400 Received: by wgdm6 with SMTP id m6so174922449wgd.2 for ; Tue, 24 Mar 2015 09:03:47 -0700 (PDT) Date: Tue, 24 Mar 2015 16:03:44 +0000 From: Stefan Hajnoczi Message-ID: <20150324160344.GG1493@stefanha-thinkpad.redhat.com> References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="IuhbYIxU28t+Kd57" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 4/6] 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 --IuhbYIxU28t+Kd57 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 10, 2015 at 05:30:48PM +0200, Alberto Garcia wrote: > @@ -179,10 +179,11 @@ static void bdrv_throttle_write_timer_cb(void *opaq= ue) > } > =20 > /* should be called before bdrv_set_io_limits if a limit is set */ > -void bdrv_io_limits_enable(BlockDriverState *bs) > +void bdrv_io_limits_enable(BlockDriverState *bs, const char *group) > { > assert(!bs->io_limits_enabled); > - throttle_init(&bs->throttle_state); > + > + throttle_group_register_bs(bs, group ? group : bdrv_get_device_name(= bs)); Please don't allow a NULL group argument. blockdev_init() should pick the group name instead of requiring bdrv_io_limits_enable() to generate a unique name. This way we avoid silently adding all BDS without a BlockBackend to a "" throttling group. I think that's a bug waiting to happen :). > @@ -201,29 +219,7 @@ static void bdrv_io_limits_intercept(BlockDriverStat= e *bs, > unsigned int bytes, > bool is_write) > { > - /* does this io must wait */ > - bool must_wait =3D throttle_schedule_timer(&bs->throttle_state, > - &bs->throttle_timers, > - is_write); > - > - /* if must wait or any request of this type throttled queue the IO */ > - if (must_wait || > - !qemu_co_queue_empty(&bs->throttled_reqs[is_write])) { > - qemu_co_queue_wait(&bs->throttled_reqs[is_write]); > - } > - > - /* the IO will be executed, do the accounting */ > - throttle_account(&bs->throttle_state, is_write, bytes); > - > - > - /* if the next request must wait -> do nothing */ > - if (throttle_schedule_timer(&bs->throttle_state, &bs->throttle_timer= s, > - is_write)) { > - return; > - } > - > - /* else queue next request for execution */ > - qemu_co_queue_next(&bs->throttled_reqs[is_write]); > + throttle_group_io_limits_intercept(bs, bytes, is_write); > } bdrv_io_limits_intercept() is no longer useful. Can you replace bdrv_io_limits_intercept() calls with throttle_group_io_limits_intercept() to eliminate this indirection? > =20 > size_t bdrv_opt_mem_align(BlockDriverState *bs) > @@ -2050,15 +2046,16 @@ static void bdrv_move_feature_fields(BlockDriverS= tate *bs_dest, > bs_dest->enable_write_cache =3D bs_src->enable_write_cache; > =20 > /* i/o throttled req */ > - memcpy(&bs_dest->throttle_state, > - &bs_src->throttle_state, > - sizeof(ThrottleState)); > + bs_dest->throttle_state =3D bs_src->throttle_state, > + bs_dest->io_limits_enabled =3D bs_src->io_limits_enabled; > + bs_dest->throttled_reqs[0] =3D bs_src->throttled_reqs[0]; > + bs_dest->throttled_reqs[1] =3D bs_src->throttled_reqs[1]; > + memcpy(&bs_dest->round_robin, > + &bs_src->round_robin, > + sizeof(bs_dest->round_robin)); The bdrv_swap() code is tricky to think about...I think this is okay because bs_dest and bs_src linked list pointers will be unchanged (although they are now pointing to a different block driver instance). Just highlighting this in case anyone else spots a problem with the pointer swapping. > @@ -145,6 +147,131 @@ static BlockDriverState *throttle_group_next_bs(Blo= ckDriverState *bs) > return next; > } > =20 > +/* Return the next BlockDriverState in the round-robin sequence with > + * pending I/O requests. > + * > + * @bs: the current BlockDriverState > + * @is_write: the type of operation (read/write) > + * @ret: the next BlockDriverState with pending requests, or bs > + * if there is none. Assumes tg->lock is held. It's helpful to document this. --IuhbYIxU28t+Kd57 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVEYrgAAoJEJykq7OBq3PIh3UH/3RxlmPZntd8yw/DwU5+Zkqx 6Yri/T/fkzwD3eaWVnV4HRl/69A7qlSJz6mbd+/NF1SPmXiXO6VlKlj6uFeb5/BZ pJCK532T6REq5V9T3KY8qfdUiLLEmvZzUL02pTZTkxI5A9W6OTieRNrAc0yttp22 XKei8dkO0H7/TlWRwWsgup/74RBWmMMbMmJgGw7m4LvwG/VHlvBvTq1oSkOmhHm7 P1F6L8Uxq8EfMPjQanqY7tPqYfEdmCE+5yC8/NaUUBPfCVyqJNfkPbc2ZUl2M0rx ZWBYOFCbi07DgvFNT0t5t36jp2RU4spB64eWnTQkrmVj9PsX4sly0NZ/eFtrY6E= =rb3o -----END PGP SIGNATURE----- --IuhbYIxU28t+Kd57--