From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51640) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKTec-00050Z-5H for qemu-devel@nongnu.org; Tue, 05 Jul 2016 12:57:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKTeZ-0003M4-TY for qemu-devel@nongnu.org; Tue, 05 Jul 2016 12:56:57 -0400 References: <1467127721-9564-1-git-send-email-silbe@linux.vnet.ibm.com> <1467127721-9564-2-git-send-email-silbe@linux.vnet.ibm.com> <818452bb-a196-bac8-87a7-e283451a9494@redhat.com> <87a8hxa319.fsf@oc4731375738.ibm.com> From: Max Reitz Message-ID: Date: Tue, 5 Jul 2016 18:56:49 +0200 MIME-Version: 1.0 In-Reply-To: <87a8hxa319.fsf@oc4731375738.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gDNLSkS6sDM2sAQG0TpNCdpTHiLVUb2M3" Subject: Re: [Qemu-devel] [PATCH 1/1] Improve block job rate limiting for small bandwidth values List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sascha Silbe , qemu-block@nongnu.org Cc: Kevin Wolf , Jeff Cody , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --gDNLSkS6sDM2sAQG0TpNCdpTHiLVUb2M3 From: Max Reitz To: Sascha Silbe , qemu-block@nongnu.org Cc: Kevin Wolf , Jeff Cody , qemu-devel@nongnu.org Message-ID: Subject: Re: [PATCH 1/1] Improve block job rate limiting for small bandwidth values References: <1467127721-9564-1-git-send-email-silbe@linux.vnet.ibm.com> <1467127721-9564-2-git-send-email-silbe@linux.vnet.ibm.com> <818452bb-a196-bac8-87a7-e283451a9494@redhat.com> <87a8hxa319.fsf@oc4731375738.ibm.com> In-Reply-To: <87a8hxa319.fsf@oc4731375738.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04.07.2016 16:30, Sascha Silbe wrote: > Dear Max, >=20 > Max Reitz writes: >=20 >> On 28.06.2016 17:28, Sascha Silbe wrote: > [block/mirror.c] >>> @@ -416,7 +416,9 @@ static uint64_t coroutine_fn mirror_iteration(Mir= rorBlockJob *s) >>> assert(io_sectors); >>> sector_num +=3D io_sectors; >>> nb_chunks -=3D DIV_ROUND_UP(io_sectors, sectors_per_chunk); >>> - delay_ns +=3D ratelimit_calculate_delay(&s->limit, io_sector= s); >>> + if (s->common.speed) { >>> + delay_ns =3D ratelimit_calculate_delay(&s->limit, io_sec= tors); >>> + } >> >> Hmm... Was it a bug that ratelimit_calculate_delay() was called >> unconditionally before? >=20 > One could argue either way. It happened to work because > ratelimit_calculate_delay() only delayed the _second_ time (within one > time slice) the quota was exceeded. With zero duration time slices, > there never was a second time. >=20 > With the new implementation we would divide by zero when slice_quota is= > 0, so we need to guard against that. Most callers already did, only > mirror_iteration() needed to be adjusted. >=20 >=20 > [...] > [include/qemu/ratelimit.h] >>> static inline int64_t ratelimit_calculate_delay(RateLimit *limit, ui= nt64_t n) >>> { >>> int64_t now =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); >>> + uint64_t delay_slices; >>> =20 >>> - if (limit->next_slice_time < now) { >>> - limit->next_slice_time =3D now + limit->slice_ns; >>> + assert(limit->slice_quota && limit->slice_ns); >>> + >>> + if (limit->slice_end_time < now) { >>> + /* Previous, possibly extended, time slice finished; reset t= he >>> + * accounting. */ >>> + limit->slice_start_time =3D now; >>> + limit->slice_end_time =3D now + limit->slice_ns; >>> limit->dispatched =3D 0; >>> } >>> - if (limit->dispatched =3D=3D 0 || limit->dispatched + n <=3D lim= it->slice_quota) { >>> - limit->dispatched +=3D n; >>> + >>> + limit->dispatched +=3D n; >>> + if (limit->dispatched < limit->slice_quota) { >> >> Nitpick: This should probably stay <=3D. >=20 > This is a subtle edge case. Previously, when limit->dispatched =3D=3D > limit->slice_quota, we returned 0 so that the _current_ request (which > is still within quota) wouldn't be delayed. Now, we return a delay so > that the _next_ request (which would be over quota) will be delayed. Hm, but that depends on the size of the next request. Of course, if we get limit->dispatched =3D=3D limit->slice_quota we know for sure that we need to delay the next request. But if we get limit->dispatched =3D=3D limit->slice_quota - 1... Then we probably also have to delay it, but we don't know for sure. So I think it would be better to have small but consistent systematic error here, i.e. that we will not delay the last request even though we should. Or you could insert a delay after the last request in all block jobs, too. Or did I fail to understand the issue? I'm not sure. > [...] >>> + /* Quota exceeded. Calculate the next time slice we may start >>> + * sending data again. */ >>> + delay_slices =3D (limit->dispatched + limit->slice_quota - 1) / >>> + limit->slice_quota; >>> + limit->slice_end_time =3D limit->slice_start_time + >>> + delay_slices * limit->slice_ns; >> >> I think it would make sense to make this a floating point calculation.= >=20 > Then we'd have fully variable length time slices, instead of just > "occupying" multiple fixed-length time slices with a single > request. Maybe that would be even better, or maybe we'd cause other > interesting things to happen (think interactions with the scheduler). :-) > A= s > this code will hopefully disappear during the 2.8 time line anyway, I'd= > prefer to go with the lowest risk option that is enough to fix the race= > conditions encountered by the test suite. OK with me. Max >> If you don't agree, though: >> >> Reviewed-by: Max Reitz >=20 > Thanks for the review! >=20 > Sascha >=20 --gDNLSkS6sDM2sAQG0TpNCdpTHiLVUb2M3 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEvBAEBCAAZBQJXe+bREhxtcmVpdHpAcmVkaGF0LmNvbQAKCRA7sUIC6DisrXwt B/0fz4BDJb4Q1ih8NOAvNzOY6lmYUGmG36bqMkEW268J1MSBpVXgAxlILqmOWRq7 IUmj1J3zEUUnLPelUageKcFyiDy9HQRaMwf8/AVJKEmT0NN8+iZhUe5jVNBf4Ws+ TDz+CMVHtwYhmW1F69GajR3l1JUdaPZrW1DaID5BefuDbj9w8VoAg73ReWgHi+Ui 7dy3driDt6p1+dTsXy/jJeVCxKTx7AKx3PWGumXEPwz1pXmp5bm3mR3PzOC0Iyrs qRmxN/psN3vZWF2/SrnmUDTPbxwG5nMYDVXVNBwjwOkvualoWy3KXBQZq6IA3jrb IfEOF3LtkTXoZIE/22Son6au =SRxh -----END PGP SIGNATURE----- --gDNLSkS6sDM2sAQG0TpNCdpTHiLVUb2M3--