ThrottleGroup is converted to an object to allow easy runtime
configuration of throttling filter nodes in the BDS graph using QOM.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block/throttle-groups.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/qemu/throttle.h |   4 +
 2 files changed, 355 insertions(+)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 7883cbb511..60079dc8ea 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -25,9 +25,11 @@
 #include "qemu/osdep.h"
 #include "sysemu/block-backend.h"
 #include "block/throttle-groups.h"
+#include "qemu/throttle-options.h"
 #include "qemu/queue.h"
 #include "qemu/thread.h"
 #include "sysemu/qtest.h"
+#include "qapi/error.h"
 
 /* The ThrottleGroup structure (with its ThrottleState) is shared
  * among different ThrottleGroupMembers and it's independent from
@@ -54,6 +56,7 @@
  * that BlockBackend has throttled requests in the queue.
  */
 typedef struct ThrottleGroup {
+    Object parent_obj;
     char *name; /* This is constant during the lifetime of the group */
 
     QemuMutex lock; /* This lock protects the following four fields */
@@ -562,3 +565,351 @@ static void throttle_groups_init(void)
 }
 
 block_init(throttle_groups_init);
+
+
+static bool throttle_group_exists(const char *name)
+{
+    ThrottleGroup *iter;
+    bool ret = false;
+
+    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)) {
+            ret = true;
+            break;
+        }
+    }
+
+    qemu_mutex_unlock(&throttle_groups_lock);
+    return ret;
+}
+
+typedef struct ThrottleGroupClass {
+    /* private */
+    ObjectClass parent_class;
+    /* public */
+} ThrottleGroupClass;
+
+
+#define DOUBLE 0
+#define UINT64 1
+#define UNSIGNED 2
+
+typedef struct {
+    BucketType type;
+    int size; /* field size */
+    ptrdiff_t offset; /* offset in LeakyBucket struct. */
+} ThrottleParamInfo;
+
+static ThrottleParamInfo throttle_iops_total_info = {
+    THROTTLE_OPS_TOTAL, DOUBLE, offsetof(LeakyBucket, avg),
+};
+
+static ThrottleParamInfo throttle_iops_total_max_info = {
+    THROTTLE_OPS_TOTAL, DOUBLE, offsetof(LeakyBucket, max),
+};
+
+static ThrottleParamInfo throttle_iops_total_max_length_info = {
+    THROTTLE_OPS_TOTAL, UNSIGNED, offsetof(LeakyBucket, burst_length),
+};
+
+static ThrottleParamInfo throttle_iops_read_info = {
+    THROTTLE_OPS_READ, DOUBLE, offsetof(LeakyBucket, avg),
+};
+
+static ThrottleParamInfo throttle_iops_read_max_info = {
+    THROTTLE_OPS_READ, DOUBLE, offsetof(LeakyBucket, max),
+};
+
+static ThrottleParamInfo throttle_iops_read_max_length_info = {
+    THROTTLE_OPS_READ, UNSIGNED, offsetof(LeakyBucket, burst_length),
+};
+
+static ThrottleParamInfo throttle_iops_write_info = {
+    THROTTLE_OPS_WRITE, DOUBLE, offsetof(LeakyBucket, avg),
+};
+
+static ThrottleParamInfo throttle_iops_write_max_info = {
+    THROTTLE_OPS_WRITE, DOUBLE, offsetof(LeakyBucket, max),
+};
+
+static ThrottleParamInfo throttle_iops_write_max_length_info = {
+    THROTTLE_OPS_WRITE, UNSIGNED, offsetof(LeakyBucket, burst_length),
+};
+
+static ThrottleParamInfo throttle_bps_total_info = {
+    THROTTLE_BPS_TOTAL, DOUBLE, offsetof(LeakyBucket, avg),
+};
+
+static ThrottleParamInfo throttle_bps_total_max_info = {
+    THROTTLE_BPS_TOTAL, DOUBLE, offsetof(LeakyBucket, max),
+};
+
+static ThrottleParamInfo throttle_bps_total_max_length_info = {
+    THROTTLE_BPS_TOTAL, UNSIGNED, offsetof(LeakyBucket, burst_length),
+};
+
+static ThrottleParamInfo throttle_bps_read_info = {
+    THROTTLE_BPS_READ, DOUBLE, offsetof(LeakyBucket, avg),
+};
+
+static ThrottleParamInfo throttle_bps_read_max_info = {
+    THROTTLE_BPS_READ, DOUBLE, offsetof(LeakyBucket, max),
+};
+
+static ThrottleParamInfo throttle_bps_read_max_length_info = {
+    THROTTLE_BPS_READ, UNSIGNED, offsetof(LeakyBucket, burst_length),
+};
+
+static ThrottleParamInfo throttle_bps_write_info = {
+    THROTTLE_BPS_WRITE, DOUBLE, offsetof(LeakyBucket, avg),
+};
+
+static ThrottleParamInfo throttle_bps_write_max_info = {
+    THROTTLE_BPS_WRITE, DOUBLE, offsetof(LeakyBucket, max),
+};
+
+static ThrottleParamInfo throttle_bps_write_max_length_info = {
+    THROTTLE_BPS_WRITE, UNSIGNED, offsetof(LeakyBucket, burst_length),
+};
+
+static ThrottleParamInfo throttle_iops_size_info = {
+    0, UINT64, offsetof(ThrottleConfig, op_size),
+};
+
+
+static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
+{
+    char *name = NULL;
+    Error *local_error = NULL;
+    ThrottleGroup *tg = THROTTLE_GROUP(obj);
+
+    name = object_get_canonical_path_component(OBJECT(obj));
+    if (throttle_group_exists(name)) {
+        error_setg(&local_error, "A throttle group with this name already \
+                                  exists.");
+        goto ret;
+    }
+
+    qemu_mutex_lock(&throttle_groups_lock);
+    tg->name = name;
+    qemu_mutex_init(&tg->lock);
+    QLIST_INIT(&tg->head);
+    QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
+    tg->refcount++;
+    qemu_mutex_unlock(&throttle_groups_lock);
+
+ret:
+    error_propagate(errp, local_error);
+    return;
+
+}
+
+static void throttle_group_set(Object *obj, Visitor *v, const char * name,
+        void *opaque, Error **errp)
+
+{
+    ThrottleGroup *tg = THROTTLE_GROUP(obj);
+    ThrottleConfig cfg = tg->ts.cfg;
+    Error *local_err = NULL;
+    ThrottleParamInfo *info = opaque;
+    int64_t value;
+
+    visit_type_int64(v, name, &value, &local_err);
+
+    if (local_err) {
+        goto out;
+    }
+    if (value < 0) {
+        error_setg(&local_err, "%s value must be in range [0, %"PRId64"]",
+                "iops-total", INT64_MAX); /* change option string */
+        goto out;
+    }
+
+    switch (info->size) {
+    case UINT64:
+        {
+            uint64_t *field = (void *)&cfg.buckets[info->type] + info->offset;
+            *field = value;
+        }
+        break;
+    case DOUBLE:
+        {
+            double *field = (void *)&cfg.buckets[info->type] + info->offset;
+            *field = value;
+        }
+        break;
+    case UNSIGNED:
+        {
+            unsigned *field = (void *)&cfg.buckets[info->type] + info->offset;
+            *field = value;
+        }
+    }
+
+    tg->ts.cfg = cfg;
+
+out:
+    error_propagate(errp, local_err);
+}
+
+static void throttle_group_get(Object *obj, Visitor *v, const char *name,
+        void *opaque, Error **errp)
+{
+    ThrottleGroup *tg = THROTTLE_GROUP(obj);
+    ThrottleConfig cfg = tg->ts.cfg;
+    ThrottleParamInfo *info = opaque;
+    int64_t value;
+
+    switch (info->size) {
+    case UINT64:
+        {
+            uint64_t *field = (void *)&cfg.buckets[info->type] + info->offset;
+            value = *field;
+        }
+        break;
+    case DOUBLE:
+        {
+            double *field = (void *)&cfg.buckets[info->type] + info->offset;
+            value = *field;
+        }
+        break;
+    case UNSIGNED:
+        {
+            unsigned *field = (void *)&cfg.buckets[info->type] + info->offset;
+            value = *field;
+        }
+    }
+
+    visit_type_int64(v, name, &value, errp);
+
+}
+
+static void throttle_group_init(Object *obj)
+{
+    ThrottleGroup *tg = THROTTLE_GROUP(obj);
+    throttle_init(&tg->ts);
+}
+
+static void throttle_group_class_init(ObjectClass *klass, void *class_data)
+{
+    UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
+
+    ucc->complete = throttle_group_obj_complete;
+    /* iops limits */
+    object_class_property_add(klass, QEMU_OPT_IOPS_TOTAL, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_iops_total_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_IOPS_TOTAL_MAX, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_iops_total_max_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_IOPS_TOTAL_MAX_LENGTH, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_iops_total_max_length_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_IOPS_READ, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_iops_read_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_IOPS_READ_MAX, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_iops_read_max_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_IOPS_READ_MAX_LENGTH, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_iops_read_max_length_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_IOPS_WRITE, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_iops_write_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_IOPS_WRITE_MAX, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_iops_write_max_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_IOPS_WRITE_MAX_LENGTH, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_iops_write_max_length_info, &error_abort);
+    /* bps limits */
+    object_class_property_add(klass, QEMU_OPT_BPS_TOTAL, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_bps_total_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_BPS_TOTAL_MAX, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_bps_total_max_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_BPS_TOTAL_MAX_LENGTH, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_bps_total_max_length_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_BPS_READ, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_bps_read_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_BPS_READ_MAX, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_bps_read_max_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_BPS_READ_MAX_LENGTH, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_bps_read_max_length_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_BPS_WRITE, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_bps_write_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_BPS_WRITE_MAX, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_bps_write_max_info, &error_abort);
+    object_class_property_add(klass, QEMU_OPT_BPS_WRITE_MAX_LENGTH, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_bps_write_max_length_info, &error_abort);
+    /* rest */
+    object_class_property_add(klass, QEMU_OPT_IOPS_SIZE, "int",
+            throttle_group_get,
+            throttle_group_set,
+            NULL, &throttle_iops_size_info, &error_abort);
+}
+
+
+static void throttle_group_finalize(Object *obj)
+{
+    ThrottleGroup *tg = THROTTLE_GROUP(obj);
+
+    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);
+
+}
+
+static const TypeInfo throttle_group_info = {
+   .name = TYPE_THROTTLE_GROUP,
+   .parent = TYPE_OBJECT,
+   .class_init = throttle_group_class_init,
+   .instance_size = sizeof(ThrottleGroup),
+   .instance_init = throttle_group_init,
+   .instance_finalize = throttle_group_finalize,
+   .interfaces = (InterfaceInfo[]) {
+       { TYPE_USER_CREATABLE },
+       { }
+   },
+};
+
+static void throttle_group_register_types(void)
+{
+    qemu_mutex_init(&throttle_groups_lock);
+    type_register_static(&throttle_group_info);
+}
+
+type_init(throttle_group_register_types);
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 3e92d4d4eb..dd56baeb35 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -28,6 +28,8 @@
 #include "qemu-common.h"
 #include "qemu/timer.h"
 #include "qemu/coroutine.h"
+#include "qom/object.h"
+#include "qom/object_interfaces.h"
 
 #define THROTTLE_VALUE_MAX 1000000000000000LL
 
@@ -180,4 +182,6 @@ typedef struct ThrottleGroupMember {
 
 } ThrottleGroupMember;
 
+#define TYPE_THROTTLE_GROUP "throttling-group"
+#define THROTTLE_GROUP(obj) OBJECT_CHECK(ThrottleGroup, (obj), TYPE_THROTTLE_GROUP)
 #endif
-- 
2.11.0

On Fri, Jun 23, 2017 at 03:46:56PM +0300, Manos Pitsidianakis wrote:
> +static bool throttle_group_exists(const char *name)
> +{
> +    ThrottleGroup *iter;
> +    bool ret = false;
> +
> +    qemu_mutex_lock(&throttle_groups_lock);

Not sure if this lock or the throttle_groups list are necessary.

Have you seen iothread.c:qmp_query_iothreads()?  All objects are put
into a container (the parent object), so you can just iterate over its
children.  There's no need for a separate list because QOM already has
all the objects.

> +typedef struct ThrottleGroupClass {
> +    /* private */
> +    ObjectClass parent_class;
> +    /* public */
> +} ThrottleGroupClass;

This is unused?

> +
> +
> +#define DOUBLE 0
> +#define UINT64 1
> +#define UNSIGNED 2
> +
> +typedef struct {
> +    BucketType type;
> +    int size; /* field size */

sizeof(double) == sizeof(uint64_t) == 8.

This is a datatype field, not a size.

> +static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
> +{
> +    char *name = NULL;
> +    Error *local_error = NULL;
> +    ThrottleGroup *tg = THROTTLE_GROUP(obj);
> +
> +    name = object_get_canonical_path_component(OBJECT(obj));
> +    if (throttle_group_exists(name)) {
> +        error_setg(&local_error, "A throttle group with this name already \
> +                                  exists.");
> +        goto ret;
> +    }

QOM should enforce unique id=<ID>.  I don't think this is necessary.

> +
> +    qemu_mutex_lock(&throttle_groups_lock);
> +    tg->name = name;
> +    qemu_mutex_init(&tg->lock);
> +    QLIST_INIT(&tg->head);
> +    QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
> +    tg->refcount++;
> +    qemu_mutex_unlock(&throttle_groups_lock);
> +
> +ret:
> +    error_propagate(errp, local_error);
> +    return;
> +
> +}
> +
> +static void throttle_group_set(Object *obj, Visitor *v, const char * name,
> +        void *opaque, Error **errp)
> +
> +{
> +    ThrottleGroup *tg = THROTTLE_GROUP(obj);
> +    ThrottleConfig cfg = tg->ts.cfg;
> +    Error *local_err = NULL;
> +    ThrottleParamInfo *info = opaque;
> +    int64_t value;

What happens if this property is set while QEMU is already running?

> +
> +    visit_type_int64(v, name, &value, &local_err);
> +
> +    if (local_err) {
> +        goto out;
> +    }
> +    if (value < 0) {
> +        error_setg(&local_err, "%s value must be in range [0, %"PRId64"]",
> +                "iops-total", INT64_MAX); /* change option string */

iops-total? :)

> +        goto out;
> +    }

This doesn't validate inputs properly for non int64_t types.

I'm also worried that the command-line bps=,iops=,... options do not
have unsigned or double types.  Input ranges and validation should match
the QEMU command-line (I know this is a bit of a pain with QOM since the
property types are different from QEMU option types).

On Mon, Jun 26, 2017 at 03:52:34PM +0100, Stefan Hajnoczi wrote:
>On Fri, Jun 23, 2017 at 03:46:56PM +0300, Manos Pitsidianakis wrote:
>> +static bool throttle_group_exists(const char *name)
>> +{
>> +    ThrottleGroup *iter;
>> +    bool ret = false;
>> +
>> +    qemu_mutex_lock(&throttle_groups_lock);
>
>Not sure if this lock or the throttle_groups list are necessary.
>
>Have you seen iothread.c:qmp_query_iothreads()?  All objects are put
>into a container (the parent object), so you can just iterate over its
>children.  There's no need for a separate list because QOM already has
>all the objects.
>

>> +static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
>> +{
>> +    char *name = NULL;
>> +    Error *local_error = NULL;
>> +    ThrottleGroup *tg = THROTTLE_GROUP(obj);
>> +
>> +    name = object_get_canonical_path_component(OBJECT(obj));
>> +    if (throttle_group_exists(name)) {
>> +        error_setg(&local_error, "A throttle group with this name already \
>> +                                  exists.");
>> +        goto ret;
>> +    }
>
>QOM should enforce unique id=<ID>.  I don't think this is necessary.
>
>> +
>> +    qemu_mutex_lock(&throttle_groups_lock);
>> +    tg->name = name;
>> +    qemu_mutex_init(&tg->lock);
>> +    QLIST_INIT(&tg->head);
>> +    QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
>> +    tg->refcount++;
>> +    qemu_mutex_unlock(&throttle_groups_lock);
>> +

Sorry for the multiple replies but I just remembered this.

This is necessary because throttle groups are created by other 
interfaces as well. Of course block/throttle-groups.c could use only QOM 
objects internally to eliminate the housekeeping.

On Mon 26 Jun 2017 06:58:32 PM CEST, Manos Pitsidianakis wrote:
> On Mon, Jun 26, 2017 at 03:52:34PM +0100, Stefan Hajnoczi wrote:
>>On Fri, Jun 23, 2017 at 03:46:56PM +0300, Manos Pitsidianakis wrote:
>>> +static bool throttle_group_exists(const char *name)
>>> +{
>>> +    ThrottleGroup *iter;
>>> +    bool ret = false;
>>> +
>>> +    qemu_mutex_lock(&throttle_groups_lock);
>>
>>Not sure if this lock or the throttle_groups list are necessary.

As Manos says accesses to the throttle_groups list need to be locked.

What I don't like at first sight is the code duplication in
throttle_group_incref() and throttle_group_obj_complete().

>>Have you seen iothread.c:qmp_query_iothreads()?  All objects are put
>>into a container (the parent object), so you can just iterate over its
>>children.  There's no need for a separate list because QOM already has
>>all the objects.

I haven't looked into this yet but it could be a solution.

Berto

On Tue, Jun 27, 2017 at 06:05:55PM +0200, Alberto Garcia wrote:
>On Mon 26 Jun 2017 06:58:32 PM CEST, Manos Pitsidianakis wrote:
>> On Mon, Jun 26, 2017 at 03:52:34PM +0100, Stefan Hajnoczi wrote:
>>>On Fri, Jun 23, 2017 at 03:46:56PM +0300, Manos Pitsidianakis wrote:
>>>> +static bool throttle_group_exists(const char *name)
>>>> +{
>>>> +    ThrottleGroup *iter;
>>>> +    bool ret = false;
>>>> +
>>>> +    qemu_mutex_lock(&throttle_groups_lock);
>>>
>>>Not sure if this lock or the throttle_groups list are necessary.
>
>As Manos says accesses to the throttle_groups list need to be locked.
>
>What I don't like at first sight is the code duplication in
>throttle_group_incref() and throttle_group_obj_complete().
>
>>>Have you seen iothread.c:qmp_query_iothreads()?  All objects are put
>>>into a container (the parent object), so you can just iterate over its
>>>children.  There's no need for a separate list because QOM already has
>>>all the objects.
>
>I haven't looked into this yet but it could be a solution.

If throttle_groups_register_blk() uses QOM instead of calling 
throttle_group_incref() the duplication could be eliminated. Then all of 
throttle-groups.c uses QOM internally. I don't see any reason why not do 
this.

>Berto
>

On Tue, Jun 27, 2017 at 06:05:55PM +0200, Alberto Garcia wrote:
> On Mon 26 Jun 2017 06:58:32 PM CEST, Manos Pitsidianakis wrote:
> > On Mon, Jun 26, 2017 at 03:52:34PM +0100, Stefan Hajnoczi wrote:
> >>On Fri, Jun 23, 2017 at 03:46:56PM +0300, Manos Pitsidianakis wrote:
> >>> +static bool throttle_group_exists(const char *name)
> >>> +{
> >>> +    ThrottleGroup *iter;
> >>> +    bool ret = false;
> >>> +
> >>> +    qemu_mutex_lock(&throttle_groups_lock);
> >>
> >>Not sure if this lock or the throttle_groups list are necessary.
> 
> As Manos says accesses to the throttle_groups list need to be locked.

Explicit locking is only necessary if the list is accessed outside the
QEMU global mutex.  If the monitor is the only thing that accesses the
list then a lock is not necessary.

Anyway, this point might be moot if every ThrottleGroup is a QOM object
and we drop this code in favor of using QOM APIs to find and iterate
over objects.

Stefan

On Mon, Jun 26, 2017 at 07:58:32PM +0300, Manos Pitsidianakis wrote:
> On Mon, Jun 26, 2017 at 03:52:34PM +0100, Stefan Hajnoczi wrote:
> > On Fri, Jun 23, 2017 at 03:46:56PM +0300, Manos Pitsidianakis wrote:
> > > +static bool throttle_group_exists(const char *name)
> > > +{
> > > +    ThrottleGroup *iter;
> > > +    bool ret = false;
> > > +
> > > +    qemu_mutex_lock(&throttle_groups_lock);
> > 
> > Not sure if this lock or the throttle_groups list are necessary.
> > 
> > Have you seen iothread.c:qmp_query_iothreads()?  All objects are put
> > into a container (the parent object), so you can just iterate over its
> > children.  There's no need for a separate list because QOM already has
> > all the objects.
> > 
> 
> > > +static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
> > > +{
> > > +    char *name = NULL;
> > > +    Error *local_error = NULL;
> > > +    ThrottleGroup *tg = THROTTLE_GROUP(obj);
> > > +
> > > +    name = object_get_canonical_path_component(OBJECT(obj));
> > > +    if (throttle_group_exists(name)) {
> > > +        error_setg(&local_error, "A throttle group with this name already \
> > > +                                  exists.");
> > > +        goto ret;
> > > +    }
> > 
> > QOM should enforce unique id=<ID>.  I don't think this is necessary.
> > 
> > > +
> > > +    qemu_mutex_lock(&throttle_groups_lock);
> > > +    tg->name = name;
> > > +    qemu_mutex_init(&tg->lock);
> > > +    QLIST_INIT(&tg->head);
> > > +    QTAILQ_INSERT_TAIL(&throttle_groups, tg, list);
> > > +    tg->refcount++;
> > > +    qemu_mutex_unlock(&throttle_groups_lock);
> > > +
> 
> Sorry for the multiple replies but I just remembered this.
> 
> This is necessary because throttle groups are created by other interfaces as
> well. Of course block/throttle-groups.c could use only QOM objects
> internally to eliminate the housekeeping.

I suggest all throttle group objects are visible in QOM.  Who are the
non-QOM users?

I have CCed Pradeep who has been working on virtio-9p throttling.

Pradeep: You may be interested in this series, which makes the throttle
group a QOM object (-object throttle-group,id=group0,bps=1235678).  In
other words, groups are becoming first-class objects instead of being
hidden behind the member devices that use them.

Stefan