[PATCH 0/5] Misc fixes for throttle

[PATCH 0/5] Misc fixes for throttle

[zhenwei pi]

Hi,

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 (5):
  throttle: introduce enum ThrottleTimerType
  test-throttle: use enum ThrottleTimerType
  throttle: support read-only and write-only
  test-throttle: test read only and write only
  cryptodev: use NULL throttle timer cb for read direction

 backends/cryptodev.c       |  3 +-
 include/qemu/throttle.h    | 12 +++++--
 tests/unit/test-throttle.c | 72 ++++++++++++++++++++++++++++++++++++--
 util/throttle.c            | 28 ++++++++++-----
 4 files changed, 98 insertions(+), 17 deletions(-)

-- 
2.34.1

[PATCH 1/5] throttle: introduce enum ThrottleTimerType

[zhenwei pi]

Use enum ThrottleTimerType instead of number index.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 include/qemu/throttle.h | 12 +++++++++---
 util/throttle.c         | 16 +++++++++-------
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 05f6346137..0956094115 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -99,13 +99,19 @@ typedef struct ThrottleState {
     int64_t previous_leak;    /* timestamp of the last leak done */
 } ThrottleState;
 
+typedef enum {
+    THROTTLE_TIMER_READ = 0,
+    THROTTLE_TIMER_WRITE,
+    THROTTLE_TIMER_MAX
+} ThrottleTimerType;
+
 typedef struct ThrottleTimers {
-    QEMUTimer *timers[2];     /* timers used to do the throttling */
+    /* timers used to do the throttling */
+    QEMUTimer *timers[THROTTLE_TIMER_MAX];
     QEMUClockType clock_type; /* the clock used */
 
     /* Callbacks */
-    QEMUTimerCB *read_timer_cb;
-    QEMUTimerCB *write_timer_cb;
+    QEMUTimerCB *timer_cb[THROTTLE_TIMER_MAX];
     void *timer_opaque;
 } ThrottleTimers;
 
diff --git a/util/throttle.c b/util/throttle.c
index 81f247a8d1..c1cc24831c 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_TIMER_READ] =
+        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+                      tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque);
+    tt->timers[THROTTLE_TIMER_WRITE] =
+        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+                      tt->timer_cb[THROTTLE_TIMER_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_TIMER_READ] = read_timer_cb;
+    tt->timer_cb[THROTTLE_TIMER_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_TIMER_MAX; i++) {
         throttle_timer_destroy(&tt->timers[i]);
     }
 }
-- 
2.34.1

[PATCH 2/5] test-throttle: use enum ThrottleTimerType

[zhenwei pi]

Use enum ThrottleTimerType instead in the throttle test codes.

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..5fc2de4d47 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_TIMER_READ]);
+    g_assert(tt->timers[THROTTLE_TIMER_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_TIMER_MAX; i++) {
         g_assert(!tt->timers[i]);
     }
 }
-- 
2.34.1

[PATCH 3/5] 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.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 util/throttle.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/util/throttle.c b/util/throttle.c
index c1cc24831c..01faee783c 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_TIMER_READ] =
-        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
-                      tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque);
-    tt->timers[THROTTLE_TIMER_WRITE] =
-        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
-                      tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque);
+    if (tt->timer_cb[THROTTLE_TIMER_READ]) {
+        tt->timers[THROTTLE_TIMER_READ] =
+            aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+                          tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque);
+    }
+
+    if (tt->timer_cb[THROTTLE_TIMER_WRITE]) {
+        tt->timers[THROTTLE_TIMER_WRITE] =
+            aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+                          tt->timer_cb[THROTTLE_TIMER_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_TIMER_READ] || tt->timers[THROTTLE_TIMER_WRITE]) {
         return true;
     }
 
-- 
2.34.1

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

[zhenwei pi]

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 5fc2de4d47..d7973980d1 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_TIMER_READ]);
+    g_assert(!tt->timers[THROTTLE_TIMER_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_TIMER_READ]);
+    g_assert(tt->timers[THROTTLE_TIMER_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

[PATCH 5/5] 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).

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 94ca393cee..19c24ccfad 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 1/5] throttle: introduce enum ThrottleTimerType

[Alberto Garcia]

On Sun 25 Jun 2023 04:56:27 PM +08, zhenwei pi wrote:
> Use enum ThrottleTimerType instead of number index.

> +typedef enum {
> +    THROTTLE_TIMER_READ = 0,
> +    THROTTLE_TIMER_WRITE,
> +    THROTTLE_TIMER_MAX
> +} ThrottleTimerType;

If you're doing this I suppose you could also change 'bool is_write'
with something like 'ThrottleTimerType timer', i.e

static bool throttle_compute_timer(ThrottleState *ts,
                                   ThrottleTimerType timer,
                                   int64_t now,
                                   int64_t *next_timestamp)

Berto

Re: [PATCH 2/5] test-throttle: use enum ThrottleTimerType

[Alberto Garcia]

On Sun 25 Jun 2023 04:56:28 PM +08, zhenwei pi wrote:
> Use enum ThrottleTimerType instead in the throttle test codes.
>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

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

[Alberto Garcia]

On Sun 25 Jun 2023 04:56:31 PM +08, zhenwei pi wrote:
> 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).
>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

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

[Alberto Garcia]

On Sun 25 Jun 2023 04:56:30 PM +08, zhenwei pi wrote:
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

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

[Alberto Garcia]

On Sun 25 Jun 2023 04:56:29 PM +08, zhenwei pi wrote:
>  void throttle_timers_attach_aio_context(ThrottleTimers *tt,
>                                          AioContext *new_context)
>  {
> -    tt->timers[THROTTLE_TIMER_READ] =
> -        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> -                      tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque);
> -    tt->timers[THROTTLE_TIMER_WRITE] =
> -        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> -                      tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque);
> +    if (tt->timer_cb[THROTTLE_TIMER_READ]) {
> +        tt->timers[THROTTLE_TIMER_READ] =
> +            aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> +                          tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque);
> +    }
> +
> +    if (tt->timer_cb[THROTTLE_TIMER_WRITE]) {
> +        tt->timers[THROTTLE_TIMER_WRITE] =
> +            aio_timer_new(new_context, tt->clock_type, SCALE_NS,
> +                          tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque);
> +    }
>  }

If now any of the timers can be NULL, don't you want to add additional
checks / assertions to (at least) throttle_schedule_timer() ?

Berto

Re: Re: [PATCH 1/5] throttle: introduce enum ThrottleTimerType

[zhenwei pi]

On 6/27/23 05:24, Alberto Garcia wrote:
> On Sun 25 Jun 2023 04:56:27 PM +08, zhenwei pi wrote:
>> Use enum ThrottleTimerType instead of number index.
> 
>> +typedef enum {
>> +    THROTTLE_TIMER_READ = 0,
>> +    THROTTLE_TIMER_WRITE,
>> +    THROTTLE_TIMER_MAX
>> +} ThrottleTimerType;
> 
> If you're doing this I suppose you could also change 'bool is_write'
> with something like 'ThrottleTimerType timer', i.e
> 
> static bool throttle_compute_timer(ThrottleState *ts,
>                                     ThrottleTimerType timer,
>                                     int64_t now,
>                                     int64_t *next_timestamp)
> 
> Berto

Hi,

Right, it's in my plan. But I prefer to do this in a followup patch 
after this series applies. Because this API leads changes from other 
subsystems.

By the way, I prepare to rename 'THROTTLE_TIMER_READ' to 'THROTTLE_READ' 
in next version.

Do you have any suggestion?

-- 
zhenwei pi

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

[zhenwei pi]

On 6/27/23 05:38, Alberto Garcia wrote:
> On Sun 25 Jun 2023 04:56:29 PM +08, zhenwei pi wrote:
>>   void throttle_timers_attach_aio_context(ThrottleTimers *tt,
>>                                           AioContext *new_context)
>>   {
>> -    tt->timers[THROTTLE_TIMER_READ] =
>> -        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
>> -                      tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque);
>> -    tt->timers[THROTTLE_TIMER_WRITE] =
>> -        aio_timer_new(new_context, tt->clock_type, SCALE_NS,
>> -                      tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque);
>> +    if (tt->timer_cb[THROTTLE_TIMER_READ]) {
>> +        tt->timers[THROTTLE_TIMER_READ] =
>> +            aio_timer_new(new_context, tt->clock_type, SCALE_NS,
>> +                          tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque);
>> +    }
>> +
>> +    if (tt->timer_cb[THROTTLE_TIMER_WRITE]) {
>> +        tt->timers[THROTTLE_TIMER_WRITE] =
>> +            aio_timer_new(new_context, tt->clock_type, SCALE_NS,
>> +                          tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque);
>> +    }
>>   }
> 
> If now any of the timers can be NULL, don't you want to add additional
> checks / assertions to (at least) throttle_schedule_timer() ?
> 
> Berto

OK.

-- 
zhenwei pi