| Message ID | 06fbcac6a26a64cce978e54195452846496323c6.1423842044.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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