* [Qemu-devel] [PATCH 1/7] throttle: Extract timers from ThrottleState into a separate structure
2015-03-30 16:19 [Qemu-devel] [PATCH v6 0/7] Block Throttle Group Support Alberto Garcia
@ 2015-03-30 16:19 ` Alberto Garcia
2015-03-30 16:19 ` [Qemu-devel] [PATCH 2/7] throttle: Add throttle group infrastructure Alberto Garcia
` (5 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Alberto Garcia @ 2015-03-30 16:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Benoît Canet, Stefan Hajnoczi
From: Benoît Canet <benoit.canet@nodalink.com>
Group throttling will share ThrottleState between multiple bs.
As a consequence the ThrottleState will be accessed by multiple aio
context.
Timers are tied to their aio context so they must go out of the
ThrottleState structure.
This commit paves the way for each bs of a common ThrottleState to
have its own timer.
Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block.c | 35 ++++++++++++--------
include/block/block_int.h | 1 +
include/qemu/throttle.h | 38 ++++++++++++++--------
tests/test-throttle.c | 82 ++++++++++++++++++++++++++---------------------
util/throttle.c | 73 ++++++++++++++++++++++++-----------------
5 files changed, 135 insertions(+), 94 deletions(-)
diff --git a/block.c b/block.c
index f2f8ae7..28508ef 100644
--- a/block.c
+++ b/block.c
@@ -130,7 +130,7 @@ void bdrv_set_io_limits(BlockDriverState *bs,
{
int i;
- throttle_config(&bs->throttle_state, cfg);
+ throttle_config(&bs->throttle_state, &bs->throttle_timers, cfg);
for (i = 0; i < 2; i++) {
qemu_co_enter_next(&bs->throttled_reqs[i]);
@@ -163,7 +163,7 @@ void bdrv_io_limits_disable(BlockDriverState *bs)
bdrv_start_throttled_reqs(bs);
- throttle_destroy(&bs->throttle_state);
+ throttle_timers_destroy(&bs->throttle_timers);
}
static void bdrv_throttle_read_timer_cb(void *opaque)
@@ -182,12 +182,13 @@ static void bdrv_throttle_write_timer_cb(void *opaque)
void bdrv_io_limits_enable(BlockDriverState *bs)
{
assert(!bs->io_limits_enabled);
- throttle_init(&bs->throttle_state,
- bdrv_get_aio_context(bs),
- QEMU_CLOCK_VIRTUAL,
- bdrv_throttle_read_timer_cb,
- bdrv_throttle_write_timer_cb,
- bs);
+ throttle_init(&bs->throttle_state);
+ throttle_timers_init(&bs->throttle_timers,
+ bdrv_get_aio_context(bs),
+ QEMU_CLOCK_VIRTUAL,
+ bdrv_throttle_read_timer_cb,
+ bdrv_throttle_write_timer_cb,
+ bs);
bs->io_limits_enabled = true;
}
@@ -201,7 +202,9 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
bool is_write)
{
/* does this io must wait */
- bool must_wait = throttle_schedule_timer(&bs->throttle_state, is_write);
+ bool must_wait = throttle_schedule_timer(&bs->throttle_state,
+ &bs->throttle_timers,
+ is_write);
/* if must wait or any request of this type throttled queue the IO */
if (must_wait ||
@@ -214,7 +217,8 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
/* if the next request must wait -> do nothing */
- if (throttle_schedule_timer(&bs->throttle_state, is_write)) {
+ if (throttle_schedule_timer(&bs->throttle_state, &bs->throttle_timers,
+ is_write)) {
return;
}
@@ -2090,6 +2094,9 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
memcpy(&bs_dest->throttle_state,
&bs_src->throttle_state,
sizeof(ThrottleState));
+ memcpy(&bs_dest->throttle_timers,
+ &bs_src->throttle_timers,
+ sizeof(ThrottleTimers));
bs_dest->throttled_reqs[0] = bs_src->throttled_reqs[0];
bs_dest->throttled_reqs[1] = bs_src->throttled_reqs[1];
bs_dest->io_limits_enabled = bs_src->io_limits_enabled;
@@ -2151,7 +2158,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
assert(bs_new->job == NULL);
assert(bs_new->io_limits_enabled == false);
- assert(!throttle_have_timer(&bs_new->throttle_state));
+ assert(!throttle_timers_are_initialized(&bs_new->throttle_timers));
tmp = *bs_new;
*bs_new = *bs_old;
@@ -2168,7 +2175,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
/* Check a few fields that should remain attached to the device */
assert(bs_new->job == NULL);
assert(bs_new->io_limits_enabled == false);
- assert(!throttle_have_timer(&bs_new->throttle_state));
+ assert(!throttle_timers_are_initialized(&bs_new->throttle_timers));
/* insert the nodes back into the graph node list if needed */
if (bs_new->node_name[0] != '\0') {
@@ -5858,7 +5865,7 @@ void bdrv_detach_aio_context(BlockDriverState *bs)
}
if (bs->io_limits_enabled) {
- throttle_detach_aio_context(&bs->throttle_state);
+ throttle_timers_detach_aio_context(&bs->throttle_timers);
}
if (bs->drv->bdrv_detach_aio_context) {
bs->drv->bdrv_detach_aio_context(bs);
@@ -5894,7 +5901,7 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
bs->drv->bdrv_attach_aio_context(bs, new_context);
}
if (bs->io_limits_enabled) {
- throttle_attach_aio_context(&bs->throttle_state, new_context);
+ throttle_timers_attach_aio_context(&bs->throttle_timers, new_context);
}
QLIST_FOREACH(ban, &bs->aio_notifiers, list) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dccb092..c88a062 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -377,6 +377,7 @@ struct BlockDriverState {
/* I/O throttling */
ThrottleState throttle_state;
+ ThrottleTimers throttle_timers;
CoQueue throttled_reqs[2];
bool io_limits_enabled;
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index b890613..2c560db 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -65,14 +65,17 @@ typedef struct ThrottleConfig {
typedef struct ThrottleState {
ThrottleConfig cfg; /* configuration */
int64_t previous_leak; /* timestamp of the last leak done */
- QEMUTimer * timers[2]; /* timers used to do the throttling */
+} ThrottleState;
+
+typedef struct ThrottleTimers {
+ QEMUTimer *timers[2]; /* timers used to do the throttling */
QEMUClockType clock_type; /* the clock used */
/* Callbacks */
QEMUTimerCB *read_timer_cb;
QEMUTimerCB *write_timer_cb;
void *timer_opaque;
-} ThrottleState;
+} ThrottleTimers;
/* operations on single leaky buckets */
void throttle_leak_bucket(LeakyBucket *bkt, int64_t delta);
@@ -86,20 +89,23 @@ bool throttle_compute_timer(ThrottleState *ts,
int64_t *next_timestamp);
/* init/destroy cycle */
-void throttle_init(ThrottleState *ts,
- AioContext *aio_context,
- QEMUClockType clock_type,
- void (read_timer)(void *),
- void (write_timer)(void *),
- void *timer_opaque);
+void throttle_init(ThrottleState *ts);
+
+void throttle_timers_init(ThrottleTimers *tt,
+ AioContext *aio_context,
+ QEMUClockType clock_type,
+ QEMUTimerCB *read_timer_cb,
+ QEMUTimerCB *write_timer_cb,
+ void *timer_opaque);
-void throttle_destroy(ThrottleState *ts);
+void throttle_timers_destroy(ThrottleTimers *tt);
-void throttle_detach_aio_context(ThrottleState *ts);
+void throttle_timers_detach_aio_context(ThrottleTimers *tt);
-void throttle_attach_aio_context(ThrottleState *ts, AioContext *new_context);
+void throttle_timers_attach_aio_context(ThrottleTimers *tt,
+ AioContext *new_context);
-bool throttle_have_timer(ThrottleState *ts);
+bool throttle_timers_are_initialized(ThrottleTimers *tt);
/* configuration */
bool throttle_enabled(ThrottleConfig *cfg);
@@ -108,12 +114,16 @@ bool throttle_conflicting(ThrottleConfig *cfg);
bool throttle_is_valid(ThrottleConfig *cfg);
-void throttle_config(ThrottleState *ts, ThrottleConfig *cfg);
+void throttle_config(ThrottleState *ts,
+ ThrottleTimers *tt,
+ ThrottleConfig *cfg);
void throttle_get_config(ThrottleState *ts, ThrottleConfig *cfg);
/* usage */
-bool throttle_schedule_timer(ThrottleState *ts, bool is_write);
+bool throttle_schedule_timer(ThrottleState *ts,
+ ThrottleTimers *tt,
+ bool is_write);
void throttle_account(ThrottleState *ts, bool is_write, uint64_t size);
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index d8ba415..458f577 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -20,6 +20,7 @@ static AioContext *ctx;
static LeakyBucket bkt;
static ThrottleConfig cfg;
static ThrottleState ts;
+static ThrottleTimers tt;
/* useful function */
static bool double_cmp(double x, double y)
@@ -103,17 +104,19 @@ static void test_init(void)
{
int i;
- /* fill the structure with crap */
+ /* fill the structures with crap */
memset(&ts, 1, sizeof(ts));
+ memset(&tt, 1, sizeof(tt));
- /* init the structure */
- throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
- read_timer_cb, write_timer_cb, &ts);
+ /* init structures */
+ throttle_init(&ts);
+ throttle_timers_init(&tt, ctx, QEMU_CLOCK_VIRTUAL,
+ read_timer_cb, write_timer_cb, &ts);
/* check initialized fields */
- g_assert(ts.clock_type == QEMU_CLOCK_VIRTUAL);
- g_assert(ts.timers[0]);
- g_assert(ts.timers[1]);
+ g_assert(tt.clock_type == QEMU_CLOCK_VIRTUAL);
+ g_assert(tt.timers[0]);
+ g_assert(tt.timers[1]);
/* check other fields where cleared */
g_assert(!ts.previous_leak);
@@ -124,17 +127,18 @@ static void test_init(void)
g_assert(!ts.cfg.buckets[i].level);
}
- throttle_destroy(&ts);
+ throttle_timers_destroy(&tt);
}
static void test_destroy(void)
{
int i;
- throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
- read_timer_cb, write_timer_cb, &ts);
- throttle_destroy(&ts);
+ throttle_init(&ts);
+ 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++) {
- g_assert(!ts.timers[i]);
+ g_assert(!tt.timers[i]);
}
}
@@ -170,11 +174,12 @@ static void test_config_functions(void)
orig_cfg.op_size = 1;
- throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
- read_timer_cb, write_timer_cb, &ts);
+ throttle_init(&ts);
+ throttle_timers_init(&tt, ctx, QEMU_CLOCK_VIRTUAL,
+ read_timer_cb, write_timer_cb, &ts);
/* structure reset by throttle_init previous_leak should be null */
g_assert(!ts.previous_leak);
- throttle_config(&ts, &orig_cfg);
+ throttle_config(&ts, &tt, &orig_cfg);
/* has previous leak been initialized by throttle_config ? */
g_assert(ts.previous_leak);
@@ -182,7 +187,7 @@ static void test_config_functions(void)
/* get back the fixed configuration */
throttle_get_config(&ts, &final_cfg);
- throttle_destroy(&ts);
+ throttle_timers_destroy(&tt);
g_assert(final_cfg.buckets[THROTTLE_BPS_TOTAL].avg == 153);
g_assert(final_cfg.buckets[THROTTLE_BPS_READ].avg == 56);
@@ -323,43 +328,47 @@ static void test_is_valid(void)
static void test_have_timer(void)
{
- /* zero the structure */
+ /* zero structures */
memset(&ts, 0, sizeof(ts));
+ memset(&tt, 0, sizeof(tt));
/* no timer set should return false */
- g_assert(!throttle_have_timer(&ts));
+ g_assert(!throttle_timers_are_initialized(&tt));
- /* init the structure */
- throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
- read_timer_cb, write_timer_cb, &ts);
+ /* init structures */
+ throttle_init(&ts);
+ throttle_timers_init(&tt, ctx, QEMU_CLOCK_VIRTUAL,
+ read_timer_cb, write_timer_cb, &ts);
/* timer set by init should return true */
- g_assert(throttle_have_timer(&ts));
+ g_assert(throttle_timers_are_initialized(&tt));
- throttle_destroy(&ts);
+ throttle_timers_destroy(&tt);
}
static void test_detach_attach(void)
{
- /* zero the structure */
+ /* zero structures */
memset(&ts, 0, sizeof(ts));
+ memset(&tt, 0, sizeof(tt));
/* init the structure */
- throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
- read_timer_cb, write_timer_cb, &ts);
+ throttle_init(&ts);
+ throttle_timers_init(&tt, ctx, QEMU_CLOCK_VIRTUAL,
+ read_timer_cb, write_timer_cb, &ts);
/* timer set by init should return true */
- g_assert(throttle_have_timer(&ts));
+ g_assert(throttle_timers_are_initialized(&tt));
/* timer should no longer exist after detaching */
- throttle_detach_aio_context(&ts);
- g_assert(!throttle_have_timer(&ts));
+ throttle_timers_detach_aio_context(&tt);
+ g_assert(!throttle_timers_are_initialized(&tt));
/* timer should exist again after attaching */
- throttle_attach_aio_context(&ts, ctx);
- g_assert(throttle_have_timer(&ts));
+ throttle_timers_attach_aio_context(&tt, ctx);
+ g_assert(throttle_timers_are_initialized(&tt));
- throttle_destroy(&ts);
+ throttle_timers_destroy(&tt);
}
static bool do_test_accounting(bool is_ops, /* are we testing bps or ops */
@@ -387,9 +396,10 @@ static bool do_test_accounting(bool is_ops, /* are we testing bps or ops */
cfg.op_size = op_size;
- throttle_init(&ts, ctx, QEMU_CLOCK_VIRTUAL,
- read_timer_cb, write_timer_cb, &ts);
- throttle_config(&ts, &cfg);
+ throttle_init(&ts);
+ throttle_timers_init(&tt, ctx, QEMU_CLOCK_VIRTUAL,
+ read_timer_cb, write_timer_cb, &ts);
+ throttle_config(&ts, &tt, &cfg);
/* account a read */
throttle_account(&ts, false, size);
@@ -414,7 +424,7 @@ static bool do_test_accounting(bool is_ops, /* are we testing bps or ops */
return false;
}
- throttle_destroy(&ts);
+ throttle_timers_destroy(&tt);
return true;
}
diff --git a/util/throttle.c b/util/throttle.c
index f976ac7..d76a48e 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -159,29 +159,36 @@ bool throttle_compute_timer(ThrottleState *ts,
}
/* Add timers to event loop */
-void throttle_attach_aio_context(ThrottleState *ts, AioContext *new_context)
+void throttle_timers_attach_aio_context(ThrottleTimers *tt,
+ AioContext *new_context)
{
- ts->timers[0] = aio_timer_new(new_context, ts->clock_type, SCALE_NS,
- ts->read_timer_cb, ts->timer_opaque);
- ts->timers[1] = aio_timer_new(new_context, ts->clock_type, SCALE_NS,
- ts->write_timer_cb, ts->timer_opaque);
+ 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);
}
/* To be called first on the ThrottleState */
-void throttle_init(ThrottleState *ts,
- AioContext *aio_context,
- QEMUClockType clock_type,
- QEMUTimerCB *read_timer_cb,
- QEMUTimerCB *write_timer_cb,
- void *timer_opaque)
+void throttle_init(ThrottleState *ts)
{
memset(ts, 0, sizeof(ThrottleState));
+}
+
+/* To be called first on the ThrottleTimers */
+void throttle_timers_init(ThrottleTimers *tt,
+ AioContext *aio_context,
+ QEMUClockType clock_type,
+ QEMUTimerCB *read_timer_cb,
+ QEMUTimerCB *write_timer_cb,
+ void *timer_opaque)
+{
+ memset(tt, 0, sizeof(ThrottleTimers));
- ts->clock_type = clock_type;
- ts->read_timer_cb = read_timer_cb;
- ts->write_timer_cb = write_timer_cb;
- ts->timer_opaque = timer_opaque;
- throttle_attach_aio_context(ts, aio_context);
+ tt->clock_type = clock_type;
+ tt->read_timer_cb = read_timer_cb;
+ tt->write_timer_cb = write_timer_cb;
+ tt->timer_opaque = timer_opaque;
+ throttle_timers_attach_aio_context(tt, aio_context);
}
/* destroy a timer */
@@ -195,25 +202,25 @@ static void throttle_timer_destroy(QEMUTimer **timer)
}
/* Remove timers from event loop */
-void throttle_detach_aio_context(ThrottleState *ts)
+void throttle_timers_detach_aio_context(ThrottleTimers *tt)
{
int i;
for (i = 0; i < 2; i++) {
- throttle_timer_destroy(&ts->timers[i]);
+ throttle_timer_destroy(&tt->timers[i]);
}
}
-/* To be called last on the ThrottleState */
-void throttle_destroy(ThrottleState *ts)
+/* To be called last on the ThrottleTimers */
+void throttle_timers_destroy(ThrottleTimers *tt)
{
- throttle_detach_aio_context(ts);
+ throttle_timers_detach_aio_context(tt);
}
/* is any throttling timer configured */
-bool throttle_have_timer(ThrottleState *ts)
+bool throttle_timers_are_initialized(ThrottleTimers *tt)
{
- if (ts->timers[0]) {
+ if (tt->timers[0]) {
return true;
}
@@ -324,9 +331,12 @@ static void throttle_cancel_timer(QEMUTimer *timer)
/* Used to configure the throttle
*
* @ts: the throttle state we are working on
+ * @tt: the throttle timers we use in this aio context
* @cfg: the config to set
*/
-void throttle_config(ThrottleState *ts, ThrottleConfig *cfg)
+void throttle_config(ThrottleState *ts,
+ ThrottleTimers *tt,
+ ThrottleConfig *cfg)
{
int i;
@@ -336,10 +346,10 @@ void throttle_config(ThrottleState *ts, ThrottleConfig *cfg)
throttle_fix_bucket(&ts->cfg.buckets[i]);
}
- ts->previous_leak = qemu_clock_get_ns(ts->clock_type);
+ ts->previous_leak = qemu_clock_get_ns(tt->clock_type);
for (i = 0; i < 2; i++) {
- throttle_cancel_timer(ts->timers[i]);
+ throttle_cancel_timer(tt->timers[i]);
}
}
@@ -358,12 +368,15 @@ 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)
* @ret: true if the timer has been scheduled else false
*/
-bool throttle_schedule_timer(ThrottleState *ts, bool is_write)
+bool throttle_schedule_timer(ThrottleState *ts,
+ ThrottleTimers *tt,
+ bool is_write)
{
- int64_t now = qemu_clock_get_ns(ts->clock_type);
+ int64_t now = qemu_clock_get_ns(tt->clock_type);
int64_t next_timestamp;
bool must_wait;
@@ -378,12 +391,12 @@ bool throttle_schedule_timer(ThrottleState *ts, bool is_write)
}
/* request throttled and timer pending -> do nothing */
- if (timer_pending(ts->timers[is_write])) {
+ if (timer_pending(tt->timers[is_write])) {
return true;
}
/* request throttled and timer not pending -> arm timer */
- timer_mod(ts->timers[is_write], next_timestamp);
+ timer_mod(tt->timers[is_write], next_timestamp);
return true;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH 2/7] throttle: Add throttle group infrastructure
2015-03-30 16:19 [Qemu-devel] [PATCH v6 0/7] Block Throttle Group Support Alberto Garcia
2015-03-30 16:19 ` [Qemu-devel] [PATCH 1/7] throttle: Extract timers from ThrottleState into a separate structure Alberto Garcia
@ 2015-03-30 16:19 ` Alberto Garcia
2015-04-09 12:28 ` Stefan Hajnoczi
2015-03-30 16:19 ` [Qemu-devel] [PATCH 3/7] throttle: Add throttle group infrastructure tests Alberto Garcia
` (4 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Alberto Garcia @ 2015-03-30 16:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Stefan Hajnoczi
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/Makefile.objs | 1 +
block/throttle-groups.c | 261 ++++++++++++++++++++++++++++++++++++++++
include/block/block_int.h | 1 +
include/block/throttle-groups.h | 39 ++++++
4 files changed, 302 insertions(+)
create mode 100644 block/throttle-groups.c
create mode 100644 include/block/throttle-groups.h
diff --git a/block/Makefile.objs b/block/Makefile.objs
index db2933e..1e61ce0 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -10,6 +10,7 @@ block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
block-obj-$(CONFIG_POSIX) += raw-posix.o
block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
block-obj-y += null.o mirror.o
+block-obj-y += throttle-groups.o
block-obj-y += nbd.o nbd-client.o sheepdog.o
block-obj-$(CONFIG_LIBISCSI) += iscsi.o
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
new file mode 100644
index 0000000..352077f
--- /dev/null
+++ b/block/throttle-groups.c
@@ -0,0 +1,261 @@
+/*
+ * QEMU block throttling group infrastructure
+ *
+ * Copyright (C) Nodalink, EURL. 2014
+ * Copyright (C) Igalia, S.L. 2015
+ *
+ * Authors:
+ * Benoît Canet <benoit.canet@nodalink.com>
+ * Alberto Garcia <berto@igalia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "block/throttle-groups.h"
+
+/* The ThrottleGroup structure (with its ThrottleState) is shared
+ * among different BlockDriverState and it's independent from
+ * AioContext, so in order to use it from different threads it needs
+ * its own locking.
+ *
+ * This locking is however handled internally in this file, so it's
+ * transparent to outside users.
+ *
+ * The whole ThrottleGroup structure is private and invisible to
+ * outside users, that only use it through its ThrottleState.
+ *
+ * In addition to the ThrottleGroup structure, BlockDriverState has
+ * fields that need to be accessed by other members of the group and
+ * therefore also need to be protected by this lock. Once a BDS is
+ * registered in a group those fields can be accessed by other threads
+ * any time.
+ *
+ * Again, all this is handled internally and is mostly transparent to
+ * the outside. The 'throttle_timers' field however has an additional
+ * constraint because it may be temporarily invalid (see for example
+ * bdrv_set_aio_context()). Therefore in this file a thread will
+ * access some other BDS's timers only after verifying that that BDS
+ * has throttled requests in the queue.
+ */
+typedef struct ThrottleGroup {
+ char *name; /* This is constant during the lifetime of the group */
+
+ QemuMutex lock; /* This lock protects the following four fields */
+ ThrottleState ts;
+ QLIST_HEAD(, BlockDriverState) head;
+ BlockDriverState *tokens[2];
+ bool any_timer_armed[2];
+
+ /* These two are protected by the global throttle_groups_lock */
+ unsigned refcount;
+ QTAILQ_ENTRY(ThrottleGroup) list;
+} ThrottleGroup;
+
+static QemuMutex throttle_groups_lock;
+static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
+ QTAILQ_HEAD_INITIALIZER(throttle_groups);
+
+/* Increments the reference count of a ThrottleGroup given its name.
+ *
+ * If no ThrottleGroup is found with the given name a new one is
+ * created.
+ *
+ * @name: the name of the ThrottleGroup
+ * @ret: the ThrottleGroup
+ */
+static ThrottleGroup *throttle_group_incref(const char *name)
+{
+ ThrottleGroup *tg = NULL;
+ ThrottleGroup *iter;
+
+ qemu_mutex_lock(&throttle_groups_lock);
+
+ /* Look for an existing group with that name */
+ QTAILQ_FOREACH(iter, &throttle_groups, list) {
+ if (!strcmp(name, iter->name)) {
+ tg = iter;
+ break;
+ }
+ }
+
+ /* Create a new one if not found */
+ if (!tg) {
+ tg = g_new0(ThrottleGroup, 1);
+ tg->name = g_strdup(name);
+ qemu_mutex_init(&tg->lock);
+ throttle_init(&tg->ts);
+ QLIST_INIT(&tg->head);
+
+ QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
+ }
+
+ tg->refcount++;
+
+ qemu_mutex_unlock(&throttle_groups_lock);
+
+ return tg;
+}
+
+/* Decrease the reference count of a ThrottleGroup.
+ *
+ * When the reference count reaches zero the ThrottleGroup is
+ * destroyed.
+ *
+ * @tg: The ThrottleGroup to unref
+ */
+static void throttle_group_unref(ThrottleGroup *tg)
+{
+ qemu_mutex_lock(&throttle_groups_lock);
+ if (--tg->refcount == 0) {
+ QTAILQ_REMOVE(&throttle_groups, tg, list);
+ qemu_mutex_destroy(&tg->lock);
+ g_free(tg->name);
+ g_free(tg);
+ }
+ qemu_mutex_unlock(&throttle_groups_lock);
+}
+
+/* Get the name from a BlockDriverState's ThrottleGroup. The name (and
+ * the pointer) is guaranteed to remain constant during the lifetime
+ * of the group.
+ *
+ * @bs: a BlockDriverState that is member of a throttling group
+ * @ret: the name of the group.
+ */
+const char *throttle_group_get_name(BlockDriverState *bs)
+{
+ ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ return tg->name;
+}
+
+/* Return the next BlockDriverState in the round-robin sequence,
+ * simulating a circular list.
+ *
+ * This assumes that tg->lock is held.
+ *
+ * @bs: the current BlockDriverState
+ * @ret: the next BlockDriverState in the sequence
+ */
+static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs)
+{
+ ThrottleState *ts = bs->throttle_state;
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ BlockDriverState *next = QLIST_NEXT(bs, round_robin);
+
+ if (!next) {
+ return QLIST_FIRST(&tg->head);
+ }
+
+ return next;
+}
+
+/* Update the throttle configuration for a particular group. Similar
+ * to throttle_config(), but guarantees atomicity within the
+ * throttling group.
+ *
+ * @bs: a BlockDriverState that is member of the group
+ * @cfg: the configuration to set
+ */
+void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg)
+{
+ ThrottleTimers *tt = &bs->throttle_timers;
+ ThrottleState *ts = bs->throttle_state;
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ qemu_mutex_lock(&tg->lock);
+ throttle_config(ts, tt, cfg);
+ /* throttle_config() cancels the timers */
+ tg->any_timer_armed[0] = tg->any_timer_armed[1] = false;
+ qemu_mutex_unlock(&tg->lock);
+}
+
+/* Get the throttle configuration from a particular group. Similar to
+ * throttle_get_config(), but guarantees atomicity within the
+ * throttling group.
+ *
+ * @bs: a BlockDriverState that is member of the group
+ * @cfg: the configuration will be written here
+ */
+void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg)
+{
+ ThrottleState *ts = bs->throttle_state;
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ qemu_mutex_lock(&tg->lock);
+ throttle_get_config(ts, cfg);
+ qemu_mutex_unlock(&tg->lock);
+}
+
+/* Register a BlockDriverState in the throttling group, also updating
+ * its throttle_state pointer to point to it. If a throttling group
+ * with that name does not exist yet, it will be created.
+ *
+ * @bs: the BlockDriverState to insert
+ * @groupname: the name of the group
+ */
+void throttle_group_register_bs(BlockDriverState *bs, const char *groupname)
+{
+ int i;
+ ThrottleGroup *tg = throttle_group_incref(groupname);
+
+ bs->throttle_state = &tg->ts;
+
+ qemu_mutex_lock(&tg->lock);
+ /* If the ThrottleGroup is new set this BlockDriverState as the token */
+ for (i = 0; i < 2; i++) {
+ if (!tg->tokens[i]) {
+ tg->tokens[i] = bs;
+ }
+ }
+
+ QLIST_INSERT_HEAD(&tg->head, bs, round_robin);
+ qemu_mutex_unlock(&tg->lock);
+}
+
+/* Unregister a BlockDriverState from its group, removing it from the
+ * list and setting the throttle_state pointer to NULL.
+ *
+ * The group will be destroyed if it's empty after this operation.
+ *
+ * @bs: the BlockDriverState to remove
+ */
+void throttle_group_unregister_bs(BlockDriverState *bs)
+{
+ ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ int i;
+
+ qemu_mutex_lock(&tg->lock);
+ for (i = 0; i < 2; i++) {
+ if (tg->tokens[i] == bs) {
+ BlockDriverState *token = throttle_group_next_bs(bs);
+ /* Take care of the case where this is the last bs in the group */
+ if (token == bs) {
+ token = NULL;
+ }
+ tg->tokens[i] = token;
+ }
+ }
+
+ /* remove the current bs from the list */
+ QLIST_REMOVE(bs, round_robin);
+ qemu_mutex_unlock(&tg->lock);
+
+ throttle_group_unref(tg);
+ bs->throttle_state = NULL;
+}
+
+static void throttle_groups_init(void)
+{
+ qemu_mutex_init(&throttle_groups_lock);
+}
+
+block_init(throttle_groups_init);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c88a062..f0896e9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -380,6 +380,7 @@ struct BlockDriverState {
ThrottleTimers throttle_timers;
CoQueue throttled_reqs[2];
bool io_limits_enabled;
+ QLIST_ENTRY(BlockDriverState) round_robin;
/* I/O stats (display with "info blockstats"). */
BlockAcctStats stats;
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
new file mode 100644
index 0000000..b966ec7
--- /dev/null
+++ b/include/block/throttle-groups.h
@@ -0,0 +1,39 @@
+/*
+ * QEMU block throttling group infrastructure
+ *
+ * Copyright (C) Nodalink, EURL. 2014
+ * Copyright (C) Igalia, S.L. 2015
+ *
+ * Authors:
+ * Benoît Canet <benoit.canet@nodalink.com>
+ * Alberto Garcia <berto@igalia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef THROTTLE_GROUPS_H
+#define THROTTLE_GROUPS_H
+
+#include "qemu/throttle.h"
+#include "block/block_int.h"
+
+const char *throttle_group_get_name(BlockDriverState *bs);
+
+void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg);
+void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg);
+
+void throttle_group_register_bs(BlockDriverState *bs, const char *groupname);
+void throttle_group_unregister_bs(BlockDriverState *bs);
+
+#endif
--
2.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH 3/7] throttle: Add throttle group infrastructure tests
2015-03-30 16:19 [Qemu-devel] [PATCH v6 0/7] Block Throttle Group Support Alberto Garcia
2015-03-30 16:19 ` [Qemu-devel] [PATCH 1/7] throttle: Extract timers from ThrottleState into a separate structure Alberto Garcia
2015-03-30 16:19 ` [Qemu-devel] [PATCH 2/7] throttle: Add throttle group infrastructure Alberto Garcia
@ 2015-03-30 16:19 ` Alberto Garcia
2015-04-01 10:00 ` Fam Zheng
2015-04-09 12:30 ` Stefan Hajnoczi
2015-03-30 16:19 ` [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support Alberto Garcia
` (3 subsequent siblings)
6 siblings, 2 replies; 24+ messages in thread
From: Alberto Garcia @ 2015-03-30 16:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Stefan Hajnoczi
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
tests/test-throttle.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 69 insertions(+), 10 deletions(-)
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 458f577..aa0e297 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -1,10 +1,12 @@
/*
* Throttle infrastructure tests
*
- * Copyright Nodalink, SARL. 2013
+ * Copyright Nodalink, EURL. 2013-2014
+ * Copyright Igalia, S.L. 2015
*
* Authors:
- * Benoît Canet <benoit.canet@irqsave.net>
+ * Benoît Canet <benoit.canet@nodalink.com>
+ * Alberto Garcia <berto@igalia.com>
*
* This work is licensed under the terms of the GNU LGPL, version 2 or later.
* See the COPYING.LIB file in the top-level directory.
@@ -15,6 +17,7 @@
#include "block/aio.h"
#include "qemu/throttle.h"
#include "qemu/error-report.h"
+#include "block/throttle-groups.h"
static AioContext *ctx;
static LeakyBucket bkt;
@@ -500,23 +503,78 @@ static void test_accounting(void)
(64.0 / 13)));
}
+static void test_groups(void)
+{
+ ThrottleConfig cfg1, cfg2;
+ BlockDriverState *bdrv1, *bdrv2, *bdrv3;
+
+ bdrv1 = bdrv_new();
+ bdrv2 = bdrv_new();
+ bdrv3 = bdrv_new();
+
+ g_assert(bdrv1->throttle_state == NULL);
+ g_assert(bdrv2->throttle_state == NULL);
+ g_assert(bdrv3->throttle_state == NULL);
+
+ throttle_group_register_bs(bdrv1, "bar");
+ throttle_group_register_bs(bdrv2, "foo");
+ throttle_group_register_bs(bdrv3, "bar");
+
+ g_assert(bdrv1->throttle_state != NULL);
+ g_assert(bdrv2->throttle_state != NULL);
+ g_assert(bdrv3->throttle_state != NULL);
+
+ g_assert(!strcmp(throttle_group_get_name(bdrv1), "bar"));
+ g_assert(!strcmp(throttle_group_get_name(bdrv2), "foo"));
+ g_assert(bdrv1->throttle_state == bdrv3->throttle_state);
+
+ /* Setting the config of a group member affects the whole group */
+ memset(&cfg1, 0, sizeof(cfg1));
+ cfg1.buckets[THROTTLE_BPS_READ].avg = 500000;
+ cfg1.buckets[THROTTLE_BPS_WRITE].avg = 285000;
+ cfg1.buckets[THROTTLE_OPS_READ].avg = 20000;
+ cfg1.buckets[THROTTLE_OPS_WRITE].avg = 12000;
+ throttle_group_config(bdrv1, &cfg1);
+
+ throttle_group_get_config(bdrv1, &cfg1);
+ throttle_group_get_config(bdrv3, &cfg2);
+ g_assert(!memcmp(&cfg1, &cfg2, sizeof(cfg1)));
+
+ cfg2.buckets[THROTTLE_BPS_READ].avg = 4547;
+ cfg2.buckets[THROTTLE_BPS_WRITE].avg = 1349;
+ cfg2.buckets[THROTTLE_OPS_READ].avg = 123;
+ cfg2.buckets[THROTTLE_OPS_WRITE].avg = 86;
+ throttle_group_config(bdrv3, &cfg1);
+
+ throttle_group_get_config(bdrv1, &cfg1);
+ throttle_group_get_config(bdrv3, &cfg2);
+ g_assert(!memcmp(&cfg1, &cfg2, sizeof(cfg1)));
+
+ throttle_group_unregister_bs(bdrv1);
+ throttle_group_unregister_bs(bdrv2);
+ throttle_group_unregister_bs(bdrv3);
+
+ g_assert(bdrv1->throttle_state == NULL);
+ g_assert(bdrv2->throttle_state == NULL);
+ g_assert(bdrv3->throttle_state == NULL);
+}
+
int main(int argc, char **argv)
{
- GSource *src;
Error *local_error = NULL;
- init_clocks();
+ qemu_init_main_loop(&local_error);
+ ctx = qemu_get_aio_context();
- ctx = aio_context_new(&local_error);
if (!ctx) {
error_report("Failed to create AIO Context: '%s'",
- error_get_pretty(local_error));
- error_free(local_error);
+ local_error ? error_get_pretty(local_error) :
+ "Failed to initialize the QEMU main loop");
+ if (local_error) {
+ error_free(local_error);
+ }
exit(1);
}
- src = aio_get_g_source(ctx);
- g_source_attach(src, NULL);
- g_source_unref(src);
do {} while (g_main_context_iteration(NULL, false));
@@ -533,6 +591,7 @@ int main(int argc, char **argv)
g_test_add_func("/throttle/config/is_valid", test_is_valid);
g_test_add_func("/throttle/config_functions", test_config_functions);
g_test_add_func("/throttle/accounting", test_accounting);
+ g_test_add_func("/throttle/groups", test_groups);
return g_test_run();
}
--
2.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] throttle: Add throttle group infrastructure tests
2015-03-30 16:19 ` [Qemu-devel] [PATCH 3/7] throttle: Add throttle group infrastructure tests Alberto Garcia
@ 2015-04-01 10:00 ` Fam Zheng
2015-04-09 12:30 ` Stefan Hajnoczi
1 sibling, 0 replies; 24+ messages in thread
From: Fam Zheng @ 2015-04-01 10:00 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Mon, 03/30 19:19, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> tests/test-throttle.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 69 insertions(+), 10 deletions(-)
>
> diff --git a/tests/test-throttle.c b/tests/test-throttle.c
> index 458f577..aa0e297 100644
> --- a/tests/test-throttle.c
> +++ b/tests/test-throttle.c
> @@ -1,10 +1,12 @@
> /*
> * Throttle infrastructure tests
> *
> - * Copyright Nodalink, SARL. 2013
> + * Copyright Nodalink, EURL. 2013-2014
> + * Copyright Igalia, S.L. 2015
> *
> * Authors:
> - * Benoît Canet <benoit.canet@irqsave.net>
> + * Benoît Canet <benoit.canet@nodalink.com>
> + * Alberto Garcia <berto@igalia.com>
Could be moved to patch 7.
Fam
> *
> * This work is licensed under the terms of the GNU LGPL, version 2 or later.
> * See the COPYING.LIB file in the top-level directory.
> @@ -15,6 +17,7 @@
> #include "block/aio.h"
> #include "qemu/throttle.h"
> #include "qemu/error-report.h"
> +#include "block/throttle-groups.h"
>
> static AioContext *ctx;
> static LeakyBucket bkt;
> @@ -500,23 +503,78 @@ static void test_accounting(void)
> (64.0 / 13)));
> }
>
> +static void test_groups(void)
> +{
> + ThrottleConfig cfg1, cfg2;
> + BlockDriverState *bdrv1, *bdrv2, *bdrv3;
> +
> + bdrv1 = bdrv_new();
> + bdrv2 = bdrv_new();
> + bdrv3 = bdrv_new();
> +
> + g_assert(bdrv1->throttle_state == NULL);
> + g_assert(bdrv2->throttle_state == NULL);
> + g_assert(bdrv3->throttle_state == NULL);
> +
> + throttle_group_register_bs(bdrv1, "bar");
> + throttle_group_register_bs(bdrv2, "foo");
> + throttle_group_register_bs(bdrv3, "bar");
> +
> + g_assert(bdrv1->throttle_state != NULL);
> + g_assert(bdrv2->throttle_state != NULL);
> + g_assert(bdrv3->throttle_state != NULL);
> +
> + g_assert(!strcmp(throttle_group_get_name(bdrv1), "bar"));
> + g_assert(!strcmp(throttle_group_get_name(bdrv2), "foo"));
> + g_assert(bdrv1->throttle_state == bdrv3->throttle_state);
> +
> + /* Setting the config of a group member affects the whole group */
> + memset(&cfg1, 0, sizeof(cfg1));
> + cfg1.buckets[THROTTLE_BPS_READ].avg = 500000;
> + cfg1.buckets[THROTTLE_BPS_WRITE].avg = 285000;
> + cfg1.buckets[THROTTLE_OPS_READ].avg = 20000;
> + cfg1.buckets[THROTTLE_OPS_WRITE].avg = 12000;
> + throttle_group_config(bdrv1, &cfg1);
> +
> + throttle_group_get_config(bdrv1, &cfg1);
> + throttle_group_get_config(bdrv3, &cfg2);
> + g_assert(!memcmp(&cfg1, &cfg2, sizeof(cfg1)));
> +
> + cfg2.buckets[THROTTLE_BPS_READ].avg = 4547;
> + cfg2.buckets[THROTTLE_BPS_WRITE].avg = 1349;
> + cfg2.buckets[THROTTLE_OPS_READ].avg = 123;
> + cfg2.buckets[THROTTLE_OPS_WRITE].avg = 86;
> + throttle_group_config(bdrv3, &cfg1);
> +
> + throttle_group_get_config(bdrv1, &cfg1);
> + throttle_group_get_config(bdrv3, &cfg2);
> + g_assert(!memcmp(&cfg1, &cfg2, sizeof(cfg1)));
> +
> + throttle_group_unregister_bs(bdrv1);
> + throttle_group_unregister_bs(bdrv2);
> + throttle_group_unregister_bs(bdrv3);
> +
> + g_assert(bdrv1->throttle_state == NULL);
> + g_assert(bdrv2->throttle_state == NULL);
> + g_assert(bdrv3->throttle_state == NULL);
> +}
> +
> int main(int argc, char **argv)
> {
> - GSource *src;
> Error *local_error = NULL;
>
> - init_clocks();
> + qemu_init_main_loop(&local_error);
> + ctx = qemu_get_aio_context();
>
> - ctx = aio_context_new(&local_error);
> if (!ctx) {
> error_report("Failed to create AIO Context: '%s'",
> - error_get_pretty(local_error));
> - error_free(local_error);
> + local_error ? error_get_pretty(local_error) :
> + "Failed to initialize the QEMU main loop");
> + if (local_error) {
> + error_free(local_error);
> + }
> exit(1);
> }
> - src = aio_get_g_source(ctx);
> - g_source_attach(src, NULL);
> - g_source_unref(src);
>
> do {} while (g_main_context_iteration(NULL, false));
>
> @@ -533,6 +591,7 @@ int main(int argc, char **argv)
> g_test_add_func("/throttle/config/is_valid", test_is_valid);
> g_test_add_func("/throttle/config_functions", test_config_functions);
> g_test_add_func("/throttle/accounting", test_accounting);
> + g_test_add_func("/throttle/groups", test_groups);
> return g_test_run();
> }
>
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] throttle: Add throttle group infrastructure tests
2015-03-30 16:19 ` [Qemu-devel] [PATCH 3/7] throttle: Add throttle group infrastructure tests Alberto Garcia
2015-04-01 10:00 ` Fam Zheng
@ 2015-04-09 12:30 ` Stefan Hajnoczi
1 sibling, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2015-04-09 12:30 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 308 bytes --]
On Mon, Mar 30, 2015 at 07:19:41PM +0300, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> tests/test-throttle.c | 79 ++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 69 insertions(+), 10 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
2015-03-30 16:19 [Qemu-devel] [PATCH v6 0/7] Block Throttle Group Support Alberto Garcia
` (2 preceding siblings ...)
2015-03-30 16:19 ` [Qemu-devel] [PATCH 3/7] throttle: Add throttle group infrastructure tests Alberto Garcia
@ 2015-03-30 16:19 ` Alberto Garcia
2015-04-01 14:44 ` Fam Zheng
2015-04-09 14:22 ` Stefan Hajnoczi
2015-03-30 16:19 ` [Qemu-devel] [PATCH 5/7] throttle: acquire the ThrottleGroup lock in bdrv_swap() Alberto Garcia
` (2 subsequent siblings)
6 siblings, 2 replies; 24+ messages in thread
From: Alberto Garcia @ 2015-03-30 16:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Stefan Hajnoczi
The throttle group support use a cooperative round robin scheduling
algorithm.
The principles of the algorithm are simple:
- Each BDS of the group is used as a token in a circular way.
- The active BDS computes if a wait must be done and arms the right
timer.
- If a wait must be done the token timer will be armed so the token
will become the next active BDS.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 83 +++++-----------
block/qapi.c | 5 +-
block/throttle-groups.c | 207 +++++++++++++++++++++++++++++++++++++++-
blockdev.c | 22 ++++-
hmp.c | 4 +-
include/block/block.h | 3 +-
include/block/block_int.h | 7 +-
include/block/throttle-groups.h | 4 +
qapi/block-core.json | 4 +-
qemu-options.hx | 1 +
qmp-commands.hx | 3 +-
11 files changed, 271 insertions(+), 72 deletions(-)
diff --git a/block.c b/block.c
index 28508ef..5ca972f 100644
--- a/block.c
+++ b/block.c
@@ -36,6 +36,7 @@
#include "qmp-commands.h"
#include "qemu/timer.h"
#include "qapi-event.h"
+#include "block/throttle-groups.h"
#ifdef CONFIG_BSD
#include <sys/types.h>
@@ -130,7 +131,7 @@ void bdrv_set_io_limits(BlockDriverState *bs,
{
int i;
- throttle_config(&bs->throttle_state, &bs->throttle_timers, cfg);
+ throttle_group_config(bs, cfg);
for (i = 0; i < 2; i++) {
qemu_co_enter_next(&bs->throttled_reqs[i]);
@@ -160,70 +161,33 @@ static bool bdrv_start_throttled_reqs(BlockDriverState *bs)
void bdrv_io_limits_disable(BlockDriverState *bs)
{
bs->io_limits_enabled = false;
-
bdrv_start_throttled_reqs(bs);
-
- throttle_timers_destroy(&bs->throttle_timers);
-}
-
-static void bdrv_throttle_read_timer_cb(void *opaque)
-{
- BlockDriverState *bs = opaque;
- qemu_co_enter_next(&bs->throttled_reqs[0]);
-}
-
-static void bdrv_throttle_write_timer_cb(void *opaque)
-{
- BlockDriverState *bs = opaque;
- qemu_co_enter_next(&bs->throttled_reqs[1]);
+ throttle_group_unregister_bs(bs);
}
/* should be called before bdrv_set_io_limits if a limit is set */
-void bdrv_io_limits_enable(BlockDriverState *bs)
+void bdrv_io_limits_enable(BlockDriverState *bs, const char *group)
{
assert(!bs->io_limits_enabled);
- throttle_init(&bs->throttle_state);
- throttle_timers_init(&bs->throttle_timers,
- bdrv_get_aio_context(bs),
- QEMU_CLOCK_VIRTUAL,
- bdrv_throttle_read_timer_cb,
- bdrv_throttle_write_timer_cb,
- bs);
+ throttle_group_register_bs(bs, group);
bs->io_limits_enabled = true;
}
-/* This function makes an IO wait if needed
- *
- * @nb_sectors: the number of sectors of the IO
- * @is_write: is the IO a write
- */
-static void bdrv_io_limits_intercept(BlockDriverState *bs,
- unsigned int bytes,
- bool is_write)
+void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group)
{
- /* does this io must wait */
- bool must_wait = throttle_schedule_timer(&bs->throttle_state,
- &bs->throttle_timers,
- is_write);
-
- /* if must wait or any request of this type throttled queue the IO */
- if (must_wait ||
- !qemu_co_queue_empty(&bs->throttled_reqs[is_write])) {
- qemu_co_queue_wait(&bs->throttled_reqs[is_write]);
+ /* this bs is not part of any group */
+ if (!bs->throttle_state) {
+ return;
}
- /* the IO will be executed, do the accounting */
- throttle_account(&bs->throttle_state, is_write, bytes);
-
-
- /* if the next request must wait -> do nothing */
- if (throttle_schedule_timer(&bs->throttle_state, &bs->throttle_timers,
- is_write)) {
+ /* this bs is a part of the same group than the one we want */
+ if (!g_strcmp0(throttle_group_get_name(bs), group)) {
return;
}
- /* else queue next request for execution */
- qemu_co_queue_next(&bs->throttled_reqs[is_write]);
+ /* need to change the group this bs belong to */
+ bdrv_io_limits_disable(bs);
+ bdrv_io_limits_enable(bs, group);
}
size_t bdrv_opt_mem_align(BlockDriverState *bs)
@@ -2091,15 +2055,18 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
bs_dest->enable_write_cache = bs_src->enable_write_cache;
/* i/o throttled req */
- memcpy(&bs_dest->throttle_state,
- &bs_src->throttle_state,
- sizeof(ThrottleState));
+ bs_dest->throttle_state = bs_src->throttle_state,
+ bs_dest->io_limits_enabled = bs_src->io_limits_enabled;
+ bs_dest->pending_reqs[0] = bs_src->pending_reqs[0];
+ bs_dest->pending_reqs[1] = bs_src->pending_reqs[1];
+ bs_dest->throttled_reqs[0] = bs_src->throttled_reqs[0];
+ bs_dest->throttled_reqs[1] = bs_src->throttled_reqs[1];
+ memcpy(&bs_dest->round_robin,
+ &bs_src->round_robin,
+ sizeof(bs_dest->round_robin));
memcpy(&bs_dest->throttle_timers,
&bs_src->throttle_timers,
sizeof(ThrottleTimers));
- bs_dest->throttled_reqs[0] = bs_src->throttled_reqs[0];
- bs_dest->throttled_reqs[1] = bs_src->throttled_reqs[1];
- bs_dest->io_limits_enabled = bs_src->io_limits_enabled;
/* r/w error */
bs_dest->on_read_error = bs_src->on_read_error;
@@ -3170,7 +3137,7 @@ static int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
/* throttling disk I/O */
if (bs->io_limits_enabled) {
- bdrv_io_limits_intercept(bs, bytes, false);
+ throttle_group_co_io_limits_intercept(bs, bytes, false);
}
/* Align read if necessary by padding qiov */
@@ -3411,7 +3378,7 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
/* throttling disk I/O */
if (bs->io_limits_enabled) {
- bdrv_io_limits_intercept(bs, bytes, true);
+ throttle_group_co_io_limits_intercept(bs, bytes, true);
}
/*
diff --git a/block/qapi.c b/block/qapi.c
index 8a19aed..65af057 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -24,6 +24,7 @@
#include "block/qapi.h"
#include "block/block_int.h"
+#include "block/throttle-groups.h"
#include "block/write-threshold.h"
#include "qmp-commands.h"
#include "qapi-visit.h"
@@ -63,7 +64,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
if (bs->io_limits_enabled) {
ThrottleConfig cfg;
- throttle_get_config(&bs->throttle_state, &cfg);
+
+ throttle_group_get_config(bs, &cfg);
+
info->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
info->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg;
info->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE].avg;
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 352077f..f909f50 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -23,6 +23,8 @@
*/
#include "block/throttle-groups.h"
+#include "qemu/queue.h"
+#include "qemu/thread.h"
/* The ThrottleGroup structure (with its ThrottleState) is shared
* among different BlockDriverState and it's independent from
@@ -160,6 +162,153 @@ static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs)
return next;
}
+/* Return the next BlockDriverState in the round-robin sequence with
+ * pending I/O requests.
+ *
+ * This assumes that tg->lock is held.
+ *
+ * @bs: the current BlockDriverState
+ * @is_write: the type of operation (read/write)
+ * @ret: the next BlockDriverState with pending requests, or bs
+ * if there is none.
+ */
+static BlockDriverState *next_throttle_token(BlockDriverState *bs,
+ bool is_write)
+{
+ ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ BlockDriverState *token, *start;
+
+ start = token = tg->tokens[is_write];
+
+ /* get next bs round in round robin style */
+ token = throttle_group_next_bs(token);
+ while (token != start && !token->pending_reqs[is_write]) {
+ token = throttle_group_next_bs(token);
+ }
+
+ /* If no IO are queued for scheduling on the next round robin token
+ * then decide the token is the current bs because chances are
+ * the current bs get the current request queued.
+ */
+ if (token == start && !token->pending_reqs[is_write]) {
+ token = bs;
+ }
+
+ return token;
+}
+
+/* Check if the next I/O request for a BlockDriverState needs to be
+ * throttled or not. If there's no timer set in this group, set one
+ * and update the token accordingly.
+ *
+ * This assumes that tg->lock is held.
+ *
+ * @bs: the current BlockDriverState
+ * @is_write: the type of operation (read/write)
+ * @ret: whether the I/O request needs to be throttled or not
+ */
+static bool throttle_group_schedule_timer(BlockDriverState *bs,
+ bool is_write)
+{
+ ThrottleState *ts = bs->throttle_state;
+ ThrottleTimers *tt = &bs->throttle_timers;
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ bool must_wait;
+
+ /* Check if any of the timers in this group is already armed */
+ if (tg->any_timer_armed[is_write]) {
+ return true;
+ }
+
+ must_wait = throttle_schedule_timer(ts, tt, is_write);
+
+ /* If a timer just got armed, set bs as the current token */
+ if (must_wait) {
+ tg->tokens[is_write] = bs;
+ tg->any_timer_armed[is_write] = true;
+ }
+
+ return must_wait;
+}
+
+/* Look for the next pending I/O request and schedule it.
+ *
+ * This assumes that tg->lock is held.
+ *
+ * @bs: the current BlockDriverState
+ * @is_write: the type of operation (read/write)
+ */
+static void schedule_next_request(BlockDriverState *bs, bool is_write)
+{
+ ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ bool must_wait;
+ BlockDriverState *token;
+
+ /* Check if there's any pending request to schedule next */
+ token = next_throttle_token(bs, is_write);
+ if (!token->pending_reqs[is_write]) {
+ return;
+ }
+
+ /* Set a timer for the request if it needs to be throttled */
+ must_wait = throttle_group_schedule_timer(token, is_write);
+
+ /* If it doesn't have to wait, queue it for immediate execution */
+ if (!must_wait) {
+ /* Give preference to requests from the current bs */
+ if (qemu_in_coroutine() &&
+ qemu_co_queue_next(&bs->throttled_reqs[is_write])) {
+ token = bs;
+ } else {
+ ThrottleTimers *tt = &token->throttle_timers;
+ int64_t now = qemu_clock_get_ns(tt->clock_type);
+ timer_mod(tt->timers[is_write], now + 1);
+ tg->any_timer_armed[is_write] = true;
+ }
+ tg->tokens[is_write] = token;
+ }
+}
+
+/* Check if an I/O request needs to be throttled, wait and set a timer
+ * if necessary, and schedule the next request using a round robin
+ * algorithm.
+ *
+ * @bs: the current BlockDriverState
+ * @bytes: the number of bytes for this I/O
+ * @is_write: the type of operation (read/write)
+ */
+void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
+ unsigned int bytes,
+ bool is_write)
+{
+ bool must_wait;
+ BlockDriverState *token;
+
+ ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ qemu_mutex_lock(&tg->lock);
+
+ /* First we check if this I/O has to be throttled. */
+ token = next_throttle_token(bs, is_write);
+ must_wait = throttle_group_schedule_timer(token, is_write);
+
+ /* Wait if there's a timer set or queued requests of this type */
+ if (must_wait || bs->pending_reqs[is_write]) {
+ bs->pending_reqs[is_write]++;
+ qemu_mutex_unlock(&tg->lock);
+ qemu_co_queue_wait(&bs->throttled_reqs[is_write]);
+ qemu_mutex_lock(&tg->lock);
+ bs->pending_reqs[is_write]--;
+ }
+
+ /* The I/O will be executed, so do the accounting */
+ throttle_account(bs->throttle_state, is_write, bytes);
+
+ /* Schedule the next request */
+ schedule_next_request(bs, is_write);
+
+ qemu_mutex_unlock(&tg->lock);
+}
+
/* Update the throttle configuration for a particular group. Similar
* to throttle_config(), but guarantees atomicity within the
* throttling group.
@@ -195,9 +344,49 @@ void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg)
qemu_mutex_unlock(&tg->lock);
}
-/* Register a BlockDriverState in the throttling group, also updating
- * its throttle_state pointer to point to it. If a throttling group
- * with that name does not exist yet, it will be created.
+/* ThrottleTimers callback. This wakes up a request that was waiting
+ * because it had been throttled.
+ *
+ * @bs: the BlockDriverState whose request had been throttled
+ * @is_write: the type of operation (read/write)
+ */
+static void timer_cb(BlockDriverState *bs, bool is_write)
+{
+ ThrottleState *ts = bs->throttle_state;
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ bool empty_queue;
+
+ /* The timer has just been fired, so we can update the flag */
+ qemu_mutex_lock(&tg->lock);
+ tg->any_timer_armed[is_write] = false;
+ qemu_mutex_unlock(&tg->lock);
+
+ /* Run the request that was waiting for this timer */
+ empty_queue = !qemu_co_enter_next(&bs->throttled_reqs[is_write]);
+
+ /* 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(bs, is_write);
+ qemu_mutex_unlock(&tg->lock);
+ }
+}
+
+static void read_timer_cb(void *opaque)
+{
+ timer_cb(opaque, false);
+}
+
+static void write_timer_cb(void *opaque)
+{
+ timer_cb(opaque, true);
+}
+
+/* Register a BlockDriverState in the throttling group, also
+ * initializing its timers and updating its throttle_state pointer to
+ * point to it. If a throttling group with that name does not exist
+ * yet, it will be created.
*
* @bs: the BlockDriverState to insert
* @groupname: the name of the group
@@ -218,11 +407,20 @@ void throttle_group_register_bs(BlockDriverState *bs, const char *groupname)
}
QLIST_INSERT_HEAD(&tg->head, bs, round_robin);
+
+ throttle_timers_init(&bs->throttle_timers,
+ bdrv_get_aio_context(bs),
+ QEMU_CLOCK_VIRTUAL,
+ read_timer_cb,
+ write_timer_cb,
+ bs);
+
qemu_mutex_unlock(&tg->lock);
}
/* Unregister a BlockDriverState from its group, removing it from the
- * list and setting the throttle_state pointer to NULL.
+ * list, destroying the timers and setting the throttle_state pointer
+ * to NULL.
*
* The group will be destroyed if it's empty after this operation.
*
@@ -247,6 +445,7 @@ void throttle_group_unregister_bs(BlockDriverState *bs)
/* remove the current bs from the list */
QLIST_REMOVE(bs, round_robin);
+ throttle_timers_destroy(&bs->throttle_timers);
qemu_mutex_unlock(&tg->lock);
throttle_group_unref(tg);
diff --git a/blockdev.c b/blockdev.c
index fbb3a79..98ef26f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -357,6 +357,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+ const char *throttling_group;
/* Check common options by copying from bs_opts to opts, all other options
* stay in bs_opts for processing by bdrv_open(). */
@@ -459,6 +460,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
cfg.op_size = qemu_opt_get_number(opts, "throttling.iops-size", 0);
+ throttling_group = qemu_opt_get(opts, "throttling.group");
+
if (!check_throttle_config(&cfg, &error)) {
error_propagate(errp, error);
goto early_err;
@@ -547,7 +550,10 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
/* disk I/O throttling */
if (throttle_enabled(&cfg)) {
- bdrv_io_limits_enable(bs);
+ if (!throttling_group) {
+ throttling_group = blk_name(blk);
+ }
+ bdrv_io_limits_enable(bs, throttling_group);
bdrv_set_io_limits(bs, &cfg);
}
@@ -711,6 +717,8 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
{ "iops_size", "throttling.iops-size" },
+ { "group", "throttling.group" },
+
{ "readonly", "read-only" },
};
@@ -1887,7 +1895,9 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
bool has_iops_wr_max,
int64_t iops_wr_max,
bool has_iops_size,
- int64_t iops_size, Error **errp)
+ int64_t iops_size,
+ bool has_group,
+ const char *group, Error **errp)
{
ThrottleConfig cfg;
BlockDriverState *bs;
@@ -1941,9 +1951,11 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
aio_context_acquire(aio_context);
if (!bs->io_limits_enabled && throttle_enabled(&cfg)) {
- bdrv_io_limits_enable(bs);
+ bdrv_io_limits_enable(bs, has_group ? group : device);
} else if (bs->io_limits_enabled && !throttle_enabled(&cfg)) {
bdrv_io_limits_disable(bs);
+ } else if (bs->io_limits_enabled && throttle_enabled(&cfg)) {
+ bdrv_io_limits_update_group(bs, has_group ? group : device);
}
if (bs->io_limits_enabled) {
@@ -3017,6 +3029,10 @@ QemuOptsList qemu_common_drive_opts = {
.type = QEMU_OPT_NUMBER,
.help = "when limiting by iops max size of an I/O in bytes",
},{
+ .name = "throttling.group",
+ .type = QEMU_OPT_STRING,
+ .help = "name of the block throttling group",
+ },{
.name = "copy-on-read",
.type = QEMU_OPT_BOOL,
.help = "copy read data from backing file into image file",
diff --git a/hmp.c b/hmp.c
index f31ae27..50f30f2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1272,7 +1272,9 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
false,
0,
false, /* No default I/O size */
- 0, &err);
+ 0,
+ false,
+ NULL, &err);
hmp_handle_error(mon, &err);
}
diff --git a/include/block/block.h b/include/block/block.h
index 4c57d63..e0d53f3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -173,8 +173,9 @@ void bdrv_stats_print(Monitor *mon, const QObject *data);
void bdrv_info_stats(Monitor *mon, QObject **ret_data);
/* disk I/O throttling */
-void bdrv_io_limits_enable(BlockDriverState *bs);
+void bdrv_io_limits_enable(BlockDriverState *bs, const char *group);
void bdrv_io_limits_disable(BlockDriverState *bs);
+void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group);
void bdrv_init(void);
void bdrv_init_with_whitelist(void);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f0896e9..4e09825 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -376,10 +376,13 @@ struct BlockDriverState {
unsigned int serialising_in_flight;
/* I/O throttling */
- ThrottleState throttle_state;
- ThrottleTimers throttle_timers;
CoQueue throttled_reqs[2];
bool io_limits_enabled;
+ /* The following fields are protected by the ThrottleGroup lock.
+ * See the ThrottleGroup documentation for details. */
+ ThrottleState *throttle_state;
+ ThrottleTimers throttle_timers;
+ unsigned pending_reqs[2];
QLIST_ENTRY(BlockDriverState) round_robin;
/* I/O stats (display with "info blockstats"). */
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index b966ec7..322139a 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -36,4 +36,8 @@ void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg);
void throttle_group_register_bs(BlockDriverState *bs, const char *groupname);
void throttle_group_unregister_bs(BlockDriverState *bs);
+void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
+ unsigned int bytes,
+ bool is_write);
+
#endif
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7873084..d8211b7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -990,6 +990,8 @@
#
# @iops_size: #optional an I/O size in bytes (Since 1.7)
#
+# @group: #optional throttle group name (Since 2.3)
+#
# Returns: Nothing on success
# If @device is not a valid block device, DeviceNotFound
#
@@ -1001,7 +1003,7 @@
'*bps_max': 'int', '*bps_rd_max': 'int',
'*bps_wr_max': 'int', '*iops_max': 'int',
'*iops_rd_max': 'int', '*iops_wr_max': 'int',
- '*iops_size': 'int' } }
+ '*iops_size': 'int', '*group': 'str' } }
##
# @block-stream:
diff --git a/qemu-options.hx b/qemu-options.hx
index 319d971..7d6b2ec 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -464,6 +464,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
" [[,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n"
" [[,iops_max=im]|[[,iops_rd_max=irm][,iops_wr_max=iwm]]]\n"
" [[,iops_size=is]]\n"
+ " [[,group=g]]\n"
" use 'file' as a drive image\n", QEMU_ARCH_ALL)
STEXI
@item -drive @var{option}[,@var{option}[,@var{option}[,...]]]
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 3a42ad0..ce897ff 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1730,7 +1730,7 @@ EQMP
{
.name = "block_set_io_throttle",
- .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?,iops_size:l?",
+ .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?,iops_size:l?,group:s?",
.mhandler.cmd_new = qmp_marshal_input_block_set_io_throttle,
},
@@ -1756,6 +1756,7 @@ Arguments:
- "iops_rd_max": read I/O operations max (json-int)
- "iops_wr_max": write I/O operations max (json-int)
- "iops_size": I/O size in bytes when limiting (json-int)
+- "group": throttle group name (json-string)
Example:
--
2.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
2015-03-30 16:19 ` [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support Alberto Garcia
@ 2015-04-01 14:44 ` Fam Zheng
2015-04-01 15:18 ` Alberto Garcia
2015-04-09 14:22 ` Stefan Hajnoczi
1 sibling, 1 reply; 24+ messages in thread
From: Fam Zheng @ 2015-04-01 14:44 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Mon, 03/30 19:19, Alberto Garcia wrote:
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7873084..d8211b7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -990,6 +990,8 @@
> #
> # @iops_size: #optional an I/O size in bytes (Since 1.7)
> #
> +# @group: #optional throttle group name (Since 2.3)
We should probably elaborate (here, and at other places of @group appearances):
@device is used as group name. This is useful since other devices could use
this device name as the group name, for exmple the following -drive definition:
...
-drive file=foo,id=foo,bps=100 \
-drive file=bar,id=foo,bps=200,group=foo \
...
will put both devices into the same group.
Also, as the two bps values don't match, I assume the last one is used? Is
there a warning when ignoring a config from previous definitions?
Thinking about this, I'd slightly prefer a canonical throttle group definition
rather than patching the existing parameters:
-object throttle-group,id=tg0,bps=100,iops=200,iops-max=1000 \
-drive file=foo,id=foo,throttle-group=tg0 \
-drive file=bar,id=bar,throttle-group=tg0 \
and error out if "bps=" etc are specified together with "throttle-group=" in
-drive.
And in QMP, add block_set_io_throttle_group which works together with
object-add.
What do you think?
> +#
> # Returns: Nothing on success
> # If @device is not a valid block device, DeviceNotFound
> #
> @@ -1001,7 +1003,7 @@
> '*bps_max': 'int', '*bps_rd_max': 'int',
> '*bps_wr_max': 'int', '*iops_max': 'int',
> '*iops_rd_max': 'int', '*iops_wr_max': 'int',
> - '*iops_size': 'int' } }
> + '*iops_size': 'int', '*group': 'str' } }
>
> ##
> # @block-stream:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 319d971..7d6b2ec 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -464,6 +464,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
> " [[,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n"
> " [[,iops_max=im]|[[,iops_rd_max=irm][,iops_wr_max=iwm]]]\n"
> " [[,iops_size=is]]\n"
> + " [[,group=g]]\n"
> " use 'file' as a drive image\n", QEMU_ARCH_ALL)
> STEXI
> @item -drive @var{option}[,@var{option}[,@var{option}[,...]]]
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 3a42ad0..ce897ff 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1730,7 +1730,7 @@ EQMP
>
> {
> .name = "block_set_io_throttle",
> - .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?,iops_size:l?",
> + .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?,iops_size:l?,group:s?",
> .mhandler.cmd_new = qmp_marshal_input_block_set_io_throttle,
> },
>
> @@ -1756,6 +1756,7 @@ Arguments:
> - "iops_rd_max": read I/O operations max (json-int)
> - "iops_wr_max": write I/O operations max (json-int)
> - "iops_size": I/O size in bytes when limiting (json-int)
Why removing these three?
> +- "group": throttle group name (json-string)
>
> Example:
>
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
2015-04-01 14:44 ` Fam Zheng
@ 2015-04-01 15:18 ` Alberto Garcia
2015-04-02 3:26 ` Fam Zheng
0 siblings, 1 reply; 24+ messages in thread
From: Alberto Garcia @ 2015-04-01 15:18 UTC (permalink / raw)
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Wed, Apr 01, 2015 at 10:44:51PM +0800, Fam Zheng wrote:
> > +# @group: #optional throttle group name (Since 2.3)
>
> We should probably elaborate (here, and at other places of @group
> appearances): @device is used as group name. This is useful since
> other devices could use this device name as the group name, for
> exmple the following -drive definition:
>
> ...
> -drive file=foo,id=foo,bps=100 \
> -drive file=bar,id=foo,bps=200,group=foo \
> ...
>
> will put both devices into the same group.
That's right, thanks for pointing it out. I can update the
documentation. Alternatively we could use some other mechanism for
auto-generating group names, rather than using the name of the device,
but either way I think it deserves being documented.
> Also, as the two bps values don't match, I assume the last one
> is used? Is there a warning when ignoring a config from previous
> definitions?
Everytime a new device is added to a group it sets new values for the
whole group, because values are not attached to any particular device.
For the same reason if one device is removed the rest will continue
working with the existing configuration.
> Thinking about this, I'd slightly prefer a canonical throttle group
> definition rather than patching the existing parameters:
>
> -object throttle-group,id=tg0,bps=100,iops=200,iops-max=1000 \
> -drive file=foo,id=foo,throttle-group=tg0 \
> -drive file=bar,id=bar,throttle-group=tg0 \
>
> and error out if "bps=" etc are specified together with
> "throttle-group=" in -drive.
>
> And in QMP, add block_set_io_throttle_group which works together
> with object-add.
And then you would create a group automatically for drives that have
bps= specified? What would happen if all members of the group are
removed? And what would happen if it's a group created with -object?
My first impression is that the idea is feasible, but I'm unsure at
the moment of its complexity or possible side effects.
> > @@ -1756,6 +1756,7 @@ Arguments:
> > - "iops_rd_max": read I/O operations max (json-int)
> > - "iops_wr_max": write I/O operations max (json-int)
> > - "iops_size": I/O size in bytes when limiting (json-int)
>
> Why removing these three?
There's nothing removed, that's just a list of bullets, note the
column where the dashes appear :)
Berto
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
2015-04-01 15:18 ` Alberto Garcia
@ 2015-04-02 3:26 ` Fam Zheng
2015-04-02 7:36 ` Alberto Garcia
0 siblings, 1 reply; 24+ messages in thread
From: Fam Zheng @ 2015-04-02 3:26 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Wed, 04/01 17:18, Alberto Garcia wrote:
> On Wed, Apr 01, 2015 at 10:44:51PM +0800, Fam Zheng wrote:
>
> > > +# @group: #optional throttle group name (Since 2.3)
> >
> > We should probably elaborate (here, and at other places of @group
> > appearances): @device is used as group name. This is useful since
> > other devices could use this device name as the group name, for
> > exmple the following -drive definition:
> >
> > ...
> > -drive file=foo,id=foo,bps=100 \
> > -drive file=bar,id=foo,bps=200,group=foo \
> > ...
> >
> > will put both devices into the same group.
>
> That's right, thanks for pointing it out. I can update the
> documentation. Alternatively we could use some other mechanism for
> auto-generating group names, rather than using the name of the device,
> but either way I think it deserves being documented.
>
> > Also, as the two bps values don't match, I assume the last one
> > is used? Is there a warning when ignoring a config from previous
> > definitions?
>
> Everytime a new device is added to a group it sets new values for the
> whole group, because values are not attached to any particular device.
> For the same reason if one device is removed the rest will continue
> working with the existing configuration.
>
> > Thinking about this, I'd slightly prefer a canonical throttle group
> > definition rather than patching the existing parameters:
> >
> > -object throttle-group,id=tg0,bps=100,iops=200,iops-max=1000 \
> > -drive file=foo,id=foo,throttle-group=tg0 \
> > -drive file=bar,id=bar,throttle-group=tg0 \
> >
> > and error out if "bps=" etc are specified together with
> > "throttle-group=" in -drive.
> >
> > And in QMP, add block_set_io_throttle_group which works together
> > with object-add.
>
> And then you would create a group automatically for drives that have
> bps= specified?
Yes.
> What would happen if all members of the group are removed?
The automatic group created from "bps=" should be anonymous, which is attached
by the only one BDS. If it is removed, refcount should release the group
automatically.
> And what would happen if it's a group created with -object?
The -object has a refcount, so it should stay even when all members are
removed, unless "(QMP) object-del".
>
> My first impression is that the idea is feasible, but I'm unsure at
> the moment of its complexity or possible side effects.
I think it is a cleaner model and easier to understand. What complexity are you
referring to?
>
> > > @@ -1756,6 +1756,7 @@ Arguments:
> > > - "iops_rd_max": read I/O operations max (json-int)
> > > - "iops_wr_max": write I/O operations max (json-int)
> > > - "iops_size": I/O size in bytes when limiting (json-int)
> >
> > Why removing these three?
>
> There's nothing removed, that's just a list of bullets, note the
> column where the dashes appear :)
:D
Fam
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
2015-04-02 3:26 ` Fam Zheng
@ 2015-04-02 7:36 ` Alberto Garcia
0 siblings, 0 replies; 24+ messages in thread
From: Alberto Garcia @ 2015-04-02 7:36 UTC (permalink / raw)
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Thu, Apr 02, 2015 at 11:26:30AM +0800, Fam Zheng wrote:
> > > Thinking about this, I'd slightly prefer a canonical throttle
> > > group definition rather than patching the existing parameters:
> > >
> > > -object throttle-group,id=tg0,bps=100,iops=200,iops-max=1000 \
> > > -drive file=foo,id=foo,throttle-group=tg0 \
> > > -drive file=bar,id=bar,throttle-group=tg0 \
> > >
> > > and error out if "bps=" etc are specified together with
> > > "throttle-group=" in -drive.
> > >
> > > And in QMP, add block_set_io_throttle_group which works together
> > > with object-add.
>
> > My first impression is that the idea is feasible, but I'm unsure
> > at the moment of its complexity or possible side effects.
>
> I think it is a cleaner model and easier to understand. What
> complexity are you referring to?
The changes on the code. At first sight it seems that it should not
be too difficult to convert the current implementation (and API) into
the one you are proposing, but I'll wait for a few days to hear other
opinions before attempting to rewrite it :)
Berto
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
2015-03-30 16:19 ` [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support Alberto Garcia
2015-04-01 14:44 ` Fam Zheng
@ 2015-04-09 14:22 ` Stefan Hajnoczi
2015-04-10 7:58 ` Alberto Garcia
1 sibling, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2015-04-09 14:22 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]
On Mon, Mar 30, 2015 at 07:19:42PM +0300, Alberto Garcia wrote:
> @@ -1941,9 +1951,11 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
> aio_context_acquire(aio_context);
>
> if (!bs->io_limits_enabled && throttle_enabled(&cfg)) {
> - bdrv_io_limits_enable(bs);
> + bdrv_io_limits_enable(bs, has_group ? group : device);
> } else if (bs->io_limits_enabled && !throttle_enabled(&cfg)) {
> bdrv_io_limits_disable(bs);
> + } else if (bs->io_limits_enabled && throttle_enabled(&cfg)) {
> + bdrv_io_limits_update_group(bs, has_group ? group : device);
> }
>
> if (bs->io_limits_enabled) {
The semantics are inconsistent:
1. Create drive0 with throttle group "mygroup".
2. Issue block-set-io-throttle device="drive0"
The result is that a new throttle group called "drive0" is created. I
expected to modify the throttling configuration for drive0 (i.e.
"mygroup").
Now let's disable the throttle group:
3. Issue block-set-io-throttle with 0 values for device="drive0"
The result is that "mygroup" is changed to all 0s.
Enable should behave like disable and operate on the device's throttle
group.
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
2015-04-09 14:22 ` Stefan Hajnoczi
@ 2015-04-10 7:58 ` Alberto Garcia
2015-04-10 9:52 ` Stefan Hajnoczi
0 siblings, 1 reply; 24+ messages in thread
From: Alberto Garcia @ 2015-04-10 7:58 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Thu, Apr 09, 2015 at 03:22:57PM +0100, Stefan Hajnoczi wrote:
> > @@ -1941,9 +1951,11 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
> > aio_context_acquire(aio_context);
> >
> > if (!bs->io_limits_enabled && throttle_enabled(&cfg)) {
> > - bdrv_io_limits_enable(bs);
> > + bdrv_io_limits_enable(bs, has_group ? group : device);
> > } else if (bs->io_limits_enabled && !throttle_enabled(&cfg)) {
> > bdrv_io_limits_disable(bs);
> > + } else if (bs->io_limits_enabled && throttle_enabled(&cfg)) {
> > + bdrv_io_limits_update_group(bs, has_group ? group : device);
> > }
> >
> > if (bs->io_limits_enabled) {
>
> The semantics are inconsistent:
>
> 1. Create drive0 with throttle group "mygroup".
> 2. Issue block-set-io-throttle device="drive0"
>
> The result is that a new throttle group called "drive0" is created.
> I expected to modify the throttling configuration for drive0 (i.e.
> "mygroup").
That's right. What we can do is choose the group to update using the
following criteria, in order of precedence:
1) The 'group' parameter from block-set-io-throttle.
2) The group the device is already member of.
3) A new group ("drive0" in this example).
Currently we're not doing 2).
> Now let's disable the throttle group:
>
> 3. Issue block-set-io-throttle with 0 values for device="drive0"
>
> The result is that "mygroup" is changed to all 0s.
No, that simply removes the device from the group without touching its
configuration:
> > } else if (bs->io_limits_enabled && !throttle_enabled(&cfg)) {
> > bdrv_io_limits_disable(bs);
The group name passed by the user is actually not used in this case.
Berto
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
2015-04-10 7:58 ` Alberto Garcia
@ 2015-04-10 9:52 ` Stefan Hajnoczi
2015-04-10 9:55 ` Alberto Garcia
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2015-04-10 9:52 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]
On Fri, Apr 10, 2015 at 09:58:34AM +0200, Alberto Garcia wrote:
> On Thu, Apr 09, 2015 at 03:22:57PM +0100, Stefan Hajnoczi wrote:
>
> > > @@ -1941,9 +1951,11 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
> > > aio_context_acquire(aio_context);
> > >
> > > if (!bs->io_limits_enabled && throttle_enabled(&cfg)) {
> > > - bdrv_io_limits_enable(bs);
> > > + bdrv_io_limits_enable(bs, has_group ? group : device);
> > > } else if (bs->io_limits_enabled && !throttle_enabled(&cfg)) {
> > > bdrv_io_limits_disable(bs);
> > > + } else if (bs->io_limits_enabled && throttle_enabled(&cfg)) {
> > > + bdrv_io_limits_update_group(bs, has_group ? group : device);
> > > }
> > >
> > > if (bs->io_limits_enabled) {
> >
> > The semantics are inconsistent:
> >
> > 1. Create drive0 with throttle group "mygroup".
> > 2. Issue block-set-io-throttle device="drive0"
> >
> > The result is that a new throttle group called "drive0" is created.
> > I expected to modify the throttling configuration for drive0 (i.e.
> > "mygroup").
>
> That's right. What we can do is choose the group to update using the
> following criteria, in order of precedence:
>
> 1) The 'group' parameter from block-set-io-throttle.
> 2) The group the device is already member of.
> 3) A new group ("drive0" in this example).
>
> Currently we're not doing 2).
Great, it makes sense to add 2).
> > Now let's disable the throttle group:
> >
> > 3. Issue block-set-io-throttle with 0 values for device="drive0"
> >
> > The result is that "mygroup" is changed to all 0s.
>
> No, that simply removes the device from the group without touching its
> configuration:
>
> > > } else if (bs->io_limits_enabled && !throttle_enabled(&cfg)) {
> > > bdrv_io_limits_disable(bs);
>
> The group name passed by the user is actually not used in this case.
You are right.
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support
2015-04-10 9:52 ` Stefan Hajnoczi
@ 2015-04-10 9:55 ` Alberto Garcia
0 siblings, 0 replies; 24+ messages in thread
From: Alberto Garcia @ 2015-04-10 9:55 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Fri, Apr 10, 2015 at 10:52:18AM +0100, Stefan Hajnoczi wrote:
> > That's right. What we can do is choose the group to update using
> > the following criteria, in order of precedence:
> >
> > 1) The 'group' parameter from block-set-io-throttle.
> > 2) The group the device is already member of.
> > 3) A new group ("drive0" in this example).
> >
> > Currently we're not doing 2).
>
> Great, it makes sense to add 2).
Ok, I'll do that and explain the whole thing in the API doc.
Berto
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 5/7] throttle: acquire the ThrottleGroup lock in bdrv_swap()
2015-03-30 16:19 [Qemu-devel] [PATCH v6 0/7] Block Throttle Group Support Alberto Garcia
` (3 preceding siblings ...)
2015-03-30 16:19 ` [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support Alberto Garcia
@ 2015-03-30 16:19 ` Alberto Garcia
2015-03-30 16:19 ` [Qemu-devel] [PATCH 6/7] throttle: add the name of the ThrottleGroup to BlockDeviceInfo Alberto Garcia
2015-03-30 16:19 ` [Qemu-devel] [PATCH 7/7] throttle: Update throttle infrastructure copyright Alberto Garcia
6 siblings, 0 replies; 24+ messages in thread
From: Alberto Garcia @ 2015-03-30 16:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Stefan Hajnoczi
bdrv_swap() touches the fields of a BlockDriverState that are
protected by the ThrottleGroup lock. Although those fields end up in
their original place, they are temporarily swapped in the process,
so there's a chance that an operation on a member of the same group
happening on a different thread can try to use them.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 15 +++++++++++++++
block/throttle-groups.c | 31 ++++++++++++++++++++++++++++++-
include/block/throttle-groups.h | 3 +++
3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 5ca972f..5643082 100644
--- a/block.c
+++ b/block.c
@@ -2120,11 +2120,20 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
QTAILQ_REMOVE(&graph_bdrv_states, bs_old, node_list);
}
+ /* If the BlockDriverState is part of a throttling group acquire
+ * its lock since we're going to mess with the protected fields.
+ * Otherwise there's no need to worry since no one else can touch
+ * them. */
+ if (bs_old->throttle_state) {
+ throttle_group_lock(bs_old);
+ }
+
/* bs_new must be unattached and shouldn't have anything fancy enabled */
assert(!bs_new->blk);
assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
assert(bs_new->job == NULL);
assert(bs_new->io_limits_enabled == false);
+ assert(bs_new->throttle_state == NULL);
assert(!throttle_timers_are_initialized(&bs_new->throttle_timers));
tmp = *bs_new;
@@ -2142,8 +2151,14 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
/* Check a few fields that should remain attached to the device */
assert(bs_new->job == NULL);
assert(bs_new->io_limits_enabled == false);
+ assert(bs_new->throttle_state == NULL);
assert(!throttle_timers_are_initialized(&bs_new->throttle_timers));
+ /* Release the ThrottleGroup lock */
+ if (bs_old->throttle_state) {
+ throttle_group_unlock(bs_old);
+ }
+
/* insert the nodes back into the graph node list if needed */
if (bs_new->node_name[0] != '\0') {
QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs_new, node_list);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index f909f50..20a510a 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -32,7 +32,8 @@
* its own locking.
*
* This locking is however handled internally in this file, so it's
- * transparent to outside users.
+ * mostly transparent to outside users (but see the documentation in
+ * throttle_groups_lock()).
*
* The whole ThrottleGroup structure is private and invisible to
* outside users, that only use it through its ThrottleState.
@@ -452,6 +453,34 @@ void throttle_group_unregister_bs(BlockDriverState *bs)
bs->throttle_state = NULL;
}
+/* Acquire the lock of this throttling group.
+ *
+ * You won't normally need to use this. None of the functions from the
+ * ThrottleGroup API require you to acquire the lock since all of them
+ * deal with it internally.
+ *
+ * This should only be used in exceptional cases when you want to
+ * access the protected fields of a BlockDriverState directly
+ * (e.g. bdrv_swap()).
+ *
+ * @bs: a BlockDriverState that is member of the group
+ */
+void throttle_group_lock(BlockDriverState *bs)
+{
+ ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ qemu_mutex_lock(&tg->lock);
+}
+
+/* Release the lock of this throttling group.
+ *
+ * See the comments in throttle_group_lock().
+ */
+void throttle_group_unlock(BlockDriverState *bs)
+{
+ ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ qemu_mutex_unlock(&tg->lock);
+}
+
static void throttle_groups_init(void)
{
qemu_mutex_init(&throttle_groups_lock);
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 322139a..fab113f 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -40,4 +40,7 @@ void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
unsigned int bytes,
bool is_write);
+void throttle_group_lock(BlockDriverState *bs);
+void throttle_group_unlock(BlockDriverState *bs);
+
#endif
--
2.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH 6/7] throttle: add the name of the ThrottleGroup to BlockDeviceInfo
2015-03-30 16:19 [Qemu-devel] [PATCH v6 0/7] Block Throttle Group Support Alberto Garcia
` (4 preceding siblings ...)
2015-03-30 16:19 ` [Qemu-devel] [PATCH 5/7] throttle: acquire the ThrottleGroup lock in bdrv_swap() Alberto Garcia
@ 2015-03-30 16:19 ` Alberto Garcia
2015-04-01 9:59 ` Fam Zheng
2015-03-30 16:19 ` [Qemu-devel] [PATCH 7/7] throttle: Update throttle infrastructure copyright Alberto Garcia
6 siblings, 1 reply; 24+ messages in thread
From: Alberto Garcia @ 2015-03-30 16:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Stefan Hajnoczi
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/qapi.c | 3 +++
hmp.c | 6 ++++--
qapi/block-core.json | 4 +++-
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 65af057..ee823d1 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -91,6 +91,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
info->has_iops_size = cfg.op_size;
info->iops_size = cfg.op_size;
+
+ info->has_group = true;
+ info->group = g_strdup(throttle_group_get_name(bs));
}
info->write_threshold = bdrv_write_threshold_get(bs);
diff --git a/hmp.c b/hmp.c
index 50f30f2..759fb86 100644
--- a/hmp.c
+++ b/hmp.c
@@ -375,7 +375,8 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
" iops_max=%" PRId64
" iops_rd_max=%" PRId64
" iops_wr_max=%" PRId64
- " iops_size=%" PRId64 "\n",
+ " iops_size=%" PRId64
+ " group=%s\n",
inserted->bps,
inserted->bps_rd,
inserted->bps_wr,
@@ -388,7 +389,8 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
inserted->iops_max,
inserted->iops_rd_max,
inserted->iops_wr_max,
- inserted->iops_size);
+ inserted->iops_size,
+ inserted->group);
}
if (verbose) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d8211b7..0057f60 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -259,6 +259,8 @@
#
# @iops_size: #optional an I/O size in bytes (Since 1.7)
#
+# @group: #optional throttle group name (Since 2.3)
+#
# @cache: the cache mode used for the block device (since: 2.3)
#
# @write_threshold: configured write threshold for the device.
@@ -278,7 +280,7 @@
'*bps_max': 'int', '*bps_rd_max': 'int',
'*bps_wr_max': 'int', '*iops_max': 'int',
'*iops_rd_max': 'int', '*iops_wr_max': 'int',
- '*iops_size': 'int', 'cache': 'BlockdevCacheInfo',
+ '*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo',
'write_threshold': 'int' } }
##
--
2.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] throttle: add the name of the ThrottleGroup to BlockDeviceInfo
2015-03-30 16:19 ` [Qemu-devel] [PATCH 6/7] throttle: add the name of the ThrottleGroup to BlockDeviceInfo Alberto Garcia
@ 2015-04-01 9:59 ` Fam Zheng
2015-04-01 10:06 ` Alberto Garcia
0 siblings, 1 reply; 24+ messages in thread
From: Fam Zheng @ 2015-04-01 9:59 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Mon, 03/30 19:19, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block/qapi.c | 3 +++
> hmp.c | 6 ++++--
> qapi/block-core.json | 4 +++-
> 3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 65af057..ee823d1 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -91,6 +91,9 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
>
> info->has_iops_size = cfg.op_size;
> info->iops_size = cfg.op_size;
> +
> + info->has_group = true;
> + info->group = g_strdup(throttle_group_get_name(bs));
True even if it's unset?
> }
>
> info->write_threshold = bdrv_write_threshold_get(bs);
> diff --git a/hmp.c b/hmp.c
> index 50f30f2..759fb86 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -375,7 +375,8 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
> " iops_max=%" PRId64
> " iops_rd_max=%" PRId64
> " iops_wr_max=%" PRId64
> - " iops_size=%" PRId64 "\n",
> + " iops_size=%" PRId64
> + " group=%s\n",
> inserted->bps,
> inserted->bps_rd,
> inserted->bps_wr,
> @@ -388,7 +389,8 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
> inserted->iops_max,
> inserted->iops_rd_max,
> inserted->iops_wr_max,
> - inserted->iops_size);
> + inserted->iops_size,
> + inserted->group);
> }
>
> if (verbose) {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d8211b7..0057f60 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -259,6 +259,8 @@
> #
> # @iops_size: #optional an I/O size in bytes (Since 1.7)
> #
> +# @group: #optional throttle group name (Since 2.3)
Will probably be 2.4.
Fam
> +#
> # @cache: the cache mode used for the block device (since: 2.3)
> #
> # @write_threshold: configured write threshold for the device.
> @@ -278,7 +280,7 @@
> '*bps_max': 'int', '*bps_rd_max': 'int',
> '*bps_wr_max': 'int', '*iops_max': 'int',
> '*iops_rd_max': 'int', '*iops_wr_max': 'int',
> - '*iops_size': 'int', 'cache': 'BlockdevCacheInfo',
> + '*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo',
> 'write_threshold': 'int' } }
>
> ##
> --
> 2.1.4
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] throttle: add the name of the ThrottleGroup to BlockDeviceInfo
2015-04-01 9:59 ` Fam Zheng
@ 2015-04-01 10:06 ` Alberto Garcia
2015-04-01 14:05 ` Fam Zheng
0 siblings, 1 reply; 24+ messages in thread
From: Alberto Garcia @ 2015-04-01 10:06 UTC (permalink / raw)
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Wed, Apr 01, 2015 at 05:59:40PM +0800, Fam Zheng wrote:
> > info->has_iops_size = cfg.op_size;
> > info->iops_size = cfg.op_size;
> > +
> > + info->has_group = true;
> > + info->group = g_strdup(throttle_group_get_name(bs));
>
> True even if it's unset?
Note that that's inside an "if (bs->io_limits_enabled)" block.
io_limits_enabled is set immediately after adding a BDS to a
throttling group, and that requires a name.
> > +# @group: #optional throttle group name (Since 2.3)
>
> Will probably be 2.4.
Right.
Berto
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] throttle: add the name of the ThrottleGroup to BlockDeviceInfo
2015-04-01 10:06 ` Alberto Garcia
@ 2015-04-01 14:05 ` Fam Zheng
0 siblings, 0 replies; 24+ messages in thread
From: Fam Zheng @ 2015-04-01 14:05 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Wed, 04/01 12:06, Alberto Garcia wrote:
> On Wed, Apr 01, 2015 at 05:59:40PM +0800, Fam Zheng wrote:
>
> > > info->has_iops_size = cfg.op_size;
> > > info->iops_size = cfg.op_size;
> > > +
> > > + info->has_group = true;
> > > + info->group = g_strdup(throttle_group_get_name(bs));
> >
> > True even if it's unset?
>
> Note that that's inside an "if (bs->io_limits_enabled)" block.
Then this is OK :)
Fam
>
> io_limits_enabled is set immediately after adding a BDS to a
> throttling group, and that requires a name.
>
> > > +# @group: #optional throttle group name (Since 2.3)
> >
> > Will probably be 2.4.
>
> Right.
>
> Berto
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 7/7] throttle: Update throttle infrastructure copyright
2015-03-30 16:19 [Qemu-devel] [PATCH v6 0/7] Block Throttle Group Support Alberto Garcia
` (5 preceding siblings ...)
2015-03-30 16:19 ` [Qemu-devel] [PATCH 6/7] throttle: add the name of the ThrottleGroup to BlockDeviceInfo Alberto Garcia
@ 2015-03-30 16:19 ` Alberto Garcia
6 siblings, 0 replies; 24+ messages in thread
From: Alberto Garcia @ 2015-03-30 16:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Stefan Hajnoczi
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
include/qemu/throttle.h | 8 +++++---
util/throttle.c | 8 +++++---
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 2c560db..5af76f0 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -1,10 +1,12 @@
/*
* QEMU throttling infrastructure
*
- * Copyright (C) Nodalink, SARL. 2013
+ * Copyright (C) Nodalink, EURL. 2013-2014
+ * Copyright (C) Igalia, S.L. 2015
*
- * Author:
- * Benoît Canet <benoit.canet@irqsave.net>
+ * Authors:
+ * Benoît Canet <benoit.canet@nodalink.com>
+ * Alberto Garcia <berto@igalia.com>
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
diff --git a/util/throttle.c b/util/throttle.c
index d76a48e..706c131 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -1,10 +1,12 @@
/*
* QEMU throttling infrastructure
*
- * Copyright (C) Nodalink, SARL. 2013
+ * Copyright (C) Nodalink, EURL. 2013-2014
+ * Copyright (C) Igalia, S.L. 2015
*
- * Author:
- * Benoît Canet <benoit.canet@irqsave.net>
+ * Authors:
+ * Benoît Canet <benoit.canet@nodalink.com>
+ * Alberto Garcia <berto@igalia.com>
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License as
--
2.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread