* [PATCH v4 1/9] throttle: introduce enum ThrottleDirection
2023-07-24 10:09 [PATCH v4 0/9] Misc fixes for throttle zhenwei pi
@ 2023-07-24 10:09 ` zhenwei pi
2023-07-24 10:09 ` [PATCH v4 2/9] test-throttle: use " zhenwei pi
` (7 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: zhenwei pi @ 2023-07-24 10:09 UTC (permalink / raw)
To: berto, kwolf, groug, qemu_oss, hreitz
Cc: qemu-devel, qemu-block, berrange, zhenwei pi
Use enum ThrottleDirection instead of number index.
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.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..9ca5ab8197 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
+} ThrottleDirection;
+
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v4 2/9] test-throttle: use enum ThrottleDirection
2023-07-24 10:09 [PATCH v4 0/9] Misc fixes for throttle zhenwei pi
2023-07-24 10:09 ` [PATCH v4 1/9] throttle: introduce enum ThrottleDirection zhenwei pi
@ 2023-07-24 10:09 ` zhenwei pi
2023-07-24 10:09 ` [PATCH v4 3/9] throttle: support read-only and write-only zhenwei pi
` (6 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: zhenwei pi @ 2023-07-24 10:09 UTC (permalink / raw)
To: berto, kwolf, groug, qemu_oss, hreitz
Cc: qemu-devel, qemu-block, berrange, zhenwei pi
Use enum ThrottleDirection instead in the throttle test codes.
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v4 3/9] throttle: support read-only and write-only
2023-07-24 10:09 [PATCH v4 0/9] Misc fixes for throttle zhenwei pi
2023-07-24 10:09 ` [PATCH v4 1/9] throttle: introduce enum ThrottleDirection zhenwei pi
2023-07-24 10:09 ` [PATCH v4 2/9] test-throttle: use " zhenwei pi
@ 2023-07-24 10:09 ` zhenwei pi
2023-07-24 10:09 ` [PATCH v4 4/9] test-throttle: test read only and write only zhenwei pi
` (5 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: zhenwei pi @ 2023-07-24 10:09 UTC (permalink / raw)
To: berto, kwolf, groug, qemu_oss, hreitz
Cc: qemu-devel, qemu-block, berrange, 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>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
util/throttle.c | 42 ++++++++++++++++++++++++++++--------------
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/util/throttle.c b/util/throttle.c
index 5642e61763..0439028d21 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -199,12 +199,15 @@ 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);
+ ThrottleDirection dir;
+
+ for (dir = THROTTLE_READ; dir < THROTTLE_MAX; dir++) {
+ if (tt->timer_cb[dir]) {
+ tt->timers[dir] =
+ aio_timer_new(new_context, tt->clock_type, SCALE_NS,
+ tt->timer_cb[dir], tt->timer_opaque);
+ }
+ }
}
/*
@@ -235,6 +238,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 +251,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;
@@ -256,10 +262,10 @@ static void throttle_timer_destroy(QEMUTimer **timer)
/* Remove timers from event loop */
void throttle_timers_detach_aio_context(ThrottleTimers *tt)
{
- int i;
+ ThrottleDirection dir;
- for (i = 0; i < THROTTLE_MAX; i++) {
- throttle_timer_destroy(&tt->timers[i]);
+ for (dir = THROTTLE_READ; dir < THROTTLE_MAX; dir++) {
+ throttle_timer_destroy(&tt->timers[dir]);
}
}
@@ -272,8 +278,12 @@ void throttle_timers_destroy(ThrottleTimers *tt)
/* is any throttling timer configured */
bool throttle_timers_are_initialized(ThrottleTimers *tt)
{
- if (tt->timers[0]) {
- return true;
+ ThrottleDirection dir;
+
+ for (dir = THROTTLE_READ; dir < THROTTLE_MAX; dir++) {
+ if (tt->timers[dir]) {
+ return true;
+ }
}
return false;
@@ -424,8 +434,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 +451,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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v4 4/9] test-throttle: test read only and write only
2023-07-24 10:09 [PATCH v4 0/9] Misc fixes for throttle zhenwei pi
` (2 preceding siblings ...)
2023-07-24 10:09 ` [PATCH v4 3/9] throttle: support read-only and write-only zhenwei pi
@ 2023-07-24 10:09 ` zhenwei pi
2023-07-24 10:09 ` [PATCH v4 5/9] cryptodev: use NULL throttle timer cb for read direction zhenwei pi
` (4 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: zhenwei pi @ 2023-07-24 10:09 UTC (permalink / raw)
To: berto, kwolf, groug, qemu_oss, hreitz
Cc: qemu-devel, qemu-block, berrange, zhenwei pi
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v4 5/9] cryptodev: use NULL throttle timer cb for read direction
2023-07-24 10:09 [PATCH v4 0/9] Misc fixes for throttle zhenwei pi
` (3 preceding siblings ...)
2023-07-24 10:09 ` [PATCH v4 4/9] test-throttle: test read only and write only zhenwei pi
@ 2023-07-24 10:09 ` zhenwei pi
2023-07-24 10:09 ` [PATCH v4 6/9] throttle: use enum ThrottleDirection instead of bool is_write zhenwei pi
` (3 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: zhenwei pi @ 2023-07-24 10:09 UTC (permalink / raw)
To: berto, kwolf, groug, qemu_oss, hreitz
Cc: qemu-devel, qemu-block, berrange, zhenwei pi, Gonglei
Operations on a cryptodev 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).
The dummy read timer(never invoked) is already removed here, it means
that the 'FIXME' tag is no longer needed.
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v4 6/9] throttle: use enum ThrottleDirection instead of bool is_write
2023-07-24 10:09 [PATCH v4 0/9] Misc fixes for throttle zhenwei pi
` (4 preceding siblings ...)
2023-07-24 10:09 ` [PATCH v4 5/9] cryptodev: use NULL throttle timer cb for read direction zhenwei pi
@ 2023-07-24 10:09 ` zhenwei pi
2023-07-27 15:41 ` Hanna Czenczek
2023-07-24 10:09 ` [PATCH v4 7/9] throttle: use THROTTLE_MAX/ARRAY_SIZE for hard code zhenwei pi
` (2 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: zhenwei pi @ 2023-07-24 10:09 UTC (permalink / raw)
To: berto, kwolf, groug, qemu_oss, hreitz
Cc: qemu-devel, qemu-block, berrange, zhenwei pi
enum ThrottleDirection is already there, use ThrottleDirection 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 | 5 +++--
tests/unit/test-throttle.c | 4 ++--
util/throttle.c | 31 +++++++++++++++++--------------
6 files changed, 36 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..3847d27801 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;
+ ThrottleDirection direction = 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, direction);
/* 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);
+ ThrottleDirection direction = 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, direction, 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..1c137d6f0f 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)
{
+ ThrottleDirection direction = 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, direction) ||
!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, direction, 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, direction)) {
qemu_co_queue_next(&fst->throttled_reqs[is_write]);
}
}
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 9ca5ab8197..181245d29b 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -154,9 +154,10 @@ void throttle_config_init(ThrottleConfig *cfg);
/* usage */
bool throttle_schedule_timer(ThrottleState *ts,
ThrottleTimers *tt,
- bool is_write);
+ ThrottleDirection direction);
-void throttle_account(ThrottleState *ts, bool is_write, uint64_t size);
+void throttle_account(ThrottleState *ts, ThrottleDirection direction,
+ 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 0439028d21..9a37209bb8 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 direction
* @ret: time to wait
*/
static int64_t throttle_compute_wait_for(ThrottleState *ts,
- bool is_write)
+ ThrottleDirection direction)
{
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[direction][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 direction
* @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,
+ ThrottleDirection direction,
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, direction);
/* if the code must wait compute when the next timer should fire */
if (wait) {
@@ -425,23 +425,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 direction
* @ret: true if the timer has been scheduled else false
*/
bool throttle_schedule_timer(ThrottleState *ts,
ThrottleTimers *tt,
- bool is_write)
+ ThrottleDirection direction)
{
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(direction < THROTTLE_MAX);
+ timer = tt->timers[direction];
assert(timer);
must_wait = throttle_compute_timer(ts,
- is_write,
+ direction,
now,
&next_timestamp);
@@ -462,10 +463,11 @@ bool throttle_schedule_timer(ThrottleState *ts,
/* do the accounting for this operation
*
- * @is_write: the type of operation (read/write)
+ * @throttle: throttle direction
* @size: the size of the operation
*/
-void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)
+void throttle_account(ThrottleState *ts, ThrottleDirection direction,
+ uint64_t size)
{
const BucketType bucket_types_size[2][2] = {
{ THROTTLE_BPS_TOTAL, THROTTLE_BPS_READ },
@@ -478,6 +480,7 @@ void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)
double units = 1.0;
unsigned i;
+ assert(direction < 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;
@@ -486,13 +489,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[direction][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[direction][i]];
bkt->level += units;
if (bkt->burst_length > 1) {
bkt->burst_level += units;
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 6/9] throttle: use enum ThrottleDirection instead of bool is_write
2023-07-24 10:09 ` [PATCH v4 6/9] throttle: use enum ThrottleDirection instead of bool is_write zhenwei pi
@ 2023-07-27 15:41 ` Hanna Czenczek
0 siblings, 0 replies; 15+ messages in thread
From: Hanna Czenczek @ 2023-07-27 15:41 UTC (permalink / raw)
To: zhenwei pi, berto, kwolf, groug, qemu_oss
Cc: qemu-devel, qemu-block, berrange
On 24.07.23 12:09, zhenwei pi wrote:
> enum ThrottleDirection is already there, use ThrottleDirection 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 | 5 +++--
> tests/unit/test-throttle.c | 4 ++--
> util/throttle.c | 31 +++++++++++++++++--------------
> 6 files changed, 36 insertions(+), 27 deletions(-)
[...]
> diff --git a/util/throttle.c b/util/throttle.c
> index 0439028d21..9a37209bb8 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 direction
The parameter is now named @direction, so this should be changed. (Same
in all other comments describing functions in this patch.)
With that done (in all 4 places):
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
> * @ret: time to wait
> */
> static int64_t throttle_compute_wait_for(ThrottleState *ts,
> - bool is_write)
> + ThrottleDirection direction)
> {
> BucketType to_check[2][4] = { {THROTTLE_BPS_TOTAL,
> THROTTLE_OPS_TOTAL,
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 7/9] throttle: use THROTTLE_MAX/ARRAY_SIZE for hard code
2023-07-24 10:09 [PATCH v4 0/9] Misc fixes for throttle zhenwei pi
` (5 preceding siblings ...)
2023-07-24 10:09 ` [PATCH v4 6/9] throttle: use enum ThrottleDirection instead of bool is_write zhenwei pi
@ 2023-07-24 10:09 ` zhenwei pi
2023-07-27 15:44 ` Hanna Czenczek
2023-07-24 10:09 ` [PATCH v4 8/9] fsdev: Use ThrottleDirection instread of bool is_write zhenwei pi
2023-07-24 10:09 ` [PATCH v4 9/9] block/throttle-groups: " zhenwei pi
8 siblings, 1 reply; 15+ messages in thread
From: zhenwei pi @ 2023-07-24 10:09 UTC (permalink / raw)
To: berto, kwolf, groug, qemu_oss, hreitz
Cc: qemu-devel, qemu-block, berrange, zhenwei pi
The first dimension of both to_check and
bucket_types_size/bucket_types_units is used as throttle direction,
use THROTTLE_MAX instead of hard coded number. Also use ARRAY_SIZE()
to avoid hard coded number for the second dimension.
Hanna noticed that the two array should be static. Yes, turn them
into static variables.
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
util/throttle.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/util/throttle.c b/util/throttle.c
index 9a37209bb8..9baa6b8a3a 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -142,7 +142,8 @@ int64_t throttle_compute_wait(LeakyBucket *bkt)
static int64_t throttle_compute_wait_for(ThrottleState *ts,
ThrottleDirection direction)
{
- BucketType to_check[2][4] = { {THROTTLE_BPS_TOTAL,
+ static const BucketType to_check[THROTTLE_MAX][4] = {
+ {THROTTLE_BPS_TOTAL,
THROTTLE_OPS_TOTAL,
THROTTLE_BPS_READ,
THROTTLE_OPS_READ},
@@ -153,7 +154,7 @@ static int64_t throttle_compute_wait_for(ThrottleState *ts,
int64_t wait, max_wait = 0;
int i;
- for (i = 0; i < 4; i++) {
+ for (i = 0; i < ARRAY_SIZE(to_check[THROTTLE_READ]); i++) {
BucketType index = to_check[direction][i];
wait = throttle_compute_wait(&ts->cfg.buckets[index]);
if (wait > max_wait) {
@@ -469,11 +470,11 @@ bool throttle_schedule_timer(ThrottleState *ts,
void throttle_account(ThrottleState *ts, ThrottleDirection direction,
uint64_t size)
{
- const BucketType bucket_types_size[2][2] = {
+ static const BucketType bucket_types_size[THROTTLE_MAX][2] = {
{ THROTTLE_BPS_TOTAL, THROTTLE_BPS_READ },
{ THROTTLE_BPS_TOTAL, THROTTLE_BPS_WRITE }
};
- const BucketType bucket_types_units[2][2] = {
+ static const BucketType bucket_types_units[THROTTLE_MAX][2] = {
{ THROTTLE_OPS_TOTAL, THROTTLE_OPS_READ },
{ THROTTLE_OPS_TOTAL, THROTTLE_OPS_WRITE }
};
@@ -486,7 +487,7 @@ void throttle_account(ThrottleState *ts, ThrottleDirection direction,
units = (double) size / ts->cfg.op_size;
}
- for (i = 0; i < 2; i++) {
+ for (i = 0; i < ARRAY_SIZE(bucket_types_size[THROTTLE_READ]); i++) {
LeakyBucket *bkt;
bkt = &ts->cfg.buckets[bucket_types_size[direction][i]];
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 7/9] throttle: use THROTTLE_MAX/ARRAY_SIZE for hard code
2023-07-24 10:09 ` [PATCH v4 7/9] throttle: use THROTTLE_MAX/ARRAY_SIZE for hard code zhenwei pi
@ 2023-07-27 15:44 ` Hanna Czenczek
0 siblings, 0 replies; 15+ messages in thread
From: Hanna Czenczek @ 2023-07-27 15:44 UTC (permalink / raw)
To: zhenwei pi, berto, kwolf, groug, qemu_oss
Cc: qemu-devel, qemu-block, berrange
On 24.07.23 12:09, zhenwei pi wrote:
> The first dimension of both to_check and
> bucket_types_size/bucket_types_units is used as throttle direction,
> use THROTTLE_MAX instead of hard coded number. Also use ARRAY_SIZE()
> to avoid hard coded number for the second dimension.
>
> Hanna noticed that the two array should be static. Yes, turn them
> into static variables.
>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
> util/throttle.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
Using ARRAY_SIZE() for the other dimension is quite nice, too, indeed.
Thanks!
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 8/9] fsdev: Use ThrottleDirection instread of bool is_write
2023-07-24 10:09 [PATCH v4 0/9] Misc fixes for throttle zhenwei pi
` (6 preceding siblings ...)
2023-07-24 10:09 ` [PATCH v4 7/9] throttle: use THROTTLE_MAX/ARRAY_SIZE for hard code zhenwei pi
@ 2023-07-24 10:09 ` zhenwei pi
2023-07-27 15:48 ` Hanna Czenczek
2023-07-24 10:09 ` [PATCH v4 9/9] block/throttle-groups: " zhenwei pi
8 siblings, 1 reply; 15+ messages in thread
From: zhenwei pi @ 2023-07-24 10:09 UTC (permalink / raw)
To: berto, kwolf, groug, qemu_oss, hreitz
Cc: qemu-devel, qemu-block, berrange, zhenwei pi
'bool is_write' style is obsolete from throttle framework, adapt
fsdev to the new style.
Cc: Greg Kurz <groug@kaod.org>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
fsdev/qemu-fsdev-throttle.c | 14 +++++++-------
fsdev/qemu-fsdev-throttle.h | 4 ++--
hw/9pfs/cofile.c | 4 ++--
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c
index 1c137d6f0f..d912da906d 100644
--- a/fsdev/qemu-fsdev-throttle.c
+++ b/fsdev/qemu-fsdev-throttle.c
@@ -94,22 +94,22 @@ void fsdev_throttle_init(FsThrottle *fst)
}
}
-void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write,
+void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst,
+ ThrottleDirection direction,
struct iovec *iov, int iovcnt)
{
- ThrottleDirection direction = is_write ? THROTTLE_WRITE : THROTTLE_READ;
-
+ assert(direction < THROTTLE_MAX);
if (throttle_enabled(&fst->cfg)) {
if (throttle_schedule_timer(&fst->ts, &fst->tt, direction) ||
- !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {
- qemu_co_queue_wait(&fst->throttled_reqs[is_write], NULL);
+ !qemu_co_queue_empty(&fst->throttled_reqs[direction])) {
+ qemu_co_queue_wait(&fst->throttled_reqs[direction], NULL);
}
throttle_account(&fst->ts, direction, iov_size(iov, iovcnt));
- if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write]) &&
+ if (!qemu_co_queue_empty(&fst->throttled_reqs[direction]) &&
!throttle_schedule_timer(&fst->ts, &fst->tt, direction)) {
- qemu_co_queue_next(&fst->throttled_reqs[is_write]);
+ qemu_co_queue_next(&fst->throttled_reqs[direction]);
}
}
}
diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h
index a21aecddc7..daa8ca2494 100644
--- a/fsdev/qemu-fsdev-throttle.h
+++ b/fsdev/qemu-fsdev-throttle.h
@@ -23,14 +23,14 @@ typedef struct FsThrottle {
ThrottleState ts;
ThrottleTimers tt;
ThrottleConfig cfg;
- CoQueue throttled_reqs[2];
+ CoQueue throttled_reqs[THROTTLE_MAX];
} FsThrottle;
int fsdev_throttle_parse_opts(QemuOpts *, FsThrottle *, Error **);
void fsdev_throttle_init(FsThrottle *);
-void coroutine_fn fsdev_co_throttle_request(FsThrottle *, bool ,
+void coroutine_fn fsdev_co_throttle_request(FsThrottle *, ThrottleDirection ,
struct iovec *, int);
void fsdev_throttle_cleanup(FsThrottle *);
diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
index 9c5344039e..71174c3e4a 100644
--- a/hw/9pfs/cofile.c
+++ b/hw/9pfs/cofile.c
@@ -252,7 +252,7 @@ int coroutine_fn v9fs_co_pwritev(V9fsPDU *pdu, V9fsFidState *fidp,
if (v9fs_request_cancelled(pdu)) {
return -EINTR;
}
- fsdev_co_throttle_request(s->ctx.fst, true, iov, iovcnt);
+ fsdev_co_throttle_request(s->ctx.fst, THROTTLE_WRITE, iov, iovcnt);
v9fs_co_run_in_worker(
{
err = s->ops->pwritev(&s->ctx, &fidp->fs, iov, iovcnt, offset);
@@ -272,7 +272,7 @@ int coroutine_fn v9fs_co_preadv(V9fsPDU *pdu, V9fsFidState *fidp,
if (v9fs_request_cancelled(pdu)) {
return -EINTR;
}
- fsdev_co_throttle_request(s->ctx.fst, false, iov, iovcnt);
+ fsdev_co_throttle_request(s->ctx.fst, THROTTLE_READ, iov, iovcnt);
v9fs_co_run_in_worker(
{
err = s->ops->preadv(&s->ctx, &fidp->fs, iov, iovcnt, offset);
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 8/9] fsdev: Use ThrottleDirection instread of bool is_write
2023-07-24 10:09 ` [PATCH v4 8/9] fsdev: Use ThrottleDirection instread of bool is_write zhenwei pi
@ 2023-07-27 15:48 ` Hanna Czenczek
0 siblings, 0 replies; 15+ messages in thread
From: Hanna Czenczek @ 2023-07-27 15:48 UTC (permalink / raw)
To: zhenwei pi, berto, kwolf, groug, qemu_oss
Cc: qemu-devel, qemu-block, berrange
On 24.07.23 12:09, zhenwei pi wrote:
> 'bool is_write' style is obsolete from throttle framework, adapt
> fsdev to the new style.
>
> Cc: Greg Kurz <groug@kaod.org>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
> fsdev/qemu-fsdev-throttle.c | 14 +++++++-------
> fsdev/qemu-fsdev-throttle.h | 4 ++--
> hw/9pfs/cofile.c | 4 ++--
> 3 files changed, 11 insertions(+), 11 deletions(-)
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 9/9] block/throttle-groups: Use ThrottleDirection instread of bool is_write
2023-07-24 10:09 [PATCH v4 0/9] Misc fixes for throttle zhenwei pi
` (7 preceding siblings ...)
2023-07-24 10:09 ` [PATCH v4 8/9] fsdev: Use ThrottleDirection instread of bool is_write zhenwei pi
@ 2023-07-24 10:09 ` zhenwei pi
2023-07-27 16:12 ` Hanna Czenczek
8 siblings, 1 reply; 15+ messages in thread
From: zhenwei pi @ 2023-07-24 10:09 UTC (permalink / raw)
To: berto, kwolf, groug, qemu_oss, hreitz
Cc: qemu-devel, qemu-block, berrange, zhenwei pi
'bool is_write' style is obsolete from throttle framework, adapt
block throttle groups to the new style.
Use a simple python script to test the new style:
#!/usr/bin/python3
import subprocess
import random
import time
commands = ['virsh blkdeviotune jammy vda --write-bytes-sec ', \
'virsh blkdeviotune jammy vda --write-iops-sec ', \
'virsh blkdeviotune jammy vda --read-bytes-sec ', \
'virsh blkdeviotune jammy vda --read-iops-sec ']
for loop in range(1, 1000):
time.sleep(random.randrange(3, 5))
command = commands[random.randrange(0, 3)] + str(random.randrange(0, 1000000))
subprocess.run(command, shell=True, check=True)
This works fine.
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
block/throttle-groups.c | 105 ++++++++++++++++----------------
block/throttle.c | 8 +--
include/block/throttle-groups.h | 6 +-
3 files changed, 60 insertions(+), 59 deletions(-)
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 3847d27801..c7c700fdb6 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -37,7 +37,7 @@
static void throttle_group_obj_init(Object *obj);
static void throttle_group_obj_complete(UserCreatable *obj, Error **errp);
-static void timer_cb(ThrottleGroupMember *tgm, bool is_write);
+static void timer_cb(ThrottleGroupMember *tgm, ThrottleDirection direction);
/* The ThrottleGroup structure (with its ThrottleState) is shared
* among different ThrottleGroupMembers and it's independent from
@@ -73,8 +73,8 @@ struct ThrottleGroup {
QemuMutex lock; /* This lock protects the following four fields */
ThrottleState ts;
QLIST_HEAD(, ThrottleGroupMember) head;
- ThrottleGroupMember *tokens[2];
- bool any_timer_armed[2];
+ ThrottleGroupMember *tokens[THROTTLE_MAX];
+ bool any_timer_armed[THROTTLE_MAX];
QEMUClockType clock_type;
/* This field is protected by the global QEMU mutex */
@@ -197,13 +197,13 @@ static ThrottleGroupMember *throttle_group_next_tgm(ThrottleGroupMember *tgm)
* This assumes that tg->lock is held.
*
* @tgm: the ThrottleGroupMember
- * @is_write: the type of operation (read/write)
+ * @direction: the ThrottleDirection
* @ret: whether the ThrottleGroupMember has pending requests.
*/
static inline bool tgm_has_pending_reqs(ThrottleGroupMember *tgm,
- bool is_write)
+ ThrottleDirection direction)
{
- return tgm->pending_reqs[is_write];
+ return tgm->pending_reqs[direction];
}
/* Return the next ThrottleGroupMember in the round-robin sequence with pending
@@ -212,12 +212,12 @@ static inline bool tgm_has_pending_reqs(ThrottleGroupMember *tgm,
* This assumes that tg->lock is held.
*
* @tgm: the current ThrottleGroupMember
- * @is_write: the type of operation (read/write)
+ * @direction: the ThrottleDirection
* @ret: the next ThrottleGroupMember with pending requests, or tgm if
* there is none.
*/
static ThrottleGroupMember *next_throttle_token(ThrottleGroupMember *tgm,
- bool is_write)
+ ThrottleDirection direction)
{
ThrottleState *ts = tgm->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
@@ -227,16 +227,16 @@ static ThrottleGroupMember *next_throttle_token(ThrottleGroupMember *tgm,
* it's being drained. Skip the round-robin search and return tgm
* immediately if it has pending requests. Otherwise we could be
* forcing it to wait for other member's throttled requests. */
- if (tgm_has_pending_reqs(tgm, is_write) &&
+ if (tgm_has_pending_reqs(tgm, direction) &&
qatomic_read(&tgm->io_limits_disabled)) {
return tgm;
}
- start = token = tg->tokens[is_write];
+ start = token = tg->tokens[direction];
/* get next bs round in round robin style */
token = throttle_group_next_tgm(token);
- while (token != start && !tgm_has_pending_reqs(token, is_write)) {
+ while (token != start && !tgm_has_pending_reqs(token, direction)) {
token = throttle_group_next_tgm(token);
}
@@ -244,12 +244,12 @@ static ThrottleGroupMember *next_throttle_token(ThrottleGroupMember *tgm,
* then decide the token is the current tgm because chances are
* the current tgm got the current request queued.
*/
- if (token == start && !tgm_has_pending_reqs(token, is_write)) {
+ if (token == start && !tgm_has_pending_reqs(token, direction)) {
token = tgm;
}
/* Either we return the original TGM, or one with pending requests */
- assert(token == tgm || tgm_has_pending_reqs(token, is_write));
+ assert(token == tgm || tgm_has_pending_reqs(token, direction));
return token;
}
@@ -261,16 +261,15 @@ static ThrottleGroupMember *next_throttle_token(ThrottleGroupMember *tgm,
* This assumes that tg->lock is held.
*
* @tgm: the current ThrottleGroupMember
- * @is_write: the type of operation (read/write)
+ * @direction: the ThrottleDirection
* @ret: whether the I/O request needs to be throttled or not
*/
static bool throttle_group_schedule_timer(ThrottleGroupMember *tgm,
- bool is_write)
+ ThrottleDirection direction)
{
ThrottleState *ts = tgm->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
ThrottleTimers *tt = &tgm->throttle_timers;
- ThrottleDirection direction = is_write ? THROTTLE_WRITE : THROTTLE_READ;
bool must_wait;
if (qatomic_read(&tgm->io_limits_disabled)) {
@@ -278,7 +277,7 @@ static bool throttle_group_schedule_timer(ThrottleGroupMember *tgm,
}
/* Check if any of the timers in this group is already armed */
- if (tg->any_timer_armed[is_write]) {
+ if (tg->any_timer_armed[direction]) {
return true;
}
@@ -286,8 +285,8 @@ static bool throttle_group_schedule_timer(ThrottleGroupMember *tgm,
/* If a timer just got armed, set tgm as the current token */
if (must_wait) {
- tg->tokens[is_write] = tgm;
- tg->any_timer_armed[is_write] = true;
+ tg->tokens[direction] = tgm;
+ tg->any_timer_armed[direction] = true;
}
return must_wait;
@@ -297,15 +296,15 @@ static bool throttle_group_schedule_timer(ThrottleGroupMember *tgm,
* any request was actually pending.
*
* @tgm: the current ThrottleGroupMember
- * @is_write: the type of operation (read/write)
+ * @direction: the ThrottleDirection
*/
static bool coroutine_fn throttle_group_co_restart_queue(ThrottleGroupMember *tgm,
- bool is_write)
+ ThrottleDirection direction)
{
bool ret;
qemu_co_mutex_lock(&tgm->throttled_reqs_lock);
- ret = qemu_co_queue_next(&tgm->throttled_reqs[is_write]);
+ ret = qemu_co_queue_next(&tgm->throttled_reqs[direction]);
qemu_co_mutex_unlock(&tgm->throttled_reqs_lock);
return ret;
@@ -316,9 +315,10 @@ static bool coroutine_fn throttle_group_co_restart_queue(ThrottleGroupMember *tg
* This assumes that tg->lock is held.
*
* @tgm: the current ThrottleGroupMember
- * @is_write: the type of operation (read/write)
+ * @direction: the ThrottleDirection
*/
-static void schedule_next_request(ThrottleGroupMember *tgm, bool is_write)
+static void schedule_next_request(ThrottleGroupMember *tgm,
+ ThrottleDirection direction)
{
ThrottleState *ts = tgm->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
@@ -326,27 +326,27 @@ static void schedule_next_request(ThrottleGroupMember *tgm, bool is_write)
ThrottleGroupMember *token;
/* Check if there's any pending request to schedule next */
- token = next_throttle_token(tgm, is_write);
- if (!tgm_has_pending_reqs(token, is_write)) {
+ token = next_throttle_token(tgm, direction);
+ if (!tgm_has_pending_reqs(token, direction)) {
return;
}
/* Set a timer for the request if it needs to be throttled */
- must_wait = throttle_group_schedule_timer(token, is_write);
+ must_wait = throttle_group_schedule_timer(token, direction);
/* If it doesn't have to wait, queue it for immediate execution */
if (!must_wait) {
/* Give preference to requests from the current tgm */
if (qemu_in_coroutine() &&
- throttle_group_co_restart_queue(tgm, is_write)) {
+ throttle_group_co_restart_queue(tgm, direction)) {
token = tgm;
} else {
ThrottleTimers *tt = &token->throttle_timers;
int64_t now = qemu_clock_get_ns(tg->clock_type);
- timer_mod(tt->timers[is_write], now);
- tg->any_timer_armed[is_write] = true;
+ timer_mod(tt->timers[direction], now);
+ tg->any_timer_armed[direction] = true;
}
- tg->tokens[is_write] = token;
+ tg->tokens[direction] = token;
}
}
@@ -356,49 +356,49 @@ static void schedule_next_request(ThrottleGroupMember *tgm, bool is_write)
*
* @tgm: the current ThrottleGroupMember
* @bytes: the number of bytes for this I/O
- * @is_write: the type of operation (read/write)
+ * @direction: the ThrottleDirection
*/
void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm,
int64_t bytes,
- bool is_write)
+ ThrottleDirection direction)
{
bool must_wait;
ThrottleGroupMember *token;
ThrottleGroup *tg = container_of(tgm->throttle_state, ThrottleGroup, ts);
- ThrottleDirection direction = is_write ? THROTTLE_WRITE : THROTTLE_READ;
assert(bytes >= 0);
+ assert(direction < THROTTLE_MAX);
qemu_mutex_lock(&tg->lock);
/* First we check if this I/O has to be throttled. */
- token = next_throttle_token(tgm, is_write);
- must_wait = throttle_group_schedule_timer(token, is_write);
+ token = next_throttle_token(tgm, direction);
+ must_wait = throttle_group_schedule_timer(token, direction);
/* Wait if there's a timer set or queued requests of this type */
- if (must_wait || tgm->pending_reqs[is_write]) {
- tgm->pending_reqs[is_write]++;
+ if (must_wait || tgm->pending_reqs[direction]) {
+ tgm->pending_reqs[direction]++;
qemu_mutex_unlock(&tg->lock);
qemu_co_mutex_lock(&tgm->throttled_reqs_lock);
- qemu_co_queue_wait(&tgm->throttled_reqs[is_write],
+ qemu_co_queue_wait(&tgm->throttled_reqs[direction],
&tgm->throttled_reqs_lock);
qemu_co_mutex_unlock(&tgm->throttled_reqs_lock);
qemu_mutex_lock(&tg->lock);
- tgm->pending_reqs[is_write]--;
+ tgm->pending_reqs[direction]--;
}
/* The I/O will be executed, so do the accounting */
throttle_account(tgm->throttle_state, direction, bytes);
/* Schedule the next request */
- schedule_next_request(tgm, is_write);
+ schedule_next_request(tgm, direction);
qemu_mutex_unlock(&tg->lock);
}
typedef struct {
ThrottleGroupMember *tgm;
- bool is_write;
+ ThrottleDirection direction;
} RestartData;
static void coroutine_fn throttle_group_restart_queue_entry(void *opaque)
@@ -407,16 +407,16 @@ static void coroutine_fn throttle_group_restart_queue_entry(void *opaque)
ThrottleGroupMember *tgm = data->tgm;
ThrottleState *ts = tgm->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
- bool is_write = data->is_write;
+ ThrottleDirection direction = data->direction;
bool empty_queue;
- empty_queue = !throttle_group_co_restart_queue(tgm, is_write);
+ empty_queue = !throttle_group_co_restart_queue(tgm, direction);
/* If the request queue was empty then we have to take care of
* scheduling the next one */
if (empty_queue) {
qemu_mutex_lock(&tg->lock);
- schedule_next_request(tgm, is_write);
+ schedule_next_request(tgm, direction);
qemu_mutex_unlock(&tg->lock);
}
@@ -426,18 +426,19 @@ static void coroutine_fn throttle_group_restart_queue_entry(void *opaque)
aio_wait_kick();
}
-static void throttle_group_restart_queue(ThrottleGroupMember *tgm, bool is_write)
+static void throttle_group_restart_queue(ThrottleGroupMember *tgm,
+ ThrottleDirection direction)
{
Coroutine *co;
RestartData *rd = g_new0(RestartData, 1);
rd->tgm = tgm;
- rd->is_write = is_write;
+ rd->direction = direction;
/* This function is called when a timer is fired or when
* throttle_group_restart_tgm() is called. Either way, there can
* be no timer pending on this tgm at this point */
- assert(!timer_pending(tgm->throttle_timers.timers[is_write]));
+ assert(!timer_pending(tgm->throttle_timers.timers[direction]));
qatomic_inc(&tgm->restart_pending);
@@ -502,20 +503,20 @@ void throttle_group_get_config(ThrottleGroupMember *tgm, ThrottleConfig *cfg)
* because it had been throttled.
*
* @tgm: the ThrottleGroupMember whose request had been throttled
- * @is_write: the type of operation (read/write)
+ * @direction: the ThrottleDirection
*/
-static void timer_cb(ThrottleGroupMember *tgm, bool is_write)
+static void timer_cb(ThrottleGroupMember *tgm, ThrottleDirection direction)
{
ThrottleState *ts = tgm->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
/* The timer has just been fired, so we can update the flag */
qemu_mutex_lock(&tg->lock);
- tg->any_timer_armed[is_write] = false;
+ tg->any_timer_armed[direction] = false;
qemu_mutex_unlock(&tg->lock);
/* Run the request that was waiting for this timer */
- throttle_group_restart_queue(tgm, is_write);
+ throttle_group_restart_queue(tgm, direction);
}
static void read_timer_cb(void *opaque)
diff --git a/block/throttle.c b/block/throttle.c
index 3aaef18d4e..1098a4ae9a 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -118,7 +118,7 @@ throttle_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
{
ThrottleGroupMember *tgm = bs->opaque;
- throttle_group_co_io_limits_intercept(tgm, bytes, false);
+ throttle_group_co_io_limits_intercept(tgm, bytes, THROTTLE_READ);
return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
}
@@ -128,7 +128,7 @@ throttle_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes,
QEMUIOVector *qiov, BdrvRequestFlags flags)
{
ThrottleGroupMember *tgm = bs->opaque;
- throttle_group_co_io_limits_intercept(tgm, bytes, true);
+ throttle_group_co_io_limits_intercept(tgm, bytes, THROTTLE_WRITE);
return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
}
@@ -138,7 +138,7 @@ throttle_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
BdrvRequestFlags flags)
{
ThrottleGroupMember *tgm = bs->opaque;
- throttle_group_co_io_limits_intercept(tgm, bytes, true);
+ throttle_group_co_io_limits_intercept(tgm, bytes, THROTTLE_WRITE);
return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
}
@@ -147,7 +147,7 @@ static int coroutine_fn GRAPH_RDLOCK
throttle_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
{
ThrottleGroupMember *tgm = bs->opaque;
- throttle_group_co_io_limits_intercept(tgm, bytes, true);
+ throttle_group_co_io_limits_intercept(tgm, bytes, THROTTLE_WRITE);
return bdrv_co_pdiscard(bs->file, offset, bytes);
}
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index ff282fc0f8..2355e8d9de 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -37,7 +37,7 @@ typedef struct ThrottleGroupMember {
AioContext *aio_context;
/* throttled_reqs_lock protects the CoQueues for throttled requests. */
CoMutex throttled_reqs_lock;
- CoQueue throttled_reqs[2];
+ CoQueue throttled_reqs[THROTTLE_MAX];
/* Nonzero if the I/O limits are currently being ignored; generally
* it is zero. Accessed with atomic operations.
@@ -54,7 +54,7 @@ typedef struct ThrottleGroupMember {
* throttle_state tells us if I/O limits are configured. */
ThrottleState *throttle_state;
ThrottleTimers throttle_timers;
- unsigned pending_reqs[2];
+ unsigned pending_reqs[THROTTLE_MAX];
QLIST_ENTRY(ThrottleGroupMember) round_robin;
} ThrottleGroupMember;
@@ -78,7 +78,7 @@ void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm,
int64_t bytes,
- bool is_write);
+ ThrottleDirection direction);
void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
AioContext *new_context);
void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 9/9] block/throttle-groups: Use ThrottleDirection instread of bool is_write
2023-07-24 10:09 ` [PATCH v4 9/9] block/throttle-groups: " zhenwei pi
@ 2023-07-27 16:12 ` Hanna Czenczek
2023-07-28 2:19 ` zhenwei pi
0 siblings, 1 reply; 15+ messages in thread
From: Hanna Czenczek @ 2023-07-27 16:12 UTC (permalink / raw)
To: zhenwei pi, berto, kwolf, groug, qemu_oss
Cc: qemu-devel, qemu-block, berrange
On 24.07.23 12:09, zhenwei pi wrote:
> 'bool is_write' style is obsolete from throttle framework, adapt
> block throttle groups to the new style.
>
> Use a simple python script to test the new style:
> #!/usr/bin/python3
> import subprocess
> import random
> import time
>
> commands = ['virsh blkdeviotune jammy vda --write-bytes-sec ', \
> 'virsh blkdeviotune jammy vda --write-iops-sec ', \
> 'virsh blkdeviotune jammy vda --read-bytes-sec ', \
> 'virsh blkdeviotune jammy vda --read-iops-sec ']
>
> for loop in range(1, 1000):
> time.sleep(random.randrange(3, 5))
> command = commands[random.randrange(0, 3)] + str(random.randrange(0, 1000000))
> subprocess.run(command, shell=True, check=True)
>
> This works fine.
>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
> block/throttle-groups.c | 105 ++++++++++++++++----------------
> block/throttle.c | 8 +--
> include/block/throttle-groups.h | 6 +-
> 3 files changed, 60 insertions(+), 59 deletions(-)
>
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index 3847d27801..c7c700fdb6 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
Thanks a lot! The changes done look good, and I think they’re nice.
There are some functions left untouched, though, which I think should
use ThrottleDirection, too:
throttle_group_register_tgm() should use THROTTLE_READ/THROTTLE_WRITE to
index tgm->throttled_reqs[] (instead of 0/1). It also has a `for (i =
0; i < 2; i++)` loop over tg->tokens[], which probably should use
THROTTLE_MAX as the upper limit, or iterate over a ThrottleDirection
variable altogether (as you’ve done it in patch 3 e.g. for
throttle_timers_attach_aio_context()).
throttle_group_unregister_tgm() has such a loop, too (over
tgm->pending_reqs[], tgm->throttled_reqs[],
tgm->throttle_timers.timers[], and tg->tokens[], all of which are arrays
over ThrottleDirection).
throttle_group_detach_aio_context() also has both the indexing “problem”
(integers instead of THROTTLE_* for tgm->pending_reqs[] and
tgm->throttled_reqs[]) and such a loop. There, the loop probably really
should be over a ThrottleDirection variable, because it passes that
variable to schedule_next_request().
throttle_group_restart_tgm() also has such a loop, it uses that variable
to index tgm->throttle_timers.timers[], and passes it to both timer_cb()
and throttle_group_restart_queue().
read_timer_cb() and write_timer_cb() should call timer_cb() with
THROTTLE_READ/THROTTLE_WRITE instead of false/true.
> diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
> index ff282fc0f8..2355e8d9de 100644
> --- a/include/block/throttle-groups.h
> +++ b/include/block/throttle-groups.h
[...]
> @@ -78,7 +78,7 @@ void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
>
> void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm,
> int64_t bytes,
> - bool is_write);
> + ThrottleDirection direction);
block/block-backend.c calls this function in
blk_co_do_p{read,write}v_part(), those calls need to be adjusted, too.
> void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
> AioContext *new_context);
> void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: Re: [PATCH v4 9/9] block/throttle-groups: Use ThrottleDirection instread of bool is_write
2023-07-27 16:12 ` Hanna Czenczek
@ 2023-07-28 2:19 ` zhenwei pi
0 siblings, 0 replies; 15+ messages in thread
From: zhenwei pi @ 2023-07-28 2:19 UTC (permalink / raw)
To: Hanna Czenczek
Cc: qemu-devel, berto, qemu_oss, groug, kwolf, qemu-block, berrange
On 7/28/23 00:12, Hanna Czenczek wrote:
> On 24.07.23 12:09, zhenwei pi wrote:
>> 'bool is_write' style is obsolete from throttle framework, adapt
>> block throttle groups to the new style.
>>
>> Use a simple python script to test the new style:
>> #!/usr/bin/python3
>> import subprocess
>> import random
>> import time
>>
>> commands = ['virsh blkdeviotune jammy vda --write-bytes-sec ', \
>> 'virsh blkdeviotune jammy vda --write-iops-sec ', \
>> 'virsh blkdeviotune jammy vda --read-bytes-sec ', \
>> 'virsh blkdeviotune jammy vda --read-iops-sec ']
>>
>> for loop in range(1, 1000):
>> time.sleep(random.randrange(3, 5))
>> command = commands[random.randrange(0, 3)] +
>> str(random.randrange(0, 1000000))
>> subprocess.run(command, shell=True, check=True)
>>
>> This works fine.
>>
>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>> ---
>> block/throttle-groups.c | 105 ++++++++++++++++----------------
>> block/throttle.c | 8 +--
>> include/block/throttle-groups.h | 6 +-
>> 3 files changed, 60 insertions(+), 59 deletions(-)
>>
>> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
>> index 3847d27801..c7c700fdb6 100644
>> --- a/block/throttle-groups.c
>> +++ b/block/throttle-groups.c
>
> Thanks a lot! The changes done look good, and I think they’re nice.
>
> There are some functions left untouched, though, which I think should
> use ThrottleDirection, too:
>
> throttle_group_register_tgm() should use THROTTLE_READ/THROTTLE_WRITE to
> index tgm->throttled_reqs[] (instead of 0/1). It also has a `for (i =
> 0; i < 2; i++)` loop over tg->tokens[], which probably should use
> THROTTLE_MAX as the upper limit, or iterate over a ThrottleDirection
> variable altogether (as you’ve done it in patch 3 e.g. for
> throttle_timers_attach_aio_context()).
>
> throttle_group_unregister_tgm() has such a loop, too (over
> tgm->pending_reqs[], tgm->throttled_reqs[],
> tgm->throttle_timers.timers[], and tg->tokens[], all of which are arrays
> over ThrottleDirection).
>
> throttle_group_detach_aio_context() also has both the indexing “problem”
> (integers instead of THROTTLE_* for tgm->pending_reqs[] and
> tgm->throttled_reqs[]) and such a loop. There, the loop probably really
> should be over a ThrottleDirection variable, because it passes that
> variable to schedule_next_request().
>
> throttle_group_restart_tgm() also has such a loop, it uses that variable
> to index tgm->throttle_timers.timers[], and passes it to both timer_cb()
> and throttle_group_restart_queue().
>
> read_timer_cb() and write_timer_cb() should call timer_cb() with
> THROTTLE_READ/THROTTLE_WRITE instead of false/true.
>
>> diff --git a/include/block/throttle-groups.h
>> b/include/block/throttle-groups.h
>> index ff282fc0f8..2355e8d9de 100644
>> --- a/include/block/throttle-groups.h
>> +++ b/include/block/throttle-groups.h
>
> [...]
>
>> @@ -78,7 +78,7 @@ void throttle_group_restart_tgm(ThrottleGroupMember
>> *tgm);
>> void coroutine_fn
>> throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm,
>> int64_t bytes,
>> - bool is_write);
>> +
>> ThrottleDirection direction);
>
> block/block-backend.c calls this function in
> blk_co_do_p{read,write}v_part(), those calls need to be adjusted, too.
>
>> void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
>> AioContext *new_context);
>> void throttle_group_detach_aio_context(ThrottleGroupMember *tgm);
>
Sorry, I missed these. Please see the v5 version. Thanks!
--
zhenwei pi
^ permalink raw reply [flat|nested] 15+ messages in thread