diff mbox series

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

Message ID 20230713064111.558652-7-pizhenwei@bytedance.com (mailing list archive)
State New, archived
Headers show
Series Misc fixes for throttle | expand

Commit Message

zhenwei pi July 13, 2023, 6:41 a.m. UTC 
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(-)

Comments

Hanna Czenczek July 21, 2023, 4:03 p.m. UTC | #1 
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
diff mbox series

Patch

diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index 5cfa25c61c..06142eae57 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -242,10 +242,11 @@  static void cryptodev_backend_throttle_timer_cb(void *opaque)
             continue;
         }
 
-        throttle_account(&backend->ts, true, ret);
+        throttle_account(&backend->ts, THROTTLE_WRITE, ret);
         cryptodev_backend_operation(backend, op_info);
         if (throttle_enabled(&backend->tc) &&
-            throttle_schedule_timer(&backend->ts, &backend->tt, true)) {
+            throttle_schedule_timer(&backend->ts, &backend->tt,
+                                    THROTTLE_WRITE)) {
             break;
         }
     }
@@ -261,7 +262,7 @@  int cryptodev_backend_crypto_operation(
         goto do_account;
     }
 
-    if (throttle_schedule_timer(&backend->ts, &backend->tt, true) ||
+    if (throttle_schedule_timer(&backend->ts, &backend->tt, THROTTLE_WRITE) ||
         !QTAILQ_EMPTY(&backend->opinfos)) {
         QTAILQ_INSERT_TAIL(&backend->opinfos, op_info, next);
         return 0;
@@ -273,7 +274,7 @@  do_account:
         return ret;
     }
 
-    throttle_account(&backend->ts, true, ret);
+    throttle_account(&backend->ts, THROTTLE_WRITE, ret);
 
     return cryptodev_backend_operation(backend, op_info);
 }
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;
     bool must_wait;
 
     if (qatomic_read(&tgm->io_limits_disabled)) {
@@ -281,7 +282,7 @@  static bool throttle_group_schedule_timer(ThrottleGroupMember *tgm,
         return true;
     }
 
-    must_wait = throttle_schedule_timer(ts, tt, is_write);
+    must_wait = throttle_schedule_timer(ts, tt, throttle);
 
     /* If a timer just got armed, set tgm as the current token */
     if (must_wait) {
@@ -364,6 +365,7 @@  void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
     bool must_wait;
     ThrottleGroupMember *token;
     ThrottleGroup *tg = container_of(tgm->throttle_state, ThrottleGroup, ts);
+    ThrottleType throttle = is_write ? THROTTLE_WRITE : THROTTLE_READ;
 
     assert(bytes >= 0);
 
@@ -386,7 +388,7 @@  void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm
     }
 
     /* The I/O will be executed, so do the accounting */
-    throttle_account(tgm->throttle_state, is_write, bytes);
+    throttle_account(tgm->throttle_state, throttle, bytes);
 
     /* Schedule the next request */
     schedule_next_request(tgm, is_write);
diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 5c83a1cc09..4aa5bc0196 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -97,16 +97,18 @@  void fsdev_throttle_init(FsThrottle *fst)
 void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write,
                                             struct iovec *iov, int iovcnt)
 {
+    ThrottleType throttle = is_write ? THROTTLE_WRITE : THROTTLE_READ;
+
     if (throttle_enabled(&fst->cfg)) {
-        if (throttle_schedule_timer(&fst->ts, &fst->tt, is_write) ||
+        if (throttle_schedule_timer(&fst->ts, &fst->tt, throttle) ||
             !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {
             qemu_co_queue_wait(&fst->throttled_reqs[is_write], NULL);
         }
 
-        throttle_account(&fst->ts, is_write, iov_size(iov, iovcnt));
+        throttle_account(&fst->ts, throttle, iov_size(iov, iovcnt));
 
         if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write]) &&
-            !throttle_schedule_timer(&fst->ts, &fst->tt, is_write)) {
+            !throttle_schedule_timer(&fst->ts, &fst->tt, throttle)) {
             qemu_co_queue_next(&fst->throttled_reqs[is_write]);
         }
     }
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index ba6293eeef..1cd6b0c397 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -154,9 +154,9 @@  void throttle_config_init(ThrottleConfig *cfg);
 /* usage */
 bool throttle_schedule_timer(ThrottleState *ts,
                              ThrottleTimers *tt,
-                             bool is_write);
+                             ThrottleType throttle);
 
-void throttle_account(ThrottleState *ts, bool is_write, uint64_t size);
+void throttle_account(ThrottleState *ts, ThrottleType throttle, uint64_t size);
 void throttle_limits_to_config(ThrottleLimits *arg, ThrottleConfig *cfg,
                                Error **errp);
 void throttle_config_to_limits(ThrottleConfig *cfg, ThrottleLimits *var);
diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c
index 5547837a58..2c4754fb8a 100644
--- a/tests/unit/test-throttle.c
+++ b/tests/unit/test-throttle.c
@@ -637,9 +637,9 @@  static bool do_test_accounting(bool is_ops, /* are we testing bps or ops */
     throttle_config(&ts, QEMU_CLOCK_VIRTUAL, &cfg);
 
     /* account a read */
-    throttle_account(&ts, false, size);
+    throttle_account(&ts, THROTTLE_READ, size);
     /* account a write */
-    throttle_account(&ts, true, size);
+    throttle_account(&ts, THROTTLE_WRITE, size);
 
     /* check total result */
     index = to_test[is_ops][0];
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
  * @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,
                                    THROTTLE_OPS_TOTAL,
@@ -154,7 +154,7 @@  static int64_t throttle_compute_wait_for(ThrottleState *ts,
     int i;
 
     for (i = 0; i < 4; i++) {
-        BucketType index = to_check[is_write][i];
+        BucketType index = to_check[throttle][i];
         wait = throttle_compute_wait(&ts->cfg.buckets[index]);
         if (wait > max_wait) {
             max_wait = wait;
@@ -166,13 +166,13 @@  static int64_t throttle_compute_wait_for(ThrottleState *ts,
 
 /* compute the timer for this type of operation
  *
- * @is_write:   the type of operation
+ * @throttle:   throttle type
  * @now:        the current clock timestamp
  * @next_timestamp: the resulting timer
  * @ret:        true if a timer must be set
  */
 static bool throttle_compute_timer(ThrottleState *ts,
-                                   bool is_write,
+                                   ThrottleType throttle,
                                    int64_t now,
                                    int64_t *next_timestamp)
 {
@@ -182,7 +182,7 @@  static bool throttle_compute_timer(ThrottleState *ts,
     throttle_do_leak(ts, now);
 
     /* compute the wait time if any */
-    wait = throttle_compute_wait_for(ts, is_write);
+    wait = throttle_compute_wait_for(ts, throttle);
 
     /* if the code must wait compute when the next timer should fire */
     if (wait) {
@@ -423,23 +423,24 @@  void throttle_get_config(ThrottleState *ts, ThrottleConfig *cfg)
  * NOTE: this function is not unit tested due to it's usage of timer_mod
  *
  * @tt:       the timers structure
- * @is_write: the type of operation (read/write)
+ * @throttle: throttle type
  * @ret:      true if the timer has been scheduled else false
  */
 bool throttle_schedule_timer(ThrottleState *ts,
                              ThrottleTimers *tt,
-                             bool is_write)
+                             ThrottleType throttle)
 {
     int64_t now = qemu_clock_get_ns(tt->clock_type);
     int64_t next_timestamp;
     QEMUTimer *timer;
     bool must_wait;
 
-    timer = is_write ? tt->timers[THROTTLE_WRITE] : tt->timers[THROTTLE_READ];
+    assert(throttle < THROTTLE_MAX);
+    timer = tt->timers[throttle];
     assert(timer);
 
     must_wait = throttle_compute_timer(ts,
-                                       is_write,
+                                       throttle,
                                        now,
                                        &next_timestamp);
 
@@ -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 },
@@ -476,6 +477,7 @@  void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)
     double units = 1.0;
     unsigned i;
 
+    assert(throttle < THROTTLE_MAX);
     /* if cfg.op_size is defined and smaller than size we compute unit count */
     if (ts->cfg.op_size && size > ts->cfg.op_size) {
         units = (double) size / ts->cfg.op_size;
@@ -484,13 +486,13 @@  void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)
     for (i = 0; i < 2; i++) {
         LeakyBucket *bkt;
 
-        bkt = &ts->cfg.buckets[bucket_types_size[is_write][i]];
+        bkt = &ts->cfg.buckets[bucket_types_size[throttle][i]];
         bkt->level += size;
         if (bkt->burst_length > 1) {
             bkt->burst_level += size;
         }
 
-        bkt = &ts->cfg.buckets[bucket_types_units[is_write][i]];
+        bkt = &ts->cfg.buckets[bucket_types_units[throttle][i]];
         bkt->level += units;
         if (bkt->burst_length > 1) {
             bkt->burst_level += units;