On Tue, Jun 27, 2017 at 02:08:54PM +0200, Alberto Garcia wrote:
On Fri 23 Jun 2017 02:46:53 PM CEST, Manos Pitsidianakis wrote:This commit gathers ThrottleGroup membership details from BlockBackendPublic into ThrottleGroupMember and refactors existing code to use the structure.Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr>Hey Manos, thanks for the patch. It looks good to me in general, I only have a couple of comments:/* If no IO are queued for scheduling on the next round robin token - * then decide the token is the current bs because chances are - * the current bs get the current request queued. + * then decide the token is the current tgm because chances are + * the current tgm get the current request queued.I wonder if it's not better to use 'member' instead of 'tgm' in general. My impression is that the former is clearer and not too long, but I don't have a very strong opinion so you can keep it like this if you want.
I will change it, no problem,
-/* Check if an I/O request needs to be throttled, wait and set a timer - * if necessary, and schedule the next request using a round robin - * algorithm. +/* Check if an I/O request needs to be throttled, wait and set a timer if + * necessary, and schedule the next request using a round robin algorithm. *There's a few cases like this when you reformat a paragraph but don't actually change the text. I think it just adds unnecessary noise to the diff...
Wiki says "It's OK to fix coding style issues in the immediate area (few lines) of the lines you're changing" so I left the reformats. Since they are noticed they must be too noisey.. I will either remove the changes or do a restyling patch later.
--- a/include/qemu/throttle.h +++ b/include/qemu/throttle.h @@ -27,6 +27,7 @@ #include "qemu-common.h" #include "qemu/timer.h" +#include "qemu/coroutine.h" #define THROTTLE_VALUE_MAX 1000000000000000LL @@ -153,4 +154,29 @@ bool throttle_schedule_timer(ThrottleState *ts, void throttle_account(ThrottleState *ts, bool is_write, uint64_t size); + +/* The ThrottleGroupMember structure indicates membership in a ThrottleGroup + * and holds related data. + */ + +typedef struct ThrottleGroupMember { + /* throttled_reqs_lock protects the CoQueues for throttled requests. */ + CoMutex throttled_reqs_lock; + CoQueue throttled_reqs[2]; + + /* Nonzero if the I/O limits are currently being ignored; generally + * it is zero. Accessed with atomic operations. + */ + unsigned int io_limits_disabled; + + /* The following fields are protected by the ThrottleGroup lock. + * See the ThrottleGroup documentation for details. + * throttle_state tells us if I/O limits are configured. */ + ThrottleState *throttle_state; + ThrottleTimers throttle_timers; + unsigned pending_reqs[2]; + QLIST_ENTRY(ThrottleGroupMember) round_robin; + +} ThrottleGroupMember; +Any reason why you add this to throttle.h (which is generic throttling code independent from the block layer) and not to throttle-groups.h?
I put it there because it's not directly ThrottleGroup-related, but you're right, if throttle.h is block layer free it should remain that way.
Otherwise it all looks good to me, thanks! Berto
signature.asc
Description: PGP signature
