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.
> -/* 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...
> --- 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?
Otherwise it all looks good to me, thanks!
Berto
