* [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support
@ 2015-02-13 16:06 Alberto Garcia
2015-02-13 16:06 ` [Qemu-devel] [PATCH 1/9] throttle: Extract timers from ThrottleState into a separate ThrottleTimers structure Alberto Garcia
` (9 more replies)
0 siblings, 10 replies; 34+ messages in thread
From: Alberto Garcia @ 2015-02-13 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Stefan Hajnoczi
Hello,
after having agreed it with him, I'm taking over Benoît Canet's work
to add support for throttle groups for block devices.
This was the last version of the patches, as written by him:
https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg00727.html
I rebased the patches and made a few changes, including some of the
suggestions that were proposed in the mailing list back then from Fam
Zheng and Eric Blake:
- The main function, bdrv_io_limits_update_group(), has been
significantly rewritten. The algorithm is essentially the same but
the code is hopefully more readable and easy to understand now.
- I replaced the magic numbers in test_groups() with actual objects
allocated using bdrv_new().
- I bumped the version number in the documentation changes from 2.2 to
2.3.
- I fixed a few typos and made other minor style changes.
- I added the name of the throttle group to BlockDeviceInfo, so now it
can be seen from the monitor.
I kept the authorship of the original patches except the one that I
had to modify substantially.
Regards,
Berto
Alberto Garcia (2):
throttle: Add throttle group support
throttle: add name of ThrottleGroup to BlockDeviceInfo
Benoît Canet (7):
throttle: Extract timers from ThrottleState into a separate
ThrottleTimers structure
throttle: Add throttle group infrastructure
throttle: Add throttle group infrastructure tests
throttle: Prepare to have multiple timers for one ThrottleState
throttle: Add a way to know if throttle_schedule_timer had armed a
timer
throttle: Add a way to fire one of the timers asap like a bottom half
throttle: Update throttle infrastructure copyright
block.c | 226 ++++++++++++++++++++++++++++++++++------
block/Makefile.objs | 1 +
block/qapi.c | 12 ++-
block/throttle-groups.c | 206 ++++++++++++++++++++++++++++++++++++
blockdev.c | 19 +++-
hmp.c | 10 +-
include/block/block.h | 3 +-
include/block/block_int.h | 9 +-
include/block/throttle-groups.h | 45 ++++++++
include/qemu/throttle.h | 48 ++++++---
qapi/block-core.json | 9 +-
qemu-options.hx | 1 +
qmp-commands.hx | 3 +-
tests/test-throttle.c | 142 ++++++++++++++++++-------
util/throttle.c | 106 ++++++++++++-------
15 files changed, 704 insertions(+), 136 deletions(-)
create mode 100644 block/throttle-groups.c
create mode 100644 include/block/throttle-groups.h
--
2.1.4
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 1/9] throttle: Extract timers from ThrottleState into a separate ThrottleTimers structure
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
@ 2015-02-13 16:06 ` Alberto Garcia
2015-03-03 16:03 ` Stefan Hajnoczi
2015-02-13 16:06 ` [Qemu-devel] [PATCH 2/9] throttle: Add throttle group infrastructure Alberto Garcia
` (8 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-02-13 16:06 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>
---
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 210fd5f..b7469a7 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;
}
@@ -2050,6 +2054,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;
@@ -2111,7 +2118,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;
@@ -2128,7 +2135,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') {
@@ -5785,7 +5792,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);
@@ -5821,7 +5828,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 7ad1950..c98acb6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -360,6 +360,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] 34+ messages in thread
* [Qemu-devel] [PATCH 2/9] throttle: Add throttle group infrastructure
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
2015-02-13 16:06 ` [Qemu-devel] [PATCH 1/9] throttle: Extract timers from ThrottleState into a separate ThrottleTimers structure Alberto Garcia
@ 2015-02-13 16:06 ` Alberto Garcia
2015-03-03 16:38 ` Stefan Hajnoczi
2015-02-13 16:06 ` [Qemu-devel] [PATCH 3/9] throttle: Add throttle group infrastructure tests Alberto Garcia
` (7 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-02-13 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Benoît Canet, Stefan Hajnoczi
From: Benoît Canet <benoit.canet@nodalink.com>
The throttle_group_incref increment the refcount of a throttle group given it's
name and return the associated throttle state.
The throttle_group_unref is the mirror function for cleaning up.
Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/Makefile.objs | 1 +
block/throttle-groups.c | 212 ++++++++++++++++++++++++++++++++++++++++
include/block/block_int.h | 1 +
include/block/throttle-groups.h | 45 +++++++++
4 files changed, 259 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..ea5baca
--- /dev/null
+++ b/block/throttle-groups.c
@@ -0,0 +1,212 @@
+/*
+ * QEMU block throttling group infrastructure
+ *
+ * Copyright (C) Nodalink, EURL. 2014
+ *
+ * Author:
+ * Benoît Canet <benoit.canet@nodalink.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"
+#include "qemu/queue.h"
+#include "qemu/thread.h"
+
+typedef struct ThrottleGroup {
+ char name[32];
+ ThrottleState ts;
+ uint64_t refcount;
+ QTAILQ_ENTRY(ThrottleGroup) list;
+ QLIST_HEAD(, BlockDriverState) head;
+ BlockDriverState *tokens[2]; /* current round-robin tokens */
+ QemuMutex lock; /* Used to synchronize all elements belonging to a group */
+} ThrottleGroup;
+
+static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
+ QTAILQ_HEAD_INITIALIZER(throttle_groups);
+
+/* increments a ThrottleGroup reference count given it's name
+ *
+ * If no ThrottleGroup is found with the given name a new one is created.
+ *
+ * @name: the name of the ThrottleGroup
+ * @ret: the ThrottleGroup's ThrottleState address
+ */
+ThrottleState *throttle_group_incref(const char *name)
+{
+ ThrottleGroup *tg;
+
+ /* return the correct ThrottleState if a group with this name exists */
+ QTAILQ_FOREACH(tg, &throttle_groups, list) {
+ /* group not found -> continue */
+ if (strcmp(name, tg->name)) {
+ continue;
+ }
+ /* group found -> increment it's refcount and return ThrottleState */
+ tg->refcount++;
+ return &tg->ts;
+ }
+
+ /* throttle group not found -> prepare new entry */
+ tg = g_new0(ThrottleGroup, 1);
+ pstrcpy(tg->name, sizeof(tg->name), name);
+ qemu_mutex_init(&tg->lock);
+ throttle_init(&tg->ts);
+ QLIST_INIT(&tg->head);
+ tg->refcount = 1;
+
+ /* insert new entry in the list */
+ QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
+
+ /* return newly allocated ThrottleState */
+ return &tg->ts;
+}
+
+/* decrement a ThrottleGroup given it's ThrottleState address
+ *
+ * When the refcount reach zero the ThrottleGroup is destroyed
+ *
+ * @ts: The ThrottleState address belonging to the ThrottleGroup to unref
+ * @ret: true on success else false
+ */
+bool throttle_group_unref(ThrottleState *ts)
+{
+ ThrottleGroup *tg;
+ bool found = false;
+
+ /* Find the ThrottleGroup of the given ThrottleState */
+ QTAILQ_FOREACH(tg, &throttle_groups, list) {
+ /* correct group found stop iterating */
+ if (&tg->ts == ts) {
+ qemu_mutex_lock(&tg->lock);
+ found = true;
+ break;
+ }
+ }
+
+ /* If the ThrottleState was not found something is seriously broken */
+ if (!found) {
+ return false;
+ }
+
+ tg->refcount--;
+
+ /* If ThrottleGroup is used keep it. */
+ if (tg->refcount) {
+ qemu_mutex_unlock(&tg->lock);
+ return true;
+ }
+
+ /* Else destroy it */
+ QTAILQ_REMOVE(&throttle_groups, tg, list);
+ qemu_mutex_unlock(&tg->lock);
+ qemu_mutex_destroy(&tg->lock);
+ g_free(tg);
+ return true;
+}
+
+/* Compare a name with a given ThrottleState group name
+ *
+ * @ts: the throttle state whose group we are inspecting
+ * @name: the name to compare
+ * @ret: true if names are equal else false
+ */
+bool throttle_group_compare(ThrottleState *ts, const char *name)
+{
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+
+ if (!name) {
+ return false;
+ }
+
+ return !strcmp(name, tg->name);
+}
+
+/* Register a BlockDriverState in the doubly linked list
+ *
+ * @ts: the ThrottleState the code is working on
+ * @bs: the BlockDriverState to insert
+ */
+void throttle_group_register_bs(ThrottleState *ts, BlockDriverState *bs)
+{
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ QLIST_INSERT_HEAD(&tg->head, bs, round_robin);
+}
+
+/* Return the next BlockDriverState in the round-robin sequence
+ *
+ * This takes care of simulating a circular list
+ *
+ * @bs: the input current BlockDriverState
+ * @ret: the next BlockDriverState in the sequence
+ */
+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;
+}
+
+/* Used to set a token BlockDriverState into a ThrottleState's ThrottleGroup
+ *
+ * @ts: the ThrottleState the code is working on
+ * @token: the token BlockDriverState to set
+ * @is_write: true if we are talking about write operations else false
+ */
+void throttle_group_set_token(ThrottleState *ts,
+ BlockDriverState *token,
+ bool is_write)
+{
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ tg->tokens[is_write] = token;
+}
+
+/* Used to get the token BlockDriverState of a ThrottleState's ThrottleGroup
+ *
+ * @ts: the ThrottleState the code is working on
+ * @ret: the token BlockDriverState that is retrieved
+ * @is_write: true if we are talking about write operations else false
+ */
+BlockDriverState *throttle_group_token(ThrottleState *ts, bool is_write)
+{
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ return tg->tokens[is_write];
+}
+
+/* Used to lock a ThrottleState's ThrottleGroup
+ *
+ * @ts: the ThrottleState the code is working on
+ */
+void throttle_group_lock(ThrottleState *ts)
+{
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ qemu_mutex_lock(&tg->lock);
+}
+
+/* Used to unlock a ThrottleState's ThrottleGroup
+ *
+ * @ts: the ThrottleState the code is working on
+ */
+void throttle_group_unlock(ThrottleState *ts)
+{
+ ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+ qemu_mutex_unlock(&tg->lock);
+}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c98acb6..28d754c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -363,6 +363,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..d000067
--- /dev/null
+++ b/include/block/throttle-groups.h
@@ -0,0 +1,45 @@
+/*
+ * QEMU block throttling group infrastructure
+ *
+ * Copyright (C) Nodalink, EURL. 2014
+ *
+ * Author:
+ * Benoît Canet <benoit.canet@nodalink.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"
+
+ThrottleState *throttle_group_incref(const char *name);
+bool throttle_group_unref(ThrottleState *ts);
+
+bool throttle_group_compare(ThrottleState *ts, const char *name);
+
+void throttle_group_register_bs(ThrottleState *ts, BlockDriverState *bs);
+BlockDriverState *throttle_group_next_bs(BlockDriverState *bs);
+
+void throttle_group_set_token(ThrottleState *ts,
+ BlockDriverState *token,
+ bool is_write);
+BlockDriverState *throttle_group_token(ThrottleState *ts, bool is_write);
+
+void throttle_group_lock(ThrottleState *ts);
+void throttle_group_unlock(ThrottleState *ts);
+
+#endif
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 3/9] throttle: Add throttle group infrastructure tests
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
2015-02-13 16:06 ` [Qemu-devel] [PATCH 1/9] throttle: Extract timers from ThrottleState into a separate ThrottleTimers structure Alberto Garcia
2015-02-13 16:06 ` [Qemu-devel] [PATCH 2/9] throttle: Add throttle group infrastructure Alberto Garcia
@ 2015-02-13 16:06 ` Alberto Garcia
2015-02-13 16:06 ` [Qemu-devel] [PATCH 4/9] throttle: Prepare to have multiple timers for one ThrottleState Alberto Garcia
` (6 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Alberto Garcia @ 2015-02-13 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Benoît Canet, Stefan Hajnoczi
From: Benoît Canet <benoit.canet@nodalink.com>
Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
tests/test-throttle.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 458f577..0b649b1 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -15,6 +15,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,6 +501,60 @@ static void test_accounting(void)
(64.0 / 13)));
}
+static void test_groups(void)
+{
+ bool removed;
+
+ ThrottleState *ts_foo, *ts_bar, *tmp;
+ BlockDriverState *bdrv1, *bdrv2;
+
+ bdrv1 = bdrv_new();
+ bdrv2 = bdrv_new();
+
+ ts_bar = throttle_group_incref("bar");
+ ts_foo = throttle_group_incref("foo");
+ tmp = throttle_group_incref("foo");
+
+ throttle_group_set_token(ts_bar, bdrv1, false);
+ throttle_group_set_token(tmp, bdrv2, true);
+
+ g_assert(tmp == ts_foo);
+
+ tmp = throttle_group_incref("bar");
+ g_assert(tmp == ts_bar);
+
+ tmp = throttle_group_incref("bar");
+ g_assert(tmp == ts_bar);
+
+ g_assert(throttle_group_token(ts_bar, false) == bdrv1);
+ g_assert(throttle_group_token(ts_foo, true) == bdrv2);
+
+ removed = throttle_group_unref(ts_foo);
+ g_assert(removed);
+ removed = throttle_group_unref(ts_bar);
+ g_assert(removed);
+
+ g_assert(throttle_group_token(ts_foo, true) == bdrv2);
+
+ removed = throttle_group_unref(ts_foo);
+ g_assert(removed);
+ removed = throttle_group_unref(ts_bar);
+ g_assert(removed);
+
+ /* "foo" group should be destroyed when reaching this */
+ removed = throttle_group_unref(ts_foo);
+ g_assert(!removed);
+
+ g_assert(throttle_group_token(ts_bar, false) == bdrv1);
+
+ removed = throttle_group_unref(ts_bar);
+ g_assert(removed);
+
+ /* "bar" group should be destroyed when reaching this */
+ removed = throttle_group_unref(ts_bar);
+ g_assert(!removed);
+}
+
int main(int argc, char **argv)
{
GSource *src;
@@ -533,6 +588,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] 34+ messages in thread
* [Qemu-devel] [PATCH 4/9] throttle: Prepare to have multiple timers for one ThrottleState
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
` (2 preceding siblings ...)
2015-02-13 16:06 ` [Qemu-devel] [PATCH 3/9] throttle: Add throttle group infrastructure tests Alberto Garcia
@ 2015-02-13 16:06 ` Alberto Garcia
2015-03-03 16:50 ` Stefan Hajnoczi
2015-02-13 16:06 ` [Qemu-devel] [PATCH 5/9] throttle: Add a way to know if throttle_schedule_timer had armed a timer Alberto Garcia
` (5 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-02-13 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Benoît Canet, Stefan Hajnoczi
From: Benoît Canet <benoit.canet@nodalink.com>
This patch transform the timer_pending call into two boolean values in the
ThrottleState structure.
This way we are sure that when multiple timers will be used only
one can be armed at a time.
Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 2 ++
include/qemu/throttle.h | 3 +++
util/throttle.c | 16 +++++++++++++---
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index b7469a7..fb45c6e 100644
--- a/block.c
+++ b/block.c
@@ -169,12 +169,14 @@ void bdrv_io_limits_disable(BlockDriverState *bs)
static void bdrv_throttle_read_timer_cb(void *opaque)
{
BlockDriverState *bs = opaque;
+ throttle_timer_fired(&bs->throttle_state, false);
qemu_co_enter_next(&bs->throttled_reqs[0]);
}
static void bdrv_throttle_write_timer_cb(void *opaque)
{
BlockDriverState *bs = opaque;
+ throttle_timer_fired(&bs->throttle_state, true);
qemu_co_enter_next(&bs->throttled_reqs[1]);
}
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 2c560db..546ba12 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -65,6 +65,7 @@ typedef struct ThrottleConfig {
typedef struct ThrottleState {
ThrottleConfig cfg; /* configuration */
int64_t previous_leak; /* timestamp of the last leak done */
+ bool any_timer_armed[2]; /* is any timer armed for this throttle state */
} ThrottleState;
typedef struct ThrottleTimers {
@@ -125,6 +126,8 @@ bool throttle_schedule_timer(ThrottleState *ts,
ThrottleTimers *tt,
bool is_write);
+void throttle_timer_fired(ThrottleState *ts, bool is_write);
+
void throttle_account(ThrottleState *ts, bool is_write, uint64_t size);
#endif
diff --git a/util/throttle.c b/util/throttle.c
index d76a48e..71770d3 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -390,16 +390,26 @@ bool throttle_schedule_timer(ThrottleState *ts,
return false;
}
- /* request throttled and timer pending -> do nothing */
- if (timer_pending(tt->timers[is_write])) {
+ /* request throttled and any timer pending -> do nothing */
+ if (ts->any_timer_armed[is_write]) {
return true;
}
- /* request throttled and timer not pending -> arm timer */
+ ts->any_timer_armed[is_write] = true;
timer_mod(tt->timers[is_write], next_timestamp);
return true;
}
+/* Remember that now timers are currently armed
+ *
+ * @ts: the throttle state we are working on
+ * @is_write: the type of operation (read/write)
+ */
+void throttle_timer_fired(ThrottleState *ts, bool is_write)
+{
+ ts->any_timer_armed[is_write] = false;
+}
+
/* do the accounting for this operation
*
* @is_write: the type of operation (read/write)
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 5/9] throttle: Add a way to know if throttle_schedule_timer had armed a timer
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
` (3 preceding siblings ...)
2015-02-13 16:06 ` [Qemu-devel] [PATCH 4/9] throttle: Prepare to have multiple timers for one ThrottleState Alberto Garcia
@ 2015-02-13 16:06 ` Alberto Garcia
2015-02-13 16:06 ` [Qemu-devel] [PATCH 6/9] throttle: Add a way to fire one of the timers asap like a bottom half Alberto Garcia
` (4 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Alberto Garcia @ 2015-02-13 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Benoît Canet, Stefan Hajnoczi
From: Benoît Canet <benoit.canet@nodalink.com>
This will be needed by the group throttling patches for the algorithm to be
accurate.
Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 7 +++++--
include/qemu/throttle.h | 3 ++-
util/throttle.c | 12 +++++++-----
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/block.c b/block.c
index fb45c6e..642ef04 100644
--- a/block.c
+++ b/block.c
@@ -203,10 +203,13 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
unsigned int bytes,
bool is_write)
{
+ bool armed;
+
/* does this io must wait */
bool must_wait = throttle_schedule_timer(&bs->throttle_state,
&bs->throttle_timers,
- is_write);
+ is_write,
+ &armed);
/* if must wait or any request of this type throttled queue the IO */
if (must_wait ||
@@ -220,7 +223,7 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
/* if the next request must wait -> do nothing */
if (throttle_schedule_timer(&bs->throttle_state, &bs->throttle_timers,
- is_write)) {
+ is_write, &armed)) {
return;
}
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 546ba12..69aa7c2 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -124,7 +124,8 @@ void throttle_get_config(ThrottleState *ts, ThrottleConfig *cfg);
/* usage */
bool throttle_schedule_timer(ThrottleState *ts,
ThrottleTimers *tt,
- bool is_write);
+ bool is_write,
+ bool *armed);
void throttle_timer_fired(ThrottleState *ts, bool is_write);
diff --git a/util/throttle.c b/util/throttle.c
index 71770d3..f5cd564 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -374,11 +374,13 @@ void throttle_get_config(ThrottleState *ts, ThrottleConfig *cfg)
*/
bool throttle_schedule_timer(ThrottleState *ts,
ThrottleTimers *tt,
- bool is_write)
+ bool is_write,
+ bool *armed)
{
int64_t now = qemu_clock_get_ns(tt->clock_type);
int64_t next_timestamp;
bool must_wait;
+ *armed = false;
must_wait = throttle_compute_timer(ts,
is_write,
@@ -391,12 +393,12 @@ bool throttle_schedule_timer(ThrottleState *ts,
}
/* request throttled and any timer pending -> do nothing */
- if (ts->any_timer_armed[is_write]) {
- return true;
+ if (!ts->any_timer_armed[is_write]) {
+ *armed = true;
+ ts->any_timer_armed[is_write] = true;
+ timer_mod(tt->timers[is_write], next_timestamp);
}
- ts->any_timer_armed[is_write] = true;
- timer_mod(tt->timers[is_write], next_timestamp);
return true;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 6/9] throttle: Add a way to fire one of the timers asap like a bottom half
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
` (4 preceding siblings ...)
2015-02-13 16:06 ` [Qemu-devel] [PATCH 5/9] throttle: Add a way to know if throttle_schedule_timer had armed a timer Alberto Garcia
@ 2015-02-13 16:06 ` Alberto Garcia
2015-03-03 17:08 ` Stefan Hajnoczi
2015-02-13 16:06 ` [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support Alberto Garcia
` (3 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-02-13 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Benoît Canet, Stefan Hajnoczi
From: Benoît Canet <benoit.canet@nodalink.com>
This will be needed by the group throttling algorithm.
Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
include/qemu/throttle.h | 2 ++
util/throttle.c | 11 +++++++++++
2 files changed, 13 insertions(+)
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 69aa7c2..f846e5a 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -127,6 +127,8 @@ bool throttle_schedule_timer(ThrottleState *ts,
bool is_write,
bool *armed);
+void throttle_fire_timer(ThrottleTimers *tt, bool is_write);
+
void throttle_timer_fired(ThrottleState *ts, bool is_write);
void throttle_account(ThrottleState *ts, bool is_write, uint64_t size);
diff --git a/util/throttle.c b/util/throttle.c
index f5cd564..4219ace 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -402,6 +402,17 @@ bool throttle_schedule_timer(ThrottleState *ts,
return true;
}
+/* Schedule a throttle timer like a BH
+ *
+ * @tt: The timers structure
+ * @is_write: the type of operation (read/write)
+ */
+void throttle_fire_timer(ThrottleTimers *tt, bool is_write)
+{
+ int64_t now = qemu_clock_get_ns(tt->clock_type);
+ timer_mod(tt->timers[is_write], now + 1);
+}
+
/* Remember that now timers are currently armed
*
* @ts: the throttle state we are working on
--
2.1.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
` (5 preceding siblings ...)
2015-02-13 16:06 ` [Qemu-devel] [PATCH 6/9] throttle: Add a way to fire one of the timers asap like a bottom half Alberto Garcia
@ 2015-02-13 16:06 ` Alberto Garcia
2015-02-24 16:45 ` Eric Blake
2015-03-03 21:00 ` Stefan Hajnoczi
2015-02-13 16:06 ` [Qemu-devel] [PATCH 8/9] throttle: Update throttle infrastructure copyright Alberto Garcia
` (2 subsequent siblings)
9 siblings, 2 replies; 34+ messages in thread
From: Alberto Garcia @ 2015-02-13 16:06 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 compute if a wait must be done and arm 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 | 212 +++++++++++++++++++++++++++++++++++++++-------
block/qapi.c | 7 +-
block/throttle-groups.c | 2 +-
blockdev.c | 19 ++++-
hmp.c | 4 +-
include/block/block.h | 3 +-
include/block/block_int.h | 9 +-
qapi/block-core.json | 5 +-
qemu-options.hx | 1 +
qmp-commands.hx | 3 +-
10 files changed, 220 insertions(+), 45 deletions(-)
diff --git a/block.c b/block.c
index 642ef04..625f1c8 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,9 @@ void bdrv_set_io_limits(BlockDriverState *bs,
{
int i;
- throttle_config(&bs->throttle_state, &bs->throttle_timers, cfg);
+ throttle_group_lock(bs->throttle_state);
+ throttle_config(bs->throttle_state, &bs->throttle_timers, cfg);
+ throttle_group_unlock(bs->throttle_state);
for (i = 0; i < 2; i++) {
qemu_co_enter_next(&bs->throttled_reqs[i]);
@@ -157,34 +160,99 @@ static bool bdrv_start_throttled_reqs(BlockDriverState *bs)
return drained;
}
+static void bdrv_throttle_group_add(BlockDriverState *bs)
+{
+ int i;
+ BlockDriverState *token;
+
+ for (i = 0; i < 2; i++) {
+ /* Get the BlockDriverState having the round robin token */
+ token = throttle_group_token(bs->throttle_state, i);
+
+ /* If the ThrottleGroup is new set the current BlockDriverState as
+ * token
+ */
+ if (!token) {
+ throttle_group_set_token(bs->throttle_state, bs, i);
+ }
+
+ }
+
+ throttle_group_register_bs(bs->throttle_state, bs);
+}
+
+static void bdrv_throttle_group_remove(BlockDriverState *bs)
+{
+ BlockDriverState *token;
+ int i;
+
+ for (i = 0; i < 2; i++) {
+ /* Get the BlockDriverState having the round robin token */
+ token = throttle_group_token(bs->throttle_state, i);
+ /* if this bs is the current token set the next bs as token */
+ if (token == bs) {
+ token = throttle_group_next_bs(token);
+ /* take care of the case where bs is the only bs of the group */
+ if (token == bs) {
+ token = NULL;
+ }
+ throttle_group_set_token(bs->throttle_state, token, i);
+ }
+ }
+
+ /* remove the current bs from the list */
+ QLIST_REMOVE(bs, round_robin);
+}
+
void bdrv_io_limits_disable(BlockDriverState *bs)
{
+
+ throttle_group_lock(bs->throttle_state);
bs->io_limits_enabled = false;
+ throttle_group_unlock(bs->throttle_state);
bdrv_start_throttled_reqs(bs);
+ throttle_group_lock(bs->throttle_state);
+ bdrv_throttle_group_remove(bs);
+ throttle_group_unlock(bs->throttle_state);
+
+ throttle_group_unref(bs->throttle_state);
+ bs->throttle_state = NULL;
+
throttle_timers_destroy(&bs->throttle_timers);
}
static void bdrv_throttle_read_timer_cb(void *opaque)
{
BlockDriverState *bs = opaque;
- throttle_timer_fired(&bs->throttle_state, false);
+
+ throttle_group_lock(bs->throttle_state);
+ throttle_timer_fired(bs->throttle_state, false);
+ throttle_group_unlock(bs->throttle_state);
+
qemu_co_enter_next(&bs->throttled_reqs[0]);
}
static void bdrv_throttle_write_timer_cb(void *opaque)
{
BlockDriverState *bs = opaque;
- throttle_timer_fired(&bs->throttle_state, true);
+
+ throttle_group_lock(bs->throttle_state);
+ throttle_timer_fired(bs->throttle_state, true);
+ throttle_group_unlock(bs->throttle_state);
+
qemu_co_enter_next(&bs->throttled_reqs[1]);
}
/* 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);
+ bs->throttle_state = throttle_group_incref(group ? group : bdrv_get_device_name(bs));
+
+ throttle_group_lock(bs->throttle_state);
+ bdrv_throttle_group_add(bs);
throttle_timers_init(&bs->throttle_timers,
bdrv_get_aio_context(bs),
QEMU_CLOCK_VIRTUAL,
@@ -192,6 +260,53 @@ void bdrv_io_limits_enable(BlockDriverState *bs)
bdrv_throttle_write_timer_cb,
bs);
bs->io_limits_enabled = true;
+ throttle_group_unlock(bs->throttle_state);
+}
+
+void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group)
+{
+ /* this bs is not part of any group */
+ if (!bs->throttle_state) {
+ return;
+ }
+
+ /* this bs is a part of the same group than the one we want */
+ if (throttle_group_compare(bs->throttle_state, group)) {
+ return;
+ }
+
+ /* need to change the group this bs belong to */
+ bdrv_io_limits_disable(bs);
+ bdrv_io_limits_enable(bs, group);
+}
+
+/* This implement the round robin policy and must be called under ThrottleGroup
+ * lock
+ */
+static BlockDriverState *bdrv_next_throttle_token(BlockDriverState *bs,
+ bool is_write)
+{
+ BlockDriverState *token, *start;
+
+ start = token = throttle_group_token(bs->throttle_state, is_write);
+
+ /* get next bs round in round robin style */
+ token = throttle_group_next_bs(token);
+ while (token != start &&
+ qemu_co_queue_empty(&token->throttled_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 &&
+ qemu_co_queue_empty(&token->throttled_reqs[is_write])) {
+ token = bs;
+ }
+
+ return token;
}
/* This function makes an IO wait if needed
@@ -204,31 +319,63 @@ static void bdrv_io_limits_intercept(BlockDriverState *bs,
bool is_write)
{
bool armed;
-
- /* does this io must wait */
- bool must_wait = throttle_schedule_timer(&bs->throttle_state,
- &bs->throttle_timers,
- is_write,
- &armed);
-
- /* 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])) {
+ bool must_wait;
+ BlockDriverState *token;
+
+ throttle_group_lock(bs->throttle_state);
+
+ /* First we check if this I/O has to be throttled.
+ * If that's the case and we need to set a new timer for that,
+ * we have to do it in the next bs according to the scheduling
+ * algorithm. In that case that bs will be used as the new
+ * token. */
+ token = bdrv_next_throttle_token(bs, is_write);
+ must_wait = throttle_schedule_timer(bs->throttle_state,
+ &token->throttle_timers,
+ is_write,
+ &armed);
+ if (armed) {
+ throttle_group_set_token(bs->throttle_state, token, is_write);
+ }
+
+ /* Wait if there's a timer set or queued requests of this type */
+ if (must_wait || !qemu_co_queue_empty(&bs->throttled_reqs[is_write])) {
+ throttle_group_unlock(bs->throttle_state);
qemu_co_queue_wait(&bs->throttled_reqs[is_write]);
- }
-
- /* the IO will be executed, do the accounting */
- throttle_account(&bs->throttle_state, is_write, bytes);
-
+ throttle_group_lock(bs->throttle_state);
+ }
+
+ /* The I/O will be executed, so do the accounting */
+ throttle_account(bs->throttle_state, is_write, bytes);
+
+ /* Now we check if there's any pending request to schedule next.
+ * If that's the case and it doesn't have to wait, we queue it for
+ * immediate execution. */
+ token = bdrv_next_throttle_token(bs, is_write);
+ if (!qemu_co_queue_empty(&token->throttled_reqs[is_write])) {
+ must_wait = throttle_schedule_timer(bs->throttle_state,
+ &token->throttle_timers,
+ is_write,
+ &armed);
+
+ if (!must_wait) {
+ /* If we don't need to do any throttling, give preference
+ * to requests from the current bs */
+ if (!qemu_co_queue_empty(&bs->throttled_reqs[is_write])) {
+ token = bs;
+ qemu_co_queue_next(&bs->throttled_reqs[is_write]);
+ } else {
+ throttle_fire_timer(&token->throttle_timers, is_write);
+ }
+ }
- /* if the next request must wait -> do nothing */
- if (throttle_schedule_timer(&bs->throttle_state, &bs->throttle_timers,
- is_write, &armed)) {
- return;
+ /* Set the new token unless a timer had already been armed */
+ if (armed || !must_wait) {
+ throttle_group_set_token(bs->throttle_state, token, is_write);
+ }
}
- /* else queue next request for execution */
- qemu_co_queue_next(&bs->throttled_reqs[is_write]);
+ throttle_group_unlock(bs->throttle_state);
}
size_t bdrv_opt_mem_align(BlockDriverState *bs)
@@ -2056,15 +2203,16 @@ 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->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;
diff --git a/block/qapi.c b/block/qapi.c
index 1808e67..9ed3e68 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,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
if (bs->io_limits_enabled) {
ThrottleConfig cfg;
- throttle_get_config(&bs->throttle_state, &cfg);
+
+ throttle_group_lock(bs->throttle_state);
+ throttle_get_config(bs->throttle_state, &cfg);
+ throttle_group_unlock(bs->throttle_state);
+
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 ea5baca..399ae5e 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -154,7 +154,7 @@ void throttle_group_register_bs(ThrottleState *ts, BlockDriverState *bs)
*/
BlockDriverState *throttle_group_next_bs(BlockDriverState *bs)
{
- ThrottleState *ts = &bs->throttle_state;
+ ThrottleState *ts = bs->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
BlockDriverState *next = QLIST_NEXT(bs, round_robin);
diff --git a/blockdev.c b/blockdev.c
index 7d34960..135cdeb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -361,6 +361,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
BlockDriver *drv = NULL;
+ 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(). */
@@ -463,6 +464,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;
@@ -518,7 +521,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
/* disk I/O throttling */
if (throttle_enabled(&cfg)) {
- bdrv_io_limits_enable(bs);
+ bdrv_io_limits_enable(bs, throttling_group);
bdrv_set_io_limits(bs, &cfg);
}
@@ -721,6 +724,8 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
{ "iops_size", "throttling.iops-size" },
+ { "group", "throttling.group" },
+
{ "readonly", "read-only" },
};
@@ -1889,7 +1894,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 +1948,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 : NULL);
} 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 : NULL);
}
if (bs->io_limits_enabled) {
@@ -3004,6 +3013,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 b47f331..47663ce 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1228,7 +1228,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 321295e..c74771f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -162,8 +162,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 28d754c..5d49ed2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -358,12 +358,13 @@ struct BlockDriverState {
/* number of in-flight serialising requests */
unsigned int serialising_in_flight;
- /* I/O throttling */
- ThrottleState throttle_state;
- ThrottleTimers throttle_timers;
- CoQueue throttled_reqs[2];
+ /* I/O throttling - following elements protected by ThrottleGroup lock */
+ ThrottleState *throttle_state;
bool io_limits_enabled;
+ CoQueue throttled_reqs[2];
QLIST_ENTRY(BlockDriverState) round_robin;
+ /* timers have their own locking */
+ ThrottleTimers throttle_timers;
/* I/O stats (display with "info blockstats"). */
BlockAcctStats stats;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a3fdaf0..563b11f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -989,6 +989,9 @@
#
# @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
#
@@ -1000,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 85ca3ad..ef8e76c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -442,6 +442,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 a85d847..2ce6af6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1704,7 +1704,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,
},
@@ -1730,6 +1730,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] 34+ messages in thread
* [Qemu-devel] [PATCH 8/9] throttle: Update throttle infrastructure copyright
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
` (6 preceding siblings ...)
2015-02-13 16:06 ` [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support Alberto Garcia
@ 2015-02-13 16:06 ` Alberto Garcia
2015-02-24 16:49 ` Eric Blake
2015-02-13 16:06 ` [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo Alberto Garcia
2015-03-03 21:07 ` [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Stefan Hajnoczi
9 siblings, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-02-13 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Benoît Canet, Stefan Hajnoczi
From: Benoît Canet <benoit.canet@nodalink.com>
Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
include/qemu/throttle.h | 4 ++--
tests/test-throttle.c | 4 ++--
util/throttle.c | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index f846e5a..6174332 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -1,10 +1,10 @@
/*
* QEMU throttling infrastructure
*
- * Copyright (C) Nodalink, SARL. 2013
+ * Copyright (C) Nodalink, EURL. 2013-2014
*
* Author:
- * Benoît Canet <benoit.canet@irqsave.net>
+ * Benoît Canet <benoit.canet@nodalink.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/tests/test-throttle.c b/tests/test-throttle.c
index 0b649b1..cf58e0e 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -1,10 +1,10 @@
/*
* Throttle infrastructure tests
*
- * Copyright Nodalink, SARL. 2013
+ * Copyright Nodalink, EURL. 2013-2014
*
* Authors:
- * Benoît Canet <benoit.canet@irqsave.net>
+ * Benoît Canet <benoit.canet@nodalink.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.
diff --git a/util/throttle.c b/util/throttle.c
index 4219ace..aa0ea65 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -1,10 +1,10 @@
/*
* QEMU throttling infrastructure
*
- * Copyright (C) Nodalink, SARL. 2013
+ * Copyright (C) Nodalink, EURL. 2013-2014
*
* Author:
- * Benoît Canet <benoit.canet@irqsave.net>
+ * Benoît Canet <benoit.canet@nodalink.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] 34+ messages in thread
* [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
` (7 preceding siblings ...)
2015-02-13 16:06 ` [Qemu-devel] [PATCH 8/9] throttle: Update throttle infrastructure copyright Alberto Garcia
@ 2015-02-13 16:06 ` Alberto Garcia
2015-02-24 16:54 ` Eric Blake
2015-03-03 21:07 ` [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Stefan Hajnoczi
9 siblings, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-02-13 16:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, Stefan Hajnoczi
Replace also throttle_group_compare() with throttle_group_get_name()
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 2 +-
block/qapi.c | 5 +++++
block/throttle-groups.c | 14 ++++----------
hmp.c | 6 ++++--
include/block/throttle-groups.h | 2 +-
qapi/block-core.json | 4 +++-
6 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/block.c b/block.c
index 625f1c8..a53cb76 100644
--- a/block.c
+++ b/block.c
@@ -271,7 +271,7 @@ void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group)
}
/* this bs is a part of the same group than the one we want */
- if (throttle_group_compare(bs->throttle_state, group)) {
+ if (!g_strcmp0(throttle_group_get_name(bs->throttle_state), group)) {
return;
}
diff --git a/block/qapi.c b/block/qapi.c
index 9ed3e68..3d61bab 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -64,9 +64,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
if (bs->io_limits_enabled) {
ThrottleConfig cfg;
+ char *group_name;
throttle_group_lock(bs->throttle_state);
throttle_get_config(bs->throttle_state, &cfg);
+ group_name = g_strdup(throttle_group_get_name(bs->throttle_state));
throttle_group_unlock(bs->throttle_state);
info->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
@@ -93,6 +95,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 = group_name;
}
info->write_threshold = bdrv_write_threshold_get(bs);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 399ae5e..98c0a5e 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -117,21 +117,15 @@ bool throttle_group_unref(ThrottleState *ts)
return true;
}
-/* Compare a name with a given ThrottleState group name
+/* Get the name from a ThrottleState's ThrottleGroup
*
* @ts: the throttle state whose group we are inspecting
- * @name: the name to compare
- * @ret: true if names are equal else false
+ * @ret: the name of the group
*/
-bool throttle_group_compare(ThrottleState *ts, const char *name)
+const char *throttle_group_get_name(ThrottleState *ts)
{
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
-
- if (!name) {
- return false;
- }
-
- return !strcmp(name, tg->name);
+ return tg->name;
}
/* Register a BlockDriverState in the doubly linked list
diff --git a/hmp.c b/hmp.c
index 47663ce..ae3ef15 100644
--- a/hmp.c
+++ b/hmp.c
@@ -369,7 +369,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,
@@ -382,7 +383,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/include/block/throttle-groups.h b/include/block/throttle-groups.h
index d000067..8f8d285 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -29,7 +29,7 @@
ThrottleState *throttle_group_incref(const char *name);
bool throttle_group_unref(ThrottleState *ts);
-bool throttle_group_compare(ThrottleState *ts, const char *name);
+const char *throttle_group_get_name(ThrottleState *ts);
void throttle_group_register_bs(ThrottleState *ts, BlockDriverState *bs);
BlockDriverState *throttle_group_next_bs(BlockDriverState *bs);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 563b11f..5653924 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -255,6 +255,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.
@@ -274,7 +276,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] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support
2015-02-13 16:06 ` [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support Alberto Garcia
@ 2015-02-24 16:45 ` Eric Blake
2015-02-24 16:47 ` Eric Blake
2015-03-03 21:00 ` Stefan Hajnoczi
1 sibling, 1 reply; 34+ messages in thread
From: Eric Blake @ 2015-02-24 16:45 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]
On 02/13/2015 09:06 AM, Alberto Garcia wrote:
> 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 compute if a wait must be done and arm 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 | 212 +++++++++++++++++++++++++++++++++++++++-------
> block/qapi.c | 7 +-
> block/throttle-groups.c | 2 +-
> blockdev.c | 19 ++++-
> hmp.c | 4 +-
> include/block/block.h | 3 +-
> include/block/block_int.h | 9 +-
> qapi/block-core.json | 5 +-
> qemu-options.hx | 1 +
> qmp-commands.hx | 3 +-
Just a qapi review for now...
> +++ b/qapi/block-core.json
> @@ -989,6 +989,9 @@
> #
> # @iops_size: #optional an I/O size in bytes (Since 1.7)
> #
> +#
Why the extra blank line?
> +# @group: #optional throttle group name (Since 2.3)
> +#
> # Returns: Nothing on success
> # If @device is not a valid block device, DeviceNotFound
> #
> @@ -1000,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' } }
What is the default if no 'group' is specified, unthrottled?
I hate write-only interfaces. What is the corresponding 'query-*'
command that I can invoke to learn which group, if any, a node is
associated with?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support
2015-02-24 16:45 ` Eric Blake
@ 2015-02-24 16:47 ` Eric Blake
0 siblings, 0 replies; 34+ messages in thread
From: Eric Blake @ 2015-02-24 16:47 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 929 bytes --]
On 02/24/2015 09:45 AM, Eric Blake wrote:
> On 02/13/2015 09:06 AM, Alberto Garcia wrote:
>> The throttle group support use a cooperative round robin scheduling algorithm.
>>
>> qapi/block-core.json | 5 +-
>> qemu-options.hx | 1 +
>> qmp-commands.hx | 3 +-
>
> Just a qapi review for now...
>
>
> I hate write-only interfaces. What is the corresponding 'query-*'
> command that I can invoke to learn which group, if any, a node is
> associated with?
Okay, I hit send too soon. I see this is part of BlockDeviceInfo, which
is in turn part of BlockInfo returned by 'query-block'. So this
addition is the output side, and there is nothing that yet exists to
associate a node with a throttle group on the input side (assuming that
comes later in 8/9 or 9/9).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 8/9] throttle: Update throttle infrastructure copyright
2015-02-13 16:06 ` [Qemu-devel] [PATCH 8/9] throttle: Update throttle infrastructure copyright Alberto Garcia
@ 2015-02-24 16:49 ` Eric Blake
2015-02-24 20:21 ` Benoît Canet
0 siblings, 1 reply; 34+ messages in thread
From: Eric Blake @ 2015-02-24 16:49 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Benoît Canet
[-- Attachment #1: Type: text/plain, Size: 868 bytes --]
On 02/13/2015 09:06 AM, Alberto Garcia wrote:
> From: Benoît Canet <benoit.canet@nodalink.com>
>
> Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> include/qemu/throttle.h | 4 ++--
> tests/test-throttle.c | 4 ++--
> util/throttle.c | 4 ++--
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> index f846e5a..6174332 100644
> --- a/include/qemu/throttle.h
> +++ b/include/qemu/throttle.h
> @@ -1,10 +1,10 @@
> /*
> * QEMU throttling infrastructure
> *
> - * Copyright (C) Nodalink, SARL. 2013
> + * Copyright (C) Nodalink, EURL. 2013-2014
Don't you also want to claim 2015 now?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo
2015-02-13 16:06 ` [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo Alberto Garcia
@ 2015-02-24 16:54 ` Eric Blake
2015-02-25 10:56 ` Alberto Garcia
0 siblings, 1 reply; 34+ messages in thread
From: Eric Blake @ 2015-02-24 16:54 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 2030 bytes --]
On 02/13/2015 09:06 AM, Alberto Garcia wrote:
> Replace also throttle_group_compare() with throttle_group_get_name()
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block.c | 2 +-
> block/qapi.c | 5 +++++
> block/throttle-groups.c | 14 ++++----------
> hmp.c | 6 ++++--
> include/block/throttle-groups.h | 2 +-
> qapi/block-core.json | 4 +++-
> 6 files changed, 18 insertions(+), 15 deletions(-)
>
> +++ b/qapi/block-core.json
> @@ -255,6 +255,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)
Ugh - now I'm getting confused by context. Looks like 7/9 touched
block_set_io_throttle, and 9/9 touches BlockDeviceInfo.
> #
> # @write_threshold: configured write threshold for the device.
> @@ -274,7 +276,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' } }
>
Questions - with this series in place, is it ever possible to have
throttling parameters without a throttle group name? Do we
auto-generate a group name (perhaps based on the node name) for any
throttling parameters set without reference to a group name? When using
block_set_io_throttle, is it legal to pass parameters (like bps_max) and
a group name at the same time, and if so, what happens if there is
already a throttle group by that name?
Is there a command that can return the list of all throttle group names?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 8/9] throttle: Update throttle infrastructure copyright
2015-02-24 16:49 ` Eric Blake
@ 2015-02-24 20:21 ` Benoît Canet
0 siblings, 0 replies; 34+ messages in thread
From: Benoît Canet @ 2015-02-24 20:21 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, Stefan Hajnoczi, Alberto Garcia, qemu-devel,
Benoît Canet
On Tue, Feb 24, 2015 at 09:49:26AM -0700, Eric Blake wrote:
> On 02/13/2015 09:06 AM, Alberto Garcia wrote:
> > From: Benoît Canet <benoit.canet@nodalink.com>
> >
> > Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > ---
> > include/qemu/throttle.h | 4 ++--
> > tests/test-throttle.c | 4 ++--
> > util/throttle.c | 4 ++--
> > 3 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> > index f846e5a..6174332 100644
> > --- a/include/qemu/throttle.h
> > +++ b/include/qemu/throttle.h
> > @@ -1,10 +1,10 @@
> > /*
> > * QEMU throttling infrastructure
> > *
> > - * Copyright (C) Nodalink, SARL. 2013
> > + * Copyright (C) Nodalink, EURL. 2013-2014
>
> Don't you also want to claim 2015 now?
I didn't worked on it on 2015.
Maybe Alberto will want to claim his work on it.
Best regards
Benoît
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo
2015-02-24 16:54 ` Eric Blake
@ 2015-02-25 10:56 ` Alberto Garcia
2015-02-25 15:23 ` Eric Blake
0 siblings, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-02-25 10:56 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Tue, Feb 24, 2015 at 09:54:13AM -0700, Eric Blake wrote:
> Questions - with this series in place, is it ever possible to have
> throttling parameters without a throttle group name?
Yes, and it will work the same as before. If the throttling parameters
are set but no group name is specified then we auto-generate one.
> When using block_set_io_throttle, is it legal to pass parameters
> (like bps_max) and a group name at the same time
Yes, you're actually supposed to do it like that.
If the group does not exist yet, it's created on the fly. If the group
name is not set, then we auto-generate one.
> and if so, what happens if there is already a throttle group by that
> name?
All members of the same group share the same ThrottleState
configuration (it's stored in the group), so when you set the
throttling parameters you set them for the whole group.
For the same reason, removing a member from the group doesn't change
the throttling parameters. But once the group is empty, it is
destroyed.
> Is there a command that can return the list of all throttle group
> names?
Not currently, but I think I can add one easily. Any suggestion for
the name of the command, and for the data that you would like it to
return?
Berto
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo
2015-02-25 10:56 ` Alberto Garcia
@ 2015-02-25 15:23 ` Eric Blake
2015-02-25 15:37 ` Alberto Garcia
2015-02-26 13:56 ` Alberto Garcia
0 siblings, 2 replies; 34+ messages in thread
From: Eric Blake @ 2015-02-25 15:23 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 867 bytes --]
On 02/25/2015 03:56 AM, Alberto Garcia wrote:
>> Is there a command that can return the list of all throttle group
>> names?
>
> Not currently, but I think I can add one easily. Any suggestion for
> the name of the command, and for the data that you would like it to
> return?
How about query-block-throttle, returning an array of dicts. Ideas for
what it could contain would be the name of the block group, its current
settings, and the node names associated with the group. Maybe something
like:
=> { "execute":"query-block-throttle" }
<= { "return": [
{ "name": "throttle1", "bps_max": 100000,
"nodes": [ "block0", "block1" ] },
{ "name": "throttle2", "iops_max": 10000,
"nodes": [ "block2" ] }
] }
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo
2015-02-25 15:23 ` Eric Blake
@ 2015-02-25 15:37 ` Alberto Garcia
2015-02-26 13:56 ` Alberto Garcia
1 sibling, 0 replies; 34+ messages in thread
From: Alberto Garcia @ 2015-02-25 15:37 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Wed, Feb 25, 2015 at 08:23:10AM -0700, Eric Blake wrote:
> >> Is there a command that can return the list of all throttle group
> >> names?
> >
> > Not currently, but I think I can add one easily. Any suggestion
> > for the name of the command, and for the data that you would like
> > it to return?
>
> How about query-block-throttle, returning an array of dicts. Ideas
> for what it could contain would be the name of the block group, its
> current settings, and the node names associated with the group.
> Maybe something like:
>
> => { "execute":"query-block-throttle" }
> <= { "return": [
> { "name": "throttle1", "bps_max": 100000,
> "nodes": [ "block0", "block1" ] },
> { "name": "throttle2", "iops_max": 10000,
> "nodes": [ "block2" ] }
> ] }
Sounds reasonable, I think it should be easily doable, I can give it a
try.
One thing to note, not that it's directly related to group throttling,
but as far as I'm aware the current throttling code cannot be used in
arbitrary nodes, only in the root (block_set_io_throttle receives a
device name).
Berto
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo
2015-02-25 15:23 ` Eric Blake
2015-02-25 15:37 ` Alberto Garcia
@ 2015-02-26 13:56 ` Alberto Garcia
2015-03-03 17:53 ` Eric Blake
1 sibling, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-02-26 13:56 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Wed, Feb 25, 2015 at 08:23:10AM -0700, Eric Blake wrote:
> How about query-block-throttle, returning an array of dicts.
>
> => { "execute":"query-block-throttle" }
> <= { "return": [
> { "name": "throttle1", "bps_max": 100000,
> "nodes": [ "block0", "block1" ] },
> { "name": "throttle2", "iops_max": 10000,
> "nodes": [ "block2" ] }
> ] }
Ok I wrote it, however I have some doubts about what to put exactly in
the 'nodes' array.
We can just put node names as you suggest, however at the moment not
all nodes have a name so we could end up with a list of empty strings.
I think the easiest solution in terms of lines of code and simplicity
of the return type is this one:
{ 'type': 'ThrottleGroupInfo',
'data': { 'name': 'str', 'nodes': ['BlockDeviceInfo'] } }
{ 'command': 'query-block-throttle',
'returns': ['ThrottleGroupInfo'] }
All the information is there, the code to fill the BlockDeviceInfo
structure is already written so I can just make use of it.
The throttling settings themselves are not present in
ThrottleGroupInfo, but since all nodes from a group have the same
settings, you can get them from any of them. It also keeps the
ThrottleGroupInfo structure simpler.
What do you think? If you're ok with this solution I can submit the
patch series again.
Berto
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 1/9] throttle: Extract timers from ThrottleState into a separate ThrottleTimers structure
2015-02-13 16:06 ` [Qemu-devel] [PATCH 1/9] throttle: Extract timers from ThrottleState into a separate ThrottleTimers structure Alberto Garcia
@ 2015-03-03 16:03 ` Stefan Hajnoczi
0 siblings, 0 replies; 34+ messages in thread
From: Stefan Hajnoczi @ 2015-03-03 16:03 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Benoît Canet
[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]
On Fri, Feb 13, 2015 at 06:06:09PM +0200, Alberto Garcia wrote:
> 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>
> ---
> 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(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 2/9] throttle: Add throttle group infrastructure
2015-02-13 16:06 ` [Qemu-devel] [PATCH 2/9] throttle: Add throttle group infrastructure Alberto Garcia
@ 2015-03-03 16:38 ` Stefan Hajnoczi
2015-03-04 10:18 ` Alberto Garcia
0 siblings, 1 reply; 34+ messages in thread
From: Stefan Hajnoczi @ 2015-03-03 16:38 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Benoît Canet
[-- Attachment #1: Type: text/plain, Size: 4943 bytes --]
On Fri, Feb 13, 2015 at 06:06:10PM +0200, Alberto Garcia wrote:
> From: Benoît Canet <benoit.canet@nodalink.com>
>
> The throttle_group_incref increment the refcount of a throttle group given it's
s/it's/its/
http://www.its-not-its.info/
> +typedef struct ThrottleGroup {
> + char name[32];
> + ThrottleState ts;
> + uint64_t refcount;
> + QTAILQ_ENTRY(ThrottleGroup) list;
> + QLIST_HEAD(, BlockDriverState) head;
> + BlockDriverState *tokens[2]; /* current round-robin tokens */
> + QemuMutex lock; /* Used to synchronize all elements belonging to a group */
Not sure what this comment means. Which fields are protected by lock?
I think refcount is not protected by lock, since it is incremented in
throttle_group_incref() without holding the lock.
> +} ThrottleGroup;
> +
> +static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
> + QTAILQ_HEAD_INITIALIZER(throttle_groups);
Is throttle_groups protected by the QEMU global mutex? It would be
helpful to add a comment.
> +
> +/* increments a ThrottleGroup reference count given it's name
s/it's/its/
> + *
> + * If no ThrottleGroup is found with the given name a new one is created.
> + *
> + * @name: the name of the ThrottleGroup
> + * @ret: the ThrottleGroup's ThrottleState address
> + */
> +ThrottleState *throttle_group_incref(const char *name)
> +{
> + ThrottleGroup *tg;
> +
> + /* return the correct ThrottleState if a group with this name exists */
> + QTAILQ_FOREACH(tg, &throttle_groups, list) {
> + /* group not found -> continue */
> + if (strcmp(name, tg->name)) {
> + continue;
> + }
> + /* group found -> increment it's refcount and return ThrottleState */
> + tg->refcount++;
> + return &tg->ts;
> + }
> +
> + /* throttle group not found -> prepare new entry */
> + tg = g_new0(ThrottleGroup, 1);
> + pstrcpy(tg->name, sizeof(tg->name), name);
Silently truncating to 32 chars is confusing. Please use g_strdup() and
g_free() the string when refcount reaches 0.
> + qemu_mutex_init(&tg->lock);
> + throttle_init(&tg->ts);
> + QLIST_INIT(&tg->head);
> + tg->refcount = 1;
> +
> + /* insert new entry in the list */
> + QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
It is safest to hold tg->lock before adding the group to the list. This
way there is a memory barrier and other threads will not access the
group until we've finished adding it to the list.
The memory barrier is important so other threads don't see old memory
values for the group's fields.
> +
> + /* return newly allocated ThrottleState */
> + return &tg->ts;
> +}
> +
> +/* decrement a ThrottleGroup given it's ThrottleState address
s/it's/its/
> + *
> + * When the refcount reach zero the ThrottleGroup is destroyed
> + *
> + * @ts: The ThrottleState address belonging to the ThrottleGroup to unref
> + * @ret: true on success else false
> + */
> +bool throttle_group_unref(ThrottleState *ts)
> +{
> + ThrottleGroup *tg;
> + bool found = false;
> +
> + /* Find the ThrottleGroup of the given ThrottleState */
> + QTAILQ_FOREACH(tg, &throttle_groups, list) {
> + /* correct group found stop iterating */
> + if (&tg->ts == ts) {
> + qemu_mutex_lock(&tg->lock);
> + found = true;
> + break;
> + }
> + }
> +
> + /* If the ThrottleState was not found something is seriously broken */
> + if (!found) {
> + return false;
> + }
Please correct me if I'm wrong but I suggest:
Make this function void and replace this statement with assert(found).
This case should never happen and I doubt callers will be able to handle
the error case.
> +/* Compare a name with a given ThrottleState group name
> + *
> + * @ts: the throttle state whose group we are inspecting
> + * @name: the name to compare
> + * @ret: true if names are equal else false
> + */
> +bool throttle_group_compare(ThrottleState *ts, const char *name)
Normally a plain "compare" function checks if two values of the same
type are equal.
The name throttle_group_compare_name() would be clearer.
> +{
> + ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
Why can this function use container_of() while throttle_group_unref()
has to loop over all ThrottleGroups to find ts?
> +/* Used to lock a ThrottleState's ThrottleGroup
> + *
> + * @ts: the ThrottleState the code is working on
> + */
> +void throttle_group_lock(ThrottleState *ts)
> +{
> + ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
> + qemu_mutex_lock(&tg->lock);
> +}
lock/unlock functions are sometimes an indication that the calling code
should really be moved into this file. I'm not sure yet since I haven't
read all the patches, but it is a little suspicious.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 4/9] throttle: Prepare to have multiple timers for one ThrottleState
2015-02-13 16:06 ` [Qemu-devel] [PATCH 4/9] throttle: Prepare to have multiple timers for one ThrottleState Alberto Garcia
@ 2015-03-03 16:50 ` Stefan Hajnoczi
0 siblings, 0 replies; 34+ messages in thread
From: Stefan Hajnoczi @ 2015-03-03 16:50 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Benoît Canet
[-- Attachment #1: Type: text/plain, Size: 681 bytes --]
On Fri, Feb 13, 2015 at 06:06:12PM +0200, Alberto Garcia wrote:
> From: Benoît Canet <benoit.canet@nodalink.com>
>
> This patch transform the timer_pending call into two boolean values in the
> ThrottleState structure.
>
> This way we are sure that when multiple timers will be used only
> one can be armed at a time.
>
> Signed-off-by: Benoit Canet <benoit.canet@nodalink.com>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block.c | 2 ++
> include/qemu/throttle.h | 3 +++
> util/throttle.c | 16 +++++++++++++---
> 3 files changed, 18 insertions(+), 3 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] throttle: Add a way to fire one of the timers asap like a bottom half
2015-02-13 16:06 ` [Qemu-devel] [PATCH 6/9] throttle: Add a way to fire one of the timers asap like a bottom half Alberto Garcia
@ 2015-03-03 17:08 ` Stefan Hajnoczi
0 siblings, 0 replies; 34+ messages in thread
From: Stefan Hajnoczi @ 2015-03-03 17:08 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Benoît Canet
[-- Attachment #1: Type: text/plain, Size: 866 bytes --]
On Fri, Feb 13, 2015 at 06:06:14PM +0200, Alberto Garcia wrote:
> diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
> index 69aa7c2..f846e5a 100644
> --- a/include/qemu/throttle.h
> +++ b/include/qemu/throttle.h
> @@ -127,6 +127,8 @@ bool throttle_schedule_timer(ThrottleState *ts,
> bool is_write,
> bool *armed);
>
> +void throttle_fire_timer(ThrottleTimers *tt, bool is_write);
> +
> void throttle_timer_fired(ThrottleState *ts, bool is_write);
>
> void throttle_account(ThrottleState *ts, bool is_write, uint64_t size);
General feeling when seeing this patch:
This is a little weird because suddenly we're dealing directly with
ThrottleTimers instead of ThrottleState or ThrottleGroup. Also, it
doesn't set any_timers_pending[], maybe that could be a problem.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo
2015-02-26 13:56 ` Alberto Garcia
@ 2015-03-03 17:53 ` Eric Blake
2015-03-04 7:09 ` Markus Armbruster
0 siblings, 1 reply; 34+ messages in thread
From: Eric Blake @ 2015-03-03 17:53 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]
On 02/26/2015 06:56 AM, Alberto Garcia wrote:
> On Wed, Feb 25, 2015 at 08:23:10AM -0700, Eric Blake wrote:
>
>> How about query-block-throttle, returning an array of dicts.
>>
>> => { "execute":"query-block-throttle" }
>> <= { "return": [
>> { "name": "throttle1", "bps_max": 100000,
>> "nodes": [ "block0", "block1" ] },
>> { "name": "throttle2", "iops_max": 10000,
>> "nodes": [ "block2" ] }
>> ] }
>
> Ok I wrote it, however I have some doubts about what to put exactly in
> the 'nodes' array.
>
> We can just put node names as you suggest, however at the moment not
> all nodes have a name so we could end up with a list of empty strings.
At one point, we were considering a patch from Jeff Cody that guarantees
ALL nodes have a name. Maybe that's still worthwhile.
>
> I think the easiest solution in terms of lines of code and simplicity
> of the return type is this one:
>
> { 'type': 'ThrottleGroupInfo',
> 'data': { 'name': 'str', 'nodes': ['BlockDeviceInfo'] } }
That's a bit more verbose, but also a bit more usable (all the block
data is available, rather than just a name that has to be looked up via
another command), so that could work.
>
> { 'command': 'query-block-throttle',
> 'returns': ['ThrottleGroupInfo'] }
>
> All the information is there, the code to fill the BlockDeviceInfo
> structure is already written so I can just make use of it.
>
> The throttling settings themselves are not present in
> ThrottleGroupInfo, but since all nodes from a group have the same
> settings, you can get them from any of them. It also keeps the
> ThrottleGroupInfo structure simpler.
>
> What do you think? If you're ok with this solution I can submit the
> patch series again.
Sure, sounds like it's worth that solution, since it reused code and
made for less work on your part.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support
2015-02-13 16:06 ` [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support Alberto Garcia
2015-02-24 16:45 ` Eric Blake
@ 2015-03-03 21:00 ` Stefan Hajnoczi
2015-03-04 13:53 ` Alberto Garcia
1 sibling, 1 reply; 34+ messages in thread
From: Stefan Hajnoczi @ 2015-03-03 21:00 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 4689 bytes --]
On Fri, Feb 13, 2015 at 06:06:15PM +0200, Alberto Garcia wrote:
> 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 compute if a wait must be done and arm 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 | 212 +++++++++++++++++++++++++++++++++++++++-------
> block/qapi.c | 7 +-
> block/throttle-groups.c | 2 +-
> blockdev.c | 19 ++++-
> hmp.c | 4 +-
> include/block/block.h | 3 +-
> include/block/block_int.h | 9 +-
> qapi/block-core.json | 5 +-
> qemu-options.hx | 1 +
> qmp-commands.hx | 3 +-
> 10 files changed, 220 insertions(+), 45 deletions(-)
>
> diff --git a/block.c b/block.c
> index 642ef04..625f1c8 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,9 @@ void bdrv_set_io_limits(BlockDriverState *bs,
> {
> int i;
>
> - throttle_config(&bs->throttle_state, &bs->throttle_timers, cfg);
> + throttle_group_lock(bs->throttle_state);
> + throttle_config(bs->throttle_state, &bs->throttle_timers, cfg);
> + throttle_group_unlock(bs->throttle_state);
>
> for (i = 0; i < 2; i++) {
> qemu_co_enter_next(&bs->throttled_reqs[i]);
> @@ -157,34 +160,99 @@ static bool bdrv_start_throttled_reqs(BlockDriverState *bs)
> return drained;
> }
>
> +static void bdrv_throttle_group_add(BlockDriverState *bs)
> +{
> + int i;
> + BlockDriverState *token;
> +
> + for (i = 0; i < 2; i++) {
> + /* Get the BlockDriverState having the round robin token */
> + token = throttle_group_token(bs->throttle_state, i);
> +
> + /* If the ThrottleGroup is new set the current BlockDriverState as
> + * token
> + */
> + if (!token) {
> + throttle_group_set_token(bs->throttle_state, bs, i);
> + }
> +
> + }
Does it make sense to move this inside throttle_group_register_bs()?
Then bdrv_throttle_group_add() could be dropped and
throttle_group_register_bs() is called directly.
> +
> + throttle_group_register_bs(bs->throttle_state, bs);
> +}
> +
> +static void bdrv_throttle_group_remove(BlockDriverState *bs)
The name is a hint that this doesn't belong in block.c. This function
should be throttle_group_unregister_bs().
> +{
> + BlockDriverState *token;
> + int i;
> +
> + for (i = 0; i < 2; i++) {
> + /* Get the BlockDriverState having the round robin token */
> + token = throttle_group_token(bs->throttle_state, i);
> + /* if this bs is the current token set the next bs as token */
> + if (token == bs) {
> + token = throttle_group_next_bs(token);
> + /* take care of the case where bs is the only bs of the group */
> + if (token == bs) {
> + token = NULL;
> + }
> + throttle_group_set_token(bs->throttle_state, token, i);
> + }
> + }
> +
> + /* remove the current bs from the list */
> + QLIST_REMOVE(bs, round_robin);
> +}
> +
> void bdrv_io_limits_disable(BlockDriverState *bs)
> {
> +
> + throttle_group_lock(bs->throttle_state);
> bs->io_limits_enabled = false;
> + throttle_group_unlock(bs->throttle_state);
What are the locking rules? I don't understand why throttle_state must
be locked to modify bs->io_limits_enabled.
>
> bdrv_start_throttled_reqs(bs);
>
> + throttle_group_lock(bs->throttle_state);
> + bdrv_throttle_group_remove(bs);
> + throttle_group_unlock(bs->throttle_state);
> +
> + throttle_group_unref(bs->throttle_state);
> + bs->throttle_state = NULL;
> +
> throttle_timers_destroy(&bs->throttle_timers);
> }
>
> static void bdrv_throttle_read_timer_cb(void *opaque)
> {
> BlockDriverState *bs = opaque;
> - throttle_timer_fired(&bs->throttle_state, false);
> +
> + throttle_group_lock(bs->throttle_state);
> + throttle_timer_fired(bs->throttle_state, false);
> + throttle_group_unlock(bs->throttle_state);
This pattern suggests throttle_timer_fired() should acquire the lock
internally instead.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
` (8 preceding siblings ...)
2015-02-13 16:06 ` [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo Alberto Garcia
@ 2015-03-03 21:07 ` Stefan Hajnoczi
9 siblings, 0 replies; 34+ messages in thread
From: Stefan Hajnoczi @ 2015-03-03 21:07 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 767 bytes --]
On Fri, Feb 13, 2015 at 06:06:08PM +0200, Alberto Garcia wrote:
> after having agreed it with him, I'm taking over Benoît Canet's work
> to add support for throttle groups for block devices.
The locking rules are undocumented so it's hard to review the patches.
I think the intent is for throttling groups to be independent of
AioContext, so I/O can be processed in multiple threads at the same
time. This requires synchronizing shared throttling state.
But the details of how the global throttle groups list is protected and
how individual ThrottleGroup and ThrottleState structs work is unclear.
Please resolve comments I've posted on individual patches and then
summarize the locking rules in a comment in throttle-groups.c.
Thanks,
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo
2015-03-03 17:53 ` Eric Blake
@ 2015-03-04 7:09 ` Markus Armbruster
2015-03-04 7:20 ` Alberto Garcia
0 siblings, 1 reply; 34+ messages in thread
From: Markus Armbruster @ 2015-03-04 7:09 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, Alberto Garcia, qemu-devel, Stefan Hajnoczi
Eric Blake <eblake@redhat.com> writes:
> On 02/26/2015 06:56 AM, Alberto Garcia wrote:
>> On Wed, Feb 25, 2015 at 08:23:10AM -0700, Eric Blake wrote:
>>
>>> How about query-block-throttle, returning an array of dicts.
>>>
>>> => { "execute":"query-block-throttle" }
>>> <= { "return": [
>>> { "name": "throttle1", "bps_max": 100000,
>>> "nodes": [ "block0", "block1" ] },
>>> { "name": "throttle2", "iops_max": 10000,
>>> "nodes": [ "block2" ] }
>>> ] }
>>
>> Ok I wrote it, however I have some doubts about what to put exactly in
>> the 'nodes' array.
>>
>> We can just put node names as you suggest, however at the moment not
>> all nodes have a name so we could end up with a list of empty strings.
>
> At one point, we were considering a patch from Jeff Cody that guarantees
> ALL nodes have a name. Maybe that's still worthwhile.
>
>>
>> I think the easiest solution in terms of lines of code and simplicity
>> of the return type is this one:
>>
>> { 'type': 'ThrottleGroupInfo',
>> 'data': { 'name': 'str', 'nodes': ['BlockDeviceInfo'] } }
>
> That's a bit more verbose, but also a bit more usable (all the block
> data is available, rather than just a name that has to be looked up via
> another command), so that could work.
>
>>
>> { 'command': 'query-block-throttle',
>> 'returns': ['ThrottleGroupInfo'] }
>>
>> All the information is there, the code to fill the BlockDeviceInfo
>> structure is already written so I can just make use of it.
>>
>> The throttling settings themselves are not present in
>> ThrottleGroupInfo, but since all nodes from a group have the same
>> settings, you can get them from any of them. It also keeps the
>> ThrottleGroupInfo structure simpler.
>>
>> What do you think? If you're ok with this solution I can submit the
>> patch series again.
>
> Sure, sounds like it's worth that solution, since it reused code and
> made for less work on your part.
We need a coherent set of block queries. What we have is an ad hoc
mess. Alberto proposes to add to the mess. Digs us deeper into the
hole. But asking him to solve the complete block query problem so he
can have his little feature is hardly fair.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo
2015-03-04 7:09 ` Markus Armbruster
@ 2015-03-04 7:20 ` Alberto Garcia
0 siblings, 0 replies; 34+ messages in thread
From: Alberto Garcia @ 2015-03-04 7:20 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Wed, Mar 04, 2015 at 08:09:22AM +0100, Markus Armbruster wrote:
> >> { 'type': 'ThrottleGroupInfo',
> >> 'data': { 'name': 'str', 'nodes': ['BlockDeviceInfo'] } }
> >>
> >> { 'command': 'query-block-throttle',
> >> 'returns': ['ThrottleGroupInfo'] }
> We need a coherent set of block queries. What we have is an ad hoc
> mess. Alberto proposes to add to the mess. Digs us deeper into the
> hole.
If you can describe what you would like to have then I can try to
help.
Berto
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 2/9] throttle: Add throttle group infrastructure
2015-03-03 16:38 ` Stefan Hajnoczi
@ 2015-03-04 10:18 ` Alberto Garcia
2015-03-04 16:02 ` Stefan Hajnoczi
0 siblings, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-03-04 10:18 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Benoît Canet
On Tue, Mar 03, 2015 at 10:38:45AM -0600, Stefan Hajnoczi wrote:
> > + qemu_mutex_init(&tg->lock);
> > + throttle_init(&tg->ts);
> > + QLIST_INIT(&tg->head);
> > + tg->refcount = 1;
> > +
> > + /* insert new entry in the list */
> > + QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
>
> It is safest to hold tg->lock before adding the group to the
> list. This way there is a memory barrier and other threads will not
> access the group until we've finished adding it to the list.
I think that the throttle_group_incref/unref calls are only made from
the QEMU main loop, and that's the only code that deals with the
throttle_groups list, so I don't see any chance for a race condition
there.
Also, there's no way a different thread can have access to a group
before it's in the list, because the only way to get a group is to
retrieve it from the list.
If it was possible for two threads to try to incref() the same group
we would need to make the whole function thread-safe, otherwise we
would have a situation where two threads can create two groups with
the same name because both think that it doesn't exist yet.
I will anyway double-check if that's the case. Maybe it's a good idea
to protect both calls with a mutex anyway so we don't have to rely on
any assumptions?
> > + /* If the ThrottleState was not found something is seriously broken */
> > + if (!found) {
> > + return false;
> > + }
>
> Please correct me if I'm wrong but I suggest:
>
> Make this function void and replace this statement with
> assert(found). This case should never happen and I doubt callers
> will be able to handle the error case.
I think you're right, it seems that the only use of that is to check
its return value from the tests, but I don't see any other use case
for an unref() function returning a bool, so I'll make it void.
> > + ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
>
> Why can this function use container_of() while
> throttle_group_unref() has to loop over all ThrottleGroups to find
> ts?
I hadn't actually noticed this, thanks for pointing it out. I don't
think there's any need to loop over the groups, so I'll just use
container_of in both cases.
Thanks also for the rest of the suggestions, I'll include them the
next time I submit the patches.
Berto
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support
2015-03-03 21:00 ` Stefan Hajnoczi
@ 2015-03-04 13:53 ` Alberto Garcia
2015-03-04 16:04 ` Stefan Hajnoczi
0 siblings, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-03-04 13:53 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On Tue, Mar 03, 2015 at 03:00:06PM -0600, Stefan Hajnoczi wrote:
> > + throttle_group_lock(bs->throttle_state);
> > + bdrv_throttle_group_remove(bs);
> > + throttle_group_unlock(bs->throttle_state);
> > +
> > + throttle_group_unref(bs->throttle_state);
> > + bs->throttle_state = NULL;
> > +
> > throttle_timers_destroy(&bs->throttle_timers);
> > }
> >
> > static void bdrv_throttle_read_timer_cb(void *opaque)
> > {
> > BlockDriverState *bs = opaque;
> > - throttle_timer_fired(&bs->throttle_state, false);
> > +
> > + throttle_group_lock(bs->throttle_state);
> > + throttle_timer_fired(bs->throttle_state, false);
> > + throttle_group_unlock(bs->throttle_state);
>
> This pattern suggests throttle_timer_fired() should acquire the lock
> internally instead.
The idea is that the ThrottleState code itself doesn't know anything
about locks or groups. As I understood it Benoît designed the
ThrottleState code to be independent from the block layer and reusable
for other things (that's why it's in util/).
Berto
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 2/9] throttle: Add throttle group infrastructure
2015-03-04 10:18 ` Alberto Garcia
@ 2015-03-04 16:02 ` Stefan Hajnoczi
0 siblings, 0 replies; 34+ messages in thread
From: Stefan Hajnoczi @ 2015-03-04 16:02 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel, Benoît Canet
[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]
On Wed, Mar 04, 2015 at 11:18:21AM +0100, Alberto Garcia wrote:
> On Tue, Mar 03, 2015 at 10:38:45AM -0600, Stefan Hajnoczi wrote:
>
> > > + qemu_mutex_init(&tg->lock);
> > > + throttle_init(&tg->ts);
> > > + QLIST_INIT(&tg->head);
> > > + tg->refcount = 1;
> > > +
> > > + /* insert new entry in the list */
> > > + QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
> >
> > It is safest to hold tg->lock before adding the group to the
> > list. This way there is a memory barrier and other threads will not
> > access the group until we've finished adding it to the list.
>
> I think that the throttle_group_incref/unref calls are only made from
> the QEMU main loop, and that's the only code that deals with the
> throttle_groups list, so I don't see any chance for a race condition
> there.
>
> Also, there's no way a different thread can have access to a group
> before it's in the list, because the only way to get a group is to
> retrieve it from the list.
>
> If it was possible for two threads to try to incref() the same group
> we would need to make the whole function thread-safe, otherwise we
> would have a situation where two threads can create two groups with
> the same name because both think that it doesn't exist yet.
>
> I will anyway double-check if that's the case. Maybe it's a good idea
> to protect both calls with a mutex anyway so we don't have to rely on
> any assumptions?
The assumption is fine if it's documented.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support
2015-03-04 13:53 ` Alberto Garcia
@ 2015-03-04 16:04 ` Stefan Hajnoczi
2015-03-04 16:16 ` Alberto Garcia
0 siblings, 1 reply; 34+ messages in thread
From: Stefan Hajnoczi @ 2015-03-04 16:04 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]
On Wed, Mar 04, 2015 at 02:53:42PM +0100, Alberto Garcia wrote:
> On Tue, Mar 03, 2015 at 03:00:06PM -0600, Stefan Hajnoczi wrote:
>
> > > + throttle_group_lock(bs->throttle_state);
> > > + bdrv_throttle_group_remove(bs);
> > > + throttle_group_unlock(bs->throttle_state);
> > > +
> > > + throttle_group_unref(bs->throttle_state);
> > > + bs->throttle_state = NULL;
> > > +
> > > throttle_timers_destroy(&bs->throttle_timers);
> > > }
> > >
> > > static void bdrv_throttle_read_timer_cb(void *opaque)
> > > {
> > > BlockDriverState *bs = opaque;
> > > - throttle_timer_fired(&bs->throttle_state, false);
> > > +
> > > + throttle_group_lock(bs->throttle_state);
> > > + throttle_timer_fired(bs->throttle_state, false);
> > > + throttle_group_unlock(bs->throttle_state);
> >
> > This pattern suggests throttle_timer_fired() should acquire the lock
> > internally instead.
>
> The idea is that the ThrottleState code itself doesn't know anything
> about locks or groups. As I understood it Benoît designed the
> ThrottleState code to be independent from the block layer and reusable
> for other things (that's why it's in util/).
Then ThrottleGroup could offer an API throttle_group_timer_fired() that
does the locking.
The advantage of encapsulating locking in ThrottleGroup is that callers
don't have to remember the take the lock. (But they must still be
careful about sequences of calls which will not be atomic.)
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support
2015-03-04 16:04 ` Stefan Hajnoczi
@ 2015-03-04 16:16 ` Alberto Garcia
2015-03-05 17:41 ` Stefan Hajnoczi
0 siblings, 1 reply; 34+ messages in thread
From: Alberto Garcia @ 2015-03-04 16:16 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel
On Wed, Mar 04, 2015 at 10:04:27AM -0600, Stefan Hajnoczi wrote:
> > > This pattern suggests throttle_timer_fired() should acquire the
> > > lock internally instead.
> >
> > The idea is that the ThrottleState code itself doesn't know
> > anything about locks or groups. As I understood it Benoît
> > designed the ThrottleState code to be independent from the block
> > layer and reusable for other things (that's why it's in util/).
>
> Then ThrottleGroup could offer an API throttle_group_timer_fired()
> that does the locking.
>
> The advantage of encapsulating locking in ThrottleGroup is that
> callers don't have to remember the take the lock. (But they must
> still be careful about sequences of calls which will not be atomic.)
No other code in ThrottleGroup takes the lock directly, so making this
an exception could be confusing.
Truth to be told I'm not a very big fan of the timer callback having
to deal with that in any case. The any_timer_armed flag should be
something internal to the throttling code. I'll try to figure out a
more elegant way to solve this.
Berto
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support
2015-03-04 16:16 ` Alberto Garcia
@ 2015-03-05 17:41 ` Stefan Hajnoczi
0 siblings, 0 replies; 34+ messages in thread
From: Stefan Hajnoczi @ 2015-03-05 17:41 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Kevin Wolf, Stefan Hajnoczi, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]
On Wed, Mar 04, 2015 at 05:16:51PM +0100, Alberto Garcia wrote:
> On Wed, Mar 04, 2015 at 10:04:27AM -0600, Stefan Hajnoczi wrote:
>
> > > > This pattern suggests throttle_timer_fired() should acquire the
> > > > lock internally instead.
> > >
> > > The idea is that the ThrottleState code itself doesn't know
> > > anything about locks or groups. As I understood it Benoît
> > > designed the ThrottleState code to be independent from the block
> > > layer and reusable for other things (that's why it's in util/).
> >
> > Then ThrottleGroup could offer an API throttle_group_timer_fired()
> > that does the locking.
> >
> > The advantage of encapsulating locking in ThrottleGroup is that
> > callers don't have to remember the take the lock. (But they must
> > still be careful about sequences of calls which will not be atomic.)
>
> No other code in ThrottleGroup takes the lock directly, so making this
> an exception could be confusing.
It shouldn't be an exception. I'm proposing that the ThrottleGroup API
should handle locking internally instead of requiring callers to do it.
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2015-03-05 17:42 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-13 16:06 [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Alberto Garcia
2015-02-13 16:06 ` [Qemu-devel] [PATCH 1/9] throttle: Extract timers from ThrottleState into a separate ThrottleTimers structure Alberto Garcia
2015-03-03 16:03 ` Stefan Hajnoczi
2015-02-13 16:06 ` [Qemu-devel] [PATCH 2/9] throttle: Add throttle group infrastructure Alberto Garcia
2015-03-03 16:38 ` Stefan Hajnoczi
2015-03-04 10:18 ` Alberto Garcia
2015-03-04 16:02 ` Stefan Hajnoczi
2015-02-13 16:06 ` [Qemu-devel] [PATCH 3/9] throttle: Add throttle group infrastructure tests Alberto Garcia
2015-02-13 16:06 ` [Qemu-devel] [PATCH 4/9] throttle: Prepare to have multiple timers for one ThrottleState Alberto Garcia
2015-03-03 16:50 ` Stefan Hajnoczi
2015-02-13 16:06 ` [Qemu-devel] [PATCH 5/9] throttle: Add a way to know if throttle_schedule_timer had armed a timer Alberto Garcia
2015-02-13 16:06 ` [Qemu-devel] [PATCH 6/9] throttle: Add a way to fire one of the timers asap like a bottom half Alberto Garcia
2015-03-03 17:08 ` Stefan Hajnoczi
2015-02-13 16:06 ` [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support Alberto Garcia
2015-02-24 16:45 ` Eric Blake
2015-02-24 16:47 ` Eric Blake
2015-03-03 21:00 ` Stefan Hajnoczi
2015-03-04 13:53 ` Alberto Garcia
2015-03-04 16:04 ` Stefan Hajnoczi
2015-03-04 16:16 ` Alberto Garcia
2015-03-05 17:41 ` Stefan Hajnoczi
2015-02-13 16:06 ` [Qemu-devel] [PATCH 8/9] throttle: Update throttle infrastructure copyright Alberto Garcia
2015-02-24 16:49 ` Eric Blake
2015-02-24 20:21 ` Benoît Canet
2015-02-13 16:06 ` [Qemu-devel] [PATCH 9/9] throttle: add name of ThrottleGroup to BlockDeviceInfo Alberto Garcia
2015-02-24 16:54 ` Eric Blake
2015-02-25 10:56 ` Alberto Garcia
2015-02-25 15:23 ` Eric Blake
2015-02-25 15:37 ` Alberto Garcia
2015-02-26 13:56 ` Alberto Garcia
2015-03-03 17:53 ` Eric Blake
2015-03-04 7:09 ` Markus Armbruster
2015-03-04 7:20 ` Alberto Garcia
2015-03-03 21:07 ` [Qemu-devel] [PATCH v2 0/9] Block Throttle Group Support Stefan Hajnoczi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.