Re: [Qemu-devel] [PATCH RFC v3 1/8] block: move ThrottleGroup membership to ThrottleGroupMember

[Manos Pitsidianakis <el13635@mail.ntua.gr>]

[-- Attachment #1: Type: text/plain, Size: 3553 bytes --]

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 <el13635@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
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]