[PATCH v3 0/6] Misc fixes for throttle

[PATCH v3 0/6] Misc fixes for throttle

[zhenwei pi]

v2 -> v3:
- patch 1 -> patch 5 are already reviewed by Alberto
- append patch 6: throttle: use enum ThrottleType instead of bool is_write

v1 -> v2:
- rename 'ThrottleTimerType' to 'ThrottleType'
- add assertion to throttle_schedule_timer

v1:
- introduce enum ThrottleTimerType instead of timers[0], timer[1]...
- support read-only and write-only for throttle
- adapt related test codes
- cryptodev uses a write-only throttle timer

Zhenwei Pi (6):
  throttle: introduce enum ThrottleType
  test-throttle: use enum ThrottleType
  throttle: support read-only and write-only
  test-throttle: test read only and write only
  cryptodev: use NULL throttle timer cb for read direction
  throttle: use enum ThrottleType instead of bool is_write

 backends/cryptodev.c        | 12 +++---
 block/throttle-groups.c     |  6 ++-
 fsdev/qemu-fsdev-throttle.c |  8 ++--
 include/qemu/throttle.h     | 15 +++++---
 tests/unit/test-throttle.c  | 76 ++++++++++++++++++++++++++++++++++---
 util/throttle.c             | 64 +++++++++++++++++++------------
 6 files changed, 136 insertions(+), 45 deletions(-)

-- 
2.34.1

[PATCH v3 1/6] throttle: introduce enum ThrottleType

[zhenwei pi]

Use enum ThrottleType instead of number index.

Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 include/qemu/throttle.h | 11 ++++++++---
 util/throttle.c         | 16 +++++++++-------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 05f6346137..ba6293eeef 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -99,13 +99,18 @@ typedef struct ThrottleState {
     int64_t previous_leak;    /* timestamp of the last leak done */
 } ThrottleState;
 
+typedef enum {
+    THROTTLE_READ = 0,
+    THROTTLE_WRITE,
+    THROTTLE_MAX
+} ThrottleType;
+
 typedef struct ThrottleTimers {
-    QEMUTimer *timers[2];     /* timers used to do the throttling */
+    QEMUTimer *timers[THROTTLE_MAX];    /* timers used to do the throttling */
     QEMUClockType clock_type; /* the clock used */
 
     /* Callbacks */
-    QEMUTimerCB *read_timer_cb;
-    QEMUTimerCB *write_timer_cb;
+    QEMUTimerCB *timer_cb[THROTTLE_MAX];
     void *timer_opaque;
 } ThrottleTimers;
 
diff --git a/util/throttle.c b/util/throttle.c
index 81f247a8d1..5642e61763 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -199,10 +199,12 @@ static bool throttle_compute_timer(ThrottleState *ts,
 void throttle_timers_attach_aio_context(ThrottleTimers *tt,
                                         AioContext *new_context)
 {
-    tt->timers[0] = aio_timer_new(new_context, tt->clock_type, SCALE_NS,
-                                  tt->read_timer_cb, tt->timer_opaque);
-    tt->timers[1] = aio_timer_new(new_context, tt->clock_type, SCALE_NS,
-                                  tt->write_timer_cb, tt->timer_opaque);
+    tt->timers[THROTTLE_READ] =
+        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+                      tt->timer_cb[THROTTLE_READ], tt->timer_opaque);
+    tt->timers[THROTTLE_WRITE] =
+        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+                      tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);
 }
 
 /*
@@ -236,8 +238,8 @@ void throttle_timers_init(ThrottleTimers *tt,
     memset(tt, 0, sizeof(ThrottleTimers));
 
     tt->clock_type = clock_type;
-    tt->read_timer_cb = read_timer_cb;
-    tt->write_timer_cb = write_timer_cb;
+    tt->timer_cb[THROTTLE_READ] = read_timer_cb;
+    tt->timer_cb[THROTTLE_WRITE] = write_timer_cb;
     tt->timer_opaque = timer_opaque;
     throttle_timers_attach_aio_context(tt, aio_context);
 }
@@ -256,7 +258,7 @@ void throttle_timers_detach_aio_context(ThrottleTimers *tt)
 {
     int i;
 
-    for (i = 0; i < 2; i++) {
+    for (i = 0; i < THROTTLE_MAX; i++) {
         throttle_timer_destroy(&tt->timers[i]);
     }
 }
-- 
2.34.1

Re: [PATCH v3 1/6] throttle: introduce enum ThrottleType

[Hanna Czenczek]

On 13.07.23 08:41, zhenwei pi wrote:
> Use enum ThrottleType instead of number index.
>
> Reviewed-by: Alberto Garcia<berto@igalia.com>
> Signed-off-by: zhenwei pi<pizhenwei@bytedance.com>
> ---
>   include/qemu/throttle.h | 11 ++++++++---
>   util/throttle.c         | 16 +++++++++-------
>   2 files changed, 17 insertions(+), 10 deletions(-)

[...]

> diff --git a/util/throttle.c b/util/throttle.c
> index 81f247a8d1..5642e61763 100644
> --- a/util/throttle.c
> +++ b/util/throttle.c
> @@ -199,10 +199,12 @@ static bool throttle_compute_timer(ThrottleState *ts,
>   void throttle_timers_attach_aio_context(ThrottleTimers *tt,
>                                           AioContext *new_context)
>   {
> -    tt->timers[0] = aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> -                                  tt->read_timer_cb, tt->timer_opaque);
> -    tt->timers[1] = aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> -                                  tt->write_timer_cb, tt->timer_opaque);
> +    tt->timers[THROTTLE_READ] =
> +        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> +                      tt->timer_cb[THROTTLE_READ], tt->timer_opaque);
> +    tt->timers[THROTTLE_WRITE] =
> +        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> +                      tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);

This could benefit from the new structure:

```
for (int i = 0; i < THROTTLE_MAX; i++) {
     tt->timers[i] = aio_timer_new(..., tt->timer_cb[i], ...);
}
```

Even more so after patch 3.  Still, optional, of course, so either way:

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Of note is that we still have instances in util/throttle.c and 
block/throttle-groups.c that don’t use these enums to access the 
tt->timers[] array, and that’s a bit unfortunate now (i.e. 
`tt->timers[is_write]` should rather be `tt->timers[is_write ? 
THROTTLE_WRITE : THROTTLE_READ]` instead).  But functionally it’s OK and 
I believe patches 3 and 6 do address this.

[PATCH v3 2/6] test-throttle: use enum ThrottleType

[zhenwei pi]

Use enum ThrottleType instead in the throttle test codes.

Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 tests/unit/test-throttle.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c
index 7adb5e6652..a60b5fe22e 100644
--- a/tests/unit/test-throttle.c
+++ b/tests/unit/test-throttle.c
@@ -169,8 +169,8 @@ static void test_init(void)
 
     /* check initialized fields */
     g_assert(tt->clock_type == QEMU_CLOCK_VIRTUAL);
-    g_assert(tt->timers[0]);
-    g_assert(tt->timers[1]);
+    g_assert(tt->timers[THROTTLE_READ]);
+    g_assert(tt->timers[THROTTLE_WRITE]);
 
     /* check other fields where cleared */
     g_assert(!ts.previous_leak);
@@ -191,7 +191,7 @@ static void test_destroy(void)
     throttle_timers_init(tt, ctx, QEMU_CLOCK_VIRTUAL,
                          read_timer_cb, write_timer_cb, &ts);
     throttle_timers_destroy(tt);
-    for (i = 0; i < 2; i++) {
+    for (i = 0; i < THROTTLE_MAX; i++) {
         g_assert(!tt->timers[i]);
     }
 }
-- 
2.34.1

Re: [PATCH v3 2/6] test-throttle: use enum ThrottleType

[Hanna Czenczek]

On 13.07.23 08:41, zhenwei pi wrote:
> Use enum ThrottleType instead in the throttle test codes.
>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>   tests/unit/test-throttle.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

[PATCH v3 3/6] throttle: support read-only and write-only

[zhenwei pi]

Only one direction is necessary in several scenarios:
- a read-only disk
- operations on a device are considered as *write* only. For example,
  encrypt/decrypt/sign/verify operations on a cryptodev use a single
  *write* timer(read timer callback is defined, but never invoked).

Allow a single direction in throttle, this reduces memory, and uplayer
does not need a dummy callback any more.

Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 util/throttle.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/util/throttle.c b/util/throttle.c
index 5642e61763..c0bd0c26c3 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -199,12 +199,17 @@ static bool throttle_compute_timer(ThrottleState *ts,
 void throttle_timers_attach_aio_context(ThrottleTimers *tt,
                                         AioContext *new_context)
 {
-    tt->timers[THROTTLE_READ] =
-        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
-                      tt->timer_cb[THROTTLE_READ], tt->timer_opaque);
-    tt->timers[THROTTLE_WRITE] =
-        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
-                      tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);
+    if (tt->timer_cb[THROTTLE_READ]) {
+        tt->timers[THROTTLE_READ] =
+            aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+                          tt->timer_cb[THROTTLE_READ], tt->timer_opaque);
+    }
+
+    if (tt->timer_cb[THROTTLE_WRITE]) {
+        tt->timers[THROTTLE_WRITE] =
+            aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+                          tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);
+    }
 }
 
 /*
@@ -235,6 +240,7 @@ void throttle_timers_init(ThrottleTimers *tt,
                           QEMUTimerCB *write_timer_cb,
                           void *timer_opaque)
 {
+    assert(read_timer_cb || write_timer_cb);
     memset(tt, 0, sizeof(ThrottleTimers));
 
     tt->clock_type = clock_type;
@@ -247,7 +253,9 @@ void throttle_timers_init(ThrottleTimers *tt,
 /* destroy a timer */
 static void throttle_timer_destroy(QEMUTimer **timer)
 {
-    assert(*timer != NULL);
+    if (*timer == NULL) {
+        return;
+    }
 
     timer_free(*timer);
     *timer = NULL;
@@ -272,7 +280,7 @@ void throttle_timers_destroy(ThrottleTimers *tt)
 /* is any throttling timer configured */
 bool throttle_timers_are_initialized(ThrottleTimers *tt)
 {
-    if (tt->timers[0]) {
+    if (tt->timers[THROTTLE_READ] || tt->timers[THROTTLE_WRITE]) {
         return true;
     }
 
@@ -424,8 +432,12 @@ bool throttle_schedule_timer(ThrottleState *ts,
 {
     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(timer);
+
     must_wait = throttle_compute_timer(ts,
                                        is_write,
                                        now,
@@ -437,12 +449,12 @@ bool throttle_schedule_timer(ThrottleState *ts,
     }
 
     /* request throttled and timer pending -> do nothing */
-    if (timer_pending(tt->timers[is_write])) {
+    if (timer_pending(timer)) {
         return true;
     }
 
     /* request throttled and timer not pending -> arm timer */
-    timer_mod(tt->timers[is_write], next_timestamp);
+    timer_mod(timer, next_timestamp);
     return true;
 }
 
-- 
2.34.1

Re: [PATCH v3 3/6] throttle: support read-only and write-only

[Hanna Czenczek]

On 13.07.23 08:41, zhenwei pi wrote:
> Only one direction is necessary in several scenarios:
> - a read-only disk
> - operations on a device are considered as *write* only. For example,
>    encrypt/decrypt/sign/verify operations on a cryptodev use a single
>    *write* timer(read timer callback is defined, but never invoked).
>
> Allow a single direction in throttle, this reduces memory, and uplayer
> does not need a dummy callback any more.
>
> Reviewed-by: Alberto Garcia<berto@igalia.com>
> Signed-off-by: zhenwei pi<pizhenwei@bytedance.com>
> ---
>   util/throttle.c | 32 ++++++++++++++++++++++----------
>   1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/util/throttle.c b/util/throttle.c
> index 5642e61763..c0bd0c26c3 100644
> --- a/util/throttle.c
> +++ b/util/throttle.c
> @@ -199,12 +199,17 @@ static bool throttle_compute_timer(ThrottleState *ts,
>   void throttle_timers_attach_aio_context(ThrottleTimers *tt,
>                                           AioContext *new_context)
>   {
> -    tt->timers[THROTTLE_READ] =
> -        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> -                      tt->timer_cb[THROTTLE_READ], tt->timer_opaque);
> -    tt->timers[THROTTLE_WRITE] =
> -        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> -                      tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);
> +    if (tt->timer_cb[THROTTLE_READ]) {
> +        tt->timers[THROTTLE_READ] =
> +            aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> +                          tt->timer_cb[THROTTLE_READ], tt->timer_opaque);
> +    }
> +
> +    if (tt->timer_cb[THROTTLE_WRITE]) {
> +        tt->timers[THROTTLE_WRITE] =
> +            aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> +                          tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);
> +    }

  I think a `for (int i = 0; i < THROTTLE_MAX; i++)` loop would make 
this nicer.  (Again: Optional.)

>   }
>   
>   /*

[...]

> @@ -272,7 +280,7 @@ void throttle_timers_destroy(ThrottleTimers *tt)
>   /* is any throttling timer configured */
>   bool throttle_timers_are_initialized(ThrottleTimers *tt)
>   {
> -    if (tt->timers[0]) {
> +    if (tt->timers[THROTTLE_READ] || tt->timers[THROTTLE_WRITE]) {

Not wrong, but I’d prefer the more general

```
for (int i = 0; i < THROTTLE_MAX; i++) {
     if (tt->timers[i]) {
         return true;
     }
}
return false;
```

>           return true;
>       }
>   
> @@ -424,8 +432,12 @@ bool throttle_schedule_timer(ThrottleState *ts,
>   {
>       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];

Could be shorter as `timer = tt->timers[is_write ? THROTTLE_WRITE : 
THROTTLE_READ];`.

Anyway, I only have style suggestion to offer, so either way:

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

> +    assert(timer);
> +
>       must_wait = throttle_compute_timer(ts,
>                                          is_write,
>                                          now,

[PATCH v3 4/6] test-throttle: test read only and write only

[zhenwei pi]

Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 tests/unit/test-throttle.c | 66 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c
index a60b5fe22e..5547837a58 100644
--- a/tests/unit/test-throttle.c
+++ b/tests/unit/test-throttle.c
@@ -184,6 +184,70 @@ static void test_init(void)
     throttle_timers_destroy(tt);
 }
 
+static void test_init_readonly(void)
+{
+    int i;
+
+    tt = &tgm.throttle_timers;
+
+    /* fill the structures with crap */
+    memset(&ts, 1, sizeof(ts));
+    memset(tt, 1, sizeof(*tt));
+
+    /* init structures */
+    throttle_init(&ts);
+    throttle_timers_init(tt, ctx, QEMU_CLOCK_VIRTUAL,
+                         read_timer_cb, NULL, &ts);
+
+    /* check initialized fields */
+    g_assert(tt->clock_type == QEMU_CLOCK_VIRTUAL);
+    g_assert(tt->timers[THROTTLE_READ]);
+    g_assert(!tt->timers[THROTTLE_WRITE]);
+
+    /* check other fields where cleared */
+    g_assert(!ts.previous_leak);
+    g_assert(!ts.cfg.op_size);
+    for (i = 0; i < BUCKETS_COUNT; i++) {
+        g_assert(!ts.cfg.buckets[i].avg);
+        g_assert(!ts.cfg.buckets[i].max);
+        g_assert(!ts.cfg.buckets[i].level);
+    }
+
+    throttle_timers_destroy(tt);
+}
+
+static void test_init_writeonly(void)
+{
+    int i;
+
+    tt = &tgm.throttle_timers;
+
+    /* fill the structures with crap */
+    memset(&ts, 1, sizeof(ts));
+    memset(tt, 1, sizeof(*tt));
+
+    /* init structures */
+    throttle_init(&ts);
+    throttle_timers_init(tt, ctx, QEMU_CLOCK_VIRTUAL,
+                         NULL, write_timer_cb, &ts);
+
+    /* check initialized fields */
+    g_assert(tt->clock_type == QEMU_CLOCK_VIRTUAL);
+    g_assert(!tt->timers[THROTTLE_READ]);
+    g_assert(tt->timers[THROTTLE_WRITE]);
+
+    /* check other fields where cleared */
+    g_assert(!ts.previous_leak);
+    g_assert(!ts.cfg.op_size);
+    for (i = 0; i < BUCKETS_COUNT; i++) {
+        g_assert(!ts.cfg.buckets[i].avg);
+        g_assert(!ts.cfg.buckets[i].max);
+        g_assert(!ts.cfg.buckets[i].level);
+    }
+
+    throttle_timers_destroy(tt);
+}
+
 static void test_destroy(void)
 {
     int i;
@@ -752,6 +816,8 @@ int main(int argc, char **argv)
     g_test_add_func("/throttle/leak_bucket",        test_leak_bucket);
     g_test_add_func("/throttle/compute_wait",       test_compute_wait);
     g_test_add_func("/throttle/init",               test_init);
+    g_test_add_func("/throttle/init_readonly",      test_init_readonly);
+    g_test_add_func("/throttle/init_writeonly",     test_init_writeonly);
     g_test_add_func("/throttle/destroy",            test_destroy);
     g_test_add_func("/throttle/have_timer",         test_have_timer);
     g_test_add_func("/throttle/detach_attach",      test_detach_attach);
-- 
2.34.1

Re: [PATCH v3 4/6] test-throttle: test read only and write only

[Hanna Czenczek]

On 13.07.23 08:41, zhenwei pi wrote:
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>   tests/unit/test-throttle.c | 66 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 66 insertions(+)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

[PATCH v3 5/6] cryptodev: use NULL throttle timer cb for read direction

[zhenwei pi]

Operations on a crytpodev are considered as *write* only, the callback
of read direction is never invoked. Use NULL instead of an unreachable
path(cryptodev_backend_throttle_timer_cb on read direction).

Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 backends/cryptodev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index 7d29517843..5cfa25c61c 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -331,8 +331,7 @@ static void cryptodev_backend_set_throttle(CryptoDevBackend *backend, int field,
     if (!enabled) {
         throttle_init(&backend->ts);
         throttle_timers_init(&backend->tt, qemu_get_aio_context(),
-                             QEMU_CLOCK_REALTIME,
-                             cryptodev_backend_throttle_timer_cb, /* FIXME */
+                             QEMU_CLOCK_REALTIME, NULL,
                              cryptodev_backend_throttle_timer_cb, backend);
     }
 
-- 
2.34.1

Re: [PATCH v3 5/6] cryptodev: use NULL throttle timer cb for read direction

[Hanna Czenczek]

On 13.07.23 08:41, zhenwei pi wrote:
> Operations on a crytpodev are considered as *write* only, the callback

s/crytpodev/cryptodev/

> of read direction is never invoked. Use NULL instead of an unreachable
> path(cryptodev_backend_throttle_timer_cb on read direction).
>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>   backends/cryptodev.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> index 7d29517843..5cfa25c61c 100644
> --- a/backends/cryptodev.c
> +++ b/backends/cryptodev.c
> @@ -331,8 +331,7 @@ static void cryptodev_backend_set_throttle(CryptoDevBackend *backend, int field,
>       if (!enabled) {
>           throttle_init(&backend->ts);
>           throttle_timers_init(&backend->tt, qemu_get_aio_context(),
> -                             QEMU_CLOCK_REALTIME,
> -                             cryptodev_backend_throttle_timer_cb, /* FIXME */

Now it’ll be gone (good), but if you happen to add a FIXME again, it 
would be nice to explain in the same comment in the code what exactly 
needs fixing, and why it isn‘t being fixed at this point.

> +                             QEMU_CLOCK_REALTIME, NULL,
>                                cryptodev_backend_throttle_timer_cb, backend);
>       }

With the typo in the commit message fixed:

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

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

[zhenwei pi]

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(-)

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;
-- 
2.34.1

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

PING: [PATCH v3 0/6] Misc fixes for throttle

[zhenwei pi]

Hi Kevin, Hanna,

Patch 1 -> patch 5 of this series are already reviewed by Alberto(these 
affects throttle framework only), the patch 6 affects qemu block layer, 
would you please review this(in the further step, merge this series if 
this is acceptable)?

On 7/13/23 14:41, zhenwei pi wrote:
> v2 -> v3:
> - patch 1 -> patch 5 are already reviewed by Alberto
> - append patch 6: throttle: use enum ThrottleType instead of bool is_write
> 
> v1 -> v2:
> - rename 'ThrottleTimerType' to 'ThrottleType'
> - add assertion to throttle_schedule_timer
> 
> v1:
> - introduce enum ThrottleTimerType instead of timers[0], timer[1]...
> - support read-only and write-only for throttle
> - adapt related test codes
> - cryptodev uses a write-only throttle timer
> 
> Zhenwei Pi (6):
>    throttle: introduce enum ThrottleType
>    test-throttle: use enum ThrottleType
>    throttle: support read-only and write-only
>    test-throttle: test read only and write only
>    cryptodev: use NULL throttle timer cb for read direction
>    throttle: use enum ThrottleType instead of bool is_write
> 
>   backends/cryptodev.c        | 12 +++---
>   block/throttle-groups.c     |  6 ++-
>   fsdev/qemu-fsdev-throttle.c |  8 ++--
>   include/qemu/throttle.h     | 15 +++++---
>   tests/unit/test-throttle.c  | 76 ++++++++++++++++++++++++++++++++++---
>   util/throttle.c             | 64 +++++++++++++++++++------------
>   6 files changed, 136 insertions(+), 45 deletions(-)
> 

-- 
zhenwei pi