diff mbox

[2/6] throttle: Add throttle group infrastructure

Message ID 3c960428503cfd363a55f72678a2f6a710d94a3e.1426000801.git.berto@igalia.com
State New
Headers show

Commit Message

Alberto Garcia March 10, 2015, 3:30 p.m. UTC
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/Makefile.objs             |   1 +
 block/throttle-groups.c         | 244 ++++++++++++++++++++++++++++++++++++++++
 include/block/block_int.h       |   1 +
 include/block/throttle-groups.h |  41 +++++++
 4 files changed, 287 insertions(+)
 create mode 100644 block/throttle-groups.c
 create mode 100644 include/block/throttle-groups.h

Comments

Stefan Hajnoczi March 24, 2015, 3:03 p.m. UTC | #1
On Tue, Mar 10, 2015 at 05:30:46PM +0200, Alberto Garcia wrote:
> +typedef struct ThrottleGroup {
> +    char *name; /* This is constant during the lifetime of the group */

Is this also protected by throttle_groups_lock?

I guess throttle_groups_lock must be held in order to read this field -
otherwise there is a risk that the object is freed unless you have
already incremented the refcount.

> +
> +    QemuMutex lock;

What does this lock protect?  I guess 'ts', 'head', and 'tokens' fields.
Please document it.

> +    ThrottleState ts;
> +    QLIST_HEAD(, BlockDriverState) head;
> +    BlockDriverState *tokens[2];
Alberto Garcia March 24, 2015, 3:33 p.m. UTC | #2
On Tue, Mar 24, 2015 at 03:03:07PM +0000, Stefan Hajnoczi wrote:

> > +typedef struct ThrottleGroup {
> > +    char *name; /* This is constant during the lifetime of the group */
> 
> Is this also protected by throttle_groups_lock?
> 
> I guess throttle_groups_lock must be held in order to read this
> field - otherwise there is a risk that the object is freed unless
> you have already incremented the refcount.

The creation and destruction of ThrottleGroup objects are protected by
throttle_groups_lock. That includes handling the memory for that field
and its contents.

Once the ThrottleGroup is created the 'name' field doesn't need any
additional locking since it remains constant during the lifetime of
the group.

The only way to read it from the outside is throttle_group_get_name()
and that's safe (until you release the reference to the group, that
is).

> > +    QemuMutex lock;
> 
> What does this lock protect?  I guess 'ts', 'head', and 'tokens'
> fields.  Please document it.

I thought it was obvious in the code but since you ask I guess it's
not :) I can add an additional comment.

Berto
Stefan Hajnoczi March 25, 2015, 12:01 p.m. UTC | #3
On Tue, Mar 24, 2015 at 04:33:48PM +0100, Alberto Garcia wrote:
> On Tue, Mar 24, 2015 at 03:03:07PM +0000, Stefan Hajnoczi wrote:
> 
> > > +typedef struct ThrottleGroup {
> > > +    char *name; /* This is constant during the lifetime of the group */
> > 
> > Is this also protected by throttle_groups_lock?
> > 
> > I guess throttle_groups_lock must be held in order to read this
> > field - otherwise there is a risk that the object is freed unless
> > you have already incremented the refcount.
> 
> The creation and destruction of ThrottleGroup objects are protected by
> throttle_groups_lock. That includes handling the memory for that field
> and its contents.
> 
> Once the ThrottleGroup is created the 'name' field doesn't need any
> additional locking since it remains constant during the lifetime of
> the group.
> 
> The only way to read it from the outside is throttle_group_get_name()
> and that's safe (until you release the reference to the group, that
> is).

Right, the race condition is when the group is released.

Looking at this again, the assumption isn't that throttle_groups_lock is
held.  The AioContext lock is held by throttle_group_get_name() users
and that's why there is no race when releasing the reference.

If someone adds a throttle_group_get_name() caller in the future without
holding AioContext, then we'd be in trouble.  That is why documenting
the locking constraints is useful.

Stefan
Alberto Garcia March 25, 2015, 12:14 p.m. UTC | #4
> > > > +typedef struct ThrottleGroup {
> > > > +    char *name; /* This is constant during the lifetime of the group */
> > > 
> > > Is this also protected by throttle_groups_lock?
> > > 
> > > I guess throttle_groups_lock must be held in order to read this
> > > field - otherwise there is a risk that the object is freed
> > > unless you have already incremented the refcount.
> > 
> > The creation and destruction of ThrottleGroup objects are
> > protected by throttle_groups_lock. That includes handling the
> > memory for that field and its contents.
> > 
> > Once the ThrottleGroup is created the 'name' field doesn't need
> > any additional locking since it remains constant during the
> > lifetime of the group.
> > 
> > The only way to read it from the outside is
> > throttle_group_get_name() and that's safe (until you release the
> > reference to the group, that is).
> 
> Right, the race condition is when the group is released.
> 
> Looking at this again, the assumption isn't that
> throttle_groups_lock is held.  The AioContext lock is held by
> throttle_group_get_name() users and that's why there is no race when
> releasing the reference.
> 
> If someone adds a throttle_group_get_name() caller in the future
> without holding AioContext, then we'd be in trouble.  That is why
> documenting the locking constraints is useful.

But that would only happen if you access bs->throttle_state without
holding bs's AioContext. I understand that it's implicit that you
should hold the context there.

Maybe I can update the throttle_group_* API to use BlockDriverState
in all cases instead of ThrottleState, it's probably more clear like
that.

Berto
diff mbox

Patch

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..e355fed
--- /dev/null
+++ b/block/throttle-groups.c
@@ -0,0 +1,244 @@ 
+/*
+ * QEMU block throttling group infrastructure
+ *
+ * Copyright (C) Nodalink, EURL. 2014
+ * Copyright (C) Igalia, S.L. 2015
+ *
+ * Authors:
+ *   Benoît Canet <benoit.canet@nodalink.com>
+ *   Alberto Garcia <berto@igalia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "block/throttle-groups.h"
+
+/* The ThrottleGroup structure (with its ThrottleState) is shared
+ * among different BlockDriverState and it's independent from
+ * AioContext, so in order to use it from different threads it needs
+ * its own locking.
+ *
+ * This locking is however handled internally in this file, so it's
+ * transparent to outside users.
+ *
+ * The whole ThrottleGroup structure is private and invisible to
+ * outside users, that only use it through its ThrottleState.
+ */
+typedef struct ThrottleGroup {
+    char *name; /* This is constant during the lifetime of the group */
+
+    QemuMutex lock;
+    ThrottleState ts;
+    QLIST_HEAD(, BlockDriverState) head;
+    BlockDriverState *tokens[2];
+
+    /* These two are protected by throttle_groups_lock */
+    unsigned refcount;
+    QTAILQ_ENTRY(ThrottleGroup) list;
+} ThrottleGroup;
+
+static QemuMutex throttle_groups_lock;
+static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =
+    QTAILQ_HEAD_INITIALIZER(throttle_groups);
+
+/* Increments the reference count of a ThrottleGroup given its name.
+ *
+ * If no ThrottleGroup is found with the given name a new one is
+ * created.
+ *
+ * @name: the name of the ThrottleGroup
+ * @ret:  the ThrottleGroup
+ */
+static ThrottleGroup *throttle_group_incref(const char *name)
+{
+    ThrottleGroup *tg = NULL;
+    ThrottleGroup *iter;
+
+    qemu_mutex_lock(&throttle_groups_lock);
+
+    /* Look for an existing group with that name */
+    QTAILQ_FOREACH(iter, &throttle_groups, list) {
+        if (!strcmp(name, iter->name)) {
+            tg = iter;
+            break;
+        }
+    }
+
+    /* Create a new one if not found */
+    if (!tg) {
+        tg = g_new0(ThrottleGroup, 1);
+        tg->name = g_strdup(name);
+        qemu_mutex_init(&tg->lock);
+        throttle_init(&tg->ts);
+        QLIST_INIT(&tg->head);
+
+        QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
+    }
+
+    tg->refcount++;
+
+    qemu_mutex_unlock(&throttle_groups_lock);
+
+    return tg;
+}
+
+/* Decrease the reference count of a ThrottleGroup.
+ *
+ * When the reference count reaches zero the ThrottleGroup is
+ * destroyed.
+ *
+ * @tg:  The ThrottleGroup to unref
+ */
+static void throttle_group_unref(ThrottleGroup *tg)
+{
+    qemu_mutex_lock(&throttle_groups_lock);
+    if (--tg->refcount == 0) {
+        QTAILQ_REMOVE(&throttle_groups, tg, list);
+        qemu_mutex_destroy(&tg->lock);
+        g_free(tg->name);
+        g_free(tg);
+    }
+    qemu_mutex_unlock(&throttle_groups_lock);
+}
+
+/* Get the name from a ThrottleState's ThrottleGroup. The name (and
+ * the pointer) is guaranteed to remain constant during the lifetime
+ * of the group.
+ *
+ * @ts:   the throttle state whose group we are inspecting
+ * @ret:  the name of the group.
+ */
+const char *throttle_group_get_name(ThrottleState *ts)
+{
+    ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+    return tg->name;
+}
+
+/* Return the next BlockDriverState in the round-robin sequence
+ *
+ * This takes care of simulating a circular list
+ *
+ * @bs:  the current BlockDriverState
+ * @ret: the next BlockDriverState in the sequence
+ */
+static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs)
+{
+    ThrottleState *ts = bs->throttle_state;
+    ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+    BlockDriverState *next = QLIST_NEXT(bs, round_robin);
+
+    if (!next) {
+        return QLIST_FIRST(&tg->head);
+    }
+
+    return next;
+}
+
+/* Update the throttle configuration for a particular group. Similar
+ * to throttle_config(), but guarantees atomicity within the
+ * throttling group.
+ *
+ * @ts:  the ThrottleState of the group to update.
+ * @tt:  the throttle timers we use in this aio context
+ * @cfg: the configuration to set
+ */
+void throttle_group_config(ThrottleState *ts,
+                           ThrottleTimers *tt,
+                           ThrottleConfig *cfg)
+{
+    ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+    qemu_mutex_lock(&tg->lock);
+    throttle_config(ts, tt, cfg);
+    qemu_mutex_unlock(&tg->lock);
+}
+
+/* Get the throttle configuration from a particular group. Similar to
+ * throttle_get_config(), but guarantees atomicity within the
+ * throttling group.
+ *
+ * @ts:  the ThrottleState of the group.
+ * @cfg: the configuration will be written here
+ */
+void throttle_group_get_config(ThrottleState *ts, ThrottleConfig *cfg)
+{
+    ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
+    qemu_mutex_lock(&tg->lock);
+    throttle_get_config(ts, cfg);
+    qemu_mutex_unlock(&tg->lock);
+}
+
+/* Register a BlockDriverState in the throttling group, also updating
+ * its throttle_state pointer to point to it. If a throttling group
+ * with that name does not exist yet, it will be created.
+ *
+ * @bs:        the BlockDriverState to insert
+ * @groupname: the name of the group
+ */
+void throttle_group_register_bs(BlockDriverState *bs, const char *groupname)
+{
+    int i;
+    ThrottleGroup *tg = throttle_group_incref(groupname);
+
+    bs->throttle_state = &tg->ts;
+
+    qemu_mutex_lock(&tg->lock);
+    /* If the ThrottleGroup is new set this BlockDriverState as the token */
+    for (i = 0; i < 2; i++) {
+        if (!tg->tokens[i]) {
+            tg->tokens[i] = bs;
+        }
+    }
+
+    QLIST_INSERT_HEAD(&tg->head, bs, round_robin);
+    qemu_mutex_unlock(&tg->lock);
+}
+
+/* Unregister a BlockDriverState from its group, removing it from the
+ * list and setting the throttle_state pointer to NULL.
+ *
+ * The group will be destroyed if it's empty after this operation.
+ *
+ * @bs: the BlockDriverState to remove
+ */
+void throttle_group_unregister_bs(BlockDriverState *bs)
+{
+    ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+    int i;
+
+    qemu_mutex_lock(&tg->lock);
+    for (i = 0; i < 2; i++) {
+        if (tg->tokens[i] == bs) {
+            BlockDriverState *token = throttle_group_next_bs(bs);
+            /* Take care of the case where this is the last bs in the group */
+            if (token == bs) {
+                token = NULL;
+            }
+            tg->tokens[i] = token;
+        }
+    }
+
+    /* remove the current bs from the list */
+    QLIST_REMOVE(bs, round_robin);
+    qemu_mutex_unlock(&tg->lock);
+
+    throttle_group_unref(tg);
+    bs->throttle_state = NULL;
+}
+
+static void throttle_groups_init(void)
+{
+    qemu_mutex_init(&throttle_groups_lock);
+}
+
+block_init(throttle_groups_init);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 58b3a5f..1314fd3 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..23a172f
--- /dev/null
+++ b/include/block/throttle-groups.h
@@ -0,0 +1,41 @@ 
+/*
+ * QEMU block throttling group infrastructure
+ *
+ * Copyright (C) Nodalink, EURL. 2014
+ * Copyright (C) Igalia, S.L. 2015
+ *
+ * Authors:
+ *   Benoît Canet <benoit.canet@nodalink.com>
+ *   Alberto Garcia <berto@igalia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef THROTTLE_GROUPS_H
+#define THROTTLE_GROUPS_H
+
+#include "qemu/throttle.h"
+#include "block/block_int.h"
+
+const char *throttle_group_get_name(ThrottleState *ts);
+
+void throttle_group_config(ThrottleState *ts,
+                           ThrottleTimers *tt,
+                           ThrottleConfig *cfg);
+void throttle_group_get_config(ThrottleState *ts, ThrottleConfig *cfg);
+
+void throttle_group_register_bs(BlockDriverState *bs, const char *groupname);
+void throttle_group_unregister_bs(BlockDriverState *bs);
+
+#endif