Re: [PATCH v3 6/6] throttle: use enum ThrottleType instead of bool is_write

[Hanna Czenczek]

On 13.07.23 08:41, zhenwei pi wrote:
> enum ThrottleType is already there, use ThrottleType instead of
> 'bool is_write' for throttle API, also modify related codes from
> block, fsdev, cryptodev and tests.
>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>   backends/cryptodev.c        |  9 +++++----
>   block/throttle-groups.c     |  6 ++++--
>   fsdev/qemu-fsdev-throttle.c |  8 +++++---
>   include/qemu/throttle.h     |  4 ++--
>   tests/unit/test-throttle.c  |  4 ++--
>   util/throttle.c             | 30 ++++++++++++++++--------------
>   6 files changed, 34 insertions(+), 27 deletions(-)

Something not addressed in this patch that I would like to see: 
schedule_next_request() in block/throttle-groups.c runs 
`timer_mod(tt->timers[is_write], now)`.  I think that at least should be 
`tt->timers[is_write ? THROTTLE_WRITE : THROTTLE_READ]`.  Even better 
would be to have it take a `ThrottleType` instead of `bool is_write`, 
too, and use this enum throughout throttle-groups.c, too (i.e. for 
ThrottleGroupMember.throttled_reqs[], 
ThrottleGroupMember.pending_reqs[], ThrottleGroup.tokens[], and 
ThrottleGroup.any_timer_armed[]).  Then throttle_group_schedule_timer() 
could also take a `ThrottleType`.

But I understand asking for throttle-groups.c to be ThrottleType-ified 
is very much, so this is just a suggestion.  But I do ask for that one 
`timer_mod()` call to use THROTTLE_READ and THROTTLE_WRITE to index 
`tt->timers[]` instead of `is_write` directly.

[...]

> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index fb203c3ced..429b9d1dae 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -270,6 +270,7 @@ static bool 
> throttle_group_schedule_timer(ThrottleGroupMember *tgm,
>       ThrottleState *ts = tgm->throttle_state;
>       ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
>       ThrottleTimers *tt = &tgm->throttle_timers;
> +    ThrottleType throttle = is_write ? THROTTLE_WRITE : THROTTLE_READ;

Again, another stylistic suggestion (for all similar places in this 
patch): It isn’t clear what just `throttle` means. `throttle_direction` 
for example would be clear, or maybe just `direction`, this is throttle 
code, so it’s clear that everything that isn’t given a context is about 
throttling (it wasn’t `is_throttled_write` either, after all, but just 
`is_write`).

>       bool must_wait;
>   
>       if (qatomic_read(&tgm->io_limits_disabled)) {

[...]

> diff --git a/util/throttle.c b/util/throttle.c
> index c0bd0c26c3..5e4dc0bfdd 100644
> --- a/util/throttle.c
> +++ b/util/throttle.c
> @@ -136,11 +136,11 @@ int64_t throttle_compute_wait(LeakyBucket *bkt)
>   
>   /* This function compute the time that must be waited while this IO
>    *
> - * @is_write:   true if the current IO is a write, false if it's a read
> + * @throttle:   throttle type

I’m not too happy about “throttle type” as a description here, because 
“type” can be anything (naïvely, I’d rather interpret it to mean the 
algorithm used for throttling, or whether we’re throttling on bytes or 
IOPS).  “throttle direction” would be better.  This also applies to 
other functions’ interface documentation.

(Yes, this also means that I don’t like the type name ThrottleType very 
much, and would prefer it to be ThrottleDirection, but that would be an 
invasive change all over this series (when Berto has already reviewed 
it), so I’m not really asking for a change there.)

>    * @ret:        time to wait
>    */
>   static int64_t throttle_compute_wait_for(ThrottleState *ts,
> -                                         bool is_write)
> +                                         ThrottleType throttle)
>   {
>       BucketType to_check[2][4] = { {THROTTLE_BPS_TOTAL,

Since we’re now using a ThrottleType to index the first dimension of 
this array, I’d prefer to make this to_check[THROTTLE_MAX][4].

(Also, but very much unrelated to this patch: Why isn’t this lookup 
table a `const static`?)

>                                      THROTTLE_OPS_TOTAL,

[...]

> @@ -460,10 +461,10 @@ bool throttle_schedule_timer(ThrottleState *ts,
>   
>   /* do the accounting for this operation
>    *
> - * @is_write: the type of operation (read/write)
> + * @throttle: throttle type
>    * @size:     the size of the operation
>    */
> -void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)
> +void throttle_account(ThrottleState *ts, ThrottleType throttle, uint64_t size)
>   {
>       const BucketType bucket_types_size[2][2] = {
>           { THROTTLE_BPS_TOTAL, THROTTLE_BPS_READ },

Like in throttle_compute_wait_for(), I’d prefer bucket_types_size and 
bucket_types_units to have [THROTTLE_MAX] in the first dimension.

(Interesting that these lookup tables are const, but not static.  I 
think they should be static, too.)

Hanna