On Fri 18 Aug 2017 05:10:17 AM CEST, Manos Pitsidianakis wrote:
> * If no ThrottleGroup is found with the given name a new one is
> * created.
> *
> - * @name: the name of the ThrottleGroup
> + * This function edits throttle_groups and must be called under the global
> + * mutex.
> + *
> + * @name: the name of the ThrottleGroup, NULL means a new anonymous group
> will
> + * be created.
> * @ret: the ThrottleState member of the ThrottleGroup
> */
> ThrottleState *throttle_group_incref(const char *name)
If we're not going to have anonymous groups in the end this patch needs
changes (name == NULL is no longer allowed).
> +/* This function edits throttle_groups and must be called under the global
> + * mutex */
> +static void throttle_group_obj_complete(UserCreatable *obj, Error **errp)
> +{
> + ThrottleGroup *tg = THROTTLE_GROUP(obj), *iter;
> + ThrottleConfig *cfg = &tg->ts.cfg;
> +
> + /* set group name to object id if it exists */
> + if (!tg->name && tg->parent_obj.parent) {
> + tg->name = object_get_canonical_path_component(OBJECT(obj));
> + }
> +
> + if (tg->name) {
> + /* error if name is duplicate */
> + QTAILQ_FOREACH(iter, &throttle_groups, list) {
> + if (!g_strcmp0(tg->name, iter->name) && tg != iter) {
I'm just nitpicking here :) but if you change this and put 'tg != iter'
first you'll save one unnecessary string comparison.
> + error_setg(errp, "A group with this name already exists");
> + return;
> + }
> + }
> + }
> +
> + /* unfix buckets to check validity */
> + throttle_get_config(&tg->ts, cfg);
> + if (!throttle_is_valid(cfg, errp)) {
> + return;
> + }
> + /* fix buckets again */
> + throttle_config(&tg->ts, tg->clock_type, cfg);
throttle_get_config(ts, cfg) makes a copy of the existing configuration,
but the cfg pointer that you are passing already points to the existing
configuration, so in practice you are doing
*(ts->cfg) = *(ts->cfg);
throttle_unfix_bucket(...);
and since you "unfixed" the configuration, then you need undo the whole
thing by setting the config again:
*(ts->cfg) = *(ts->cfg);
throttle_fix_bucket(...);
You should declare a local ThrottleConfig variable, copy the config
there, and run throttle_is_valid() on that copy. The final
throttle_config() call is unnecessary.
Once the patches I sent yesterday are merged we'll be able to skip the
throttle_get_config() call and do throttle_is_valid(&tg->ts.cfg, errp)
directly.
> +static void throttle_group_set(Object *obj, Visitor *v, const char * name,
> + void *opaque, Error **errp)
> +
> +{
> + ThrottleGroup *tg = THROTTLE_GROUP(obj);
> + ThrottleConfig cfg;
> + ThrottleParamInfo *info = opaque;
> + Error *local_err = NULL;
> + int64_t value;
> +
> + /* If we have finished initialization, don't accept individual property
> + * changes through QOM. Throttle configuration limits must be set in one
> + * transaction, as certain combinations are invalid.
> + */
> + if (tg->is_initialized) {
> + error_setg(&local_err, "Property cannot be set after
> initialization");
> + goto ret;
> + }
> +
> + visit_type_int64(v, name, &value, &local_err);
> + if (local_err) {
> + goto ret;
> + }
> + if (value < 0) {
> + error_setg(&local_err, "Property values cannot be negative");
> + goto ret;
> + }
> +
> + cfg = tg->ts.cfg;
> + switch (info->data_type) {
> + 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:
> + {
> + if (value > UINT_MAX) {
> + error_setg(&local_err, "%s value must be in the"
> + "range [0, %u]", info->name,
> UINT_MAX);
> + goto ret;
> + }
> + unsigned *field = (void *)&cfg.buckets[info->type] +
> info->offset;
> + *field = value;
> + }
> + }
> +
> + tg->ts.cfg = cfg;
There's a bit of black magic here :) you have a user-defined enumeration
(UNSIGNED, DOUBLE, UINT64) to identify C types and I'm worried about
what happens if the data types of LeakyBucket change, will we be able to
detect the problem?
Out of the blue I can think of the following alternative:
- There's 6 different buckets (we have BucketType listing them)
- There's 3 values we can set in each bucket (max, avg,
burst_length). For those we can have an internal enumeration
(probably with one additional value for iops-size).
- In the 'properties' array, for each property we know its category
(and I mean: avg, max, burst-lenght, iops-size) and the bucket
where they belong.
- In throttle_group_set() the switch could be something like this:
switch (info->category) {
case THROTTLE_VALUE_AVG:
cfg->buckets[info->bkt_type].avg = value;
break;
case THROTTLE_VALUE_MAX:
cfg->buckets[info->bkt_type].max = value;
break;
case THROTTLE_VALUE_BURST_LENGTH:
/* Code here to check that value <= UINT_MAX */
cfg->buckets[info->bkt_type].iops_length = value;
break;
case THROTTLE_VALUE_IOPS_SIZE:
cfg->op_size = value;
}
> +static void throttle_group_get(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
And the same approach here, of course.
> +static void throttle_group_set_limits(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +
> +{
> + ThrottleGroup *tg = THROTTLE_GROUP(obj);
> + ThrottleConfig cfg;
> + ThrottleLimits *arg = NULL;
> + Error *local_err = NULL;
> +
> + arg = g_new0(ThrottleLimits, 1);
Do you need to allocate ThrottleLimits in the heap?
> +static void throttle_group_get_limits(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + ThrottleGroup *tg = THROTTLE_GROUP(obj);
> + ThrottleConfig cfg;
> + ThrottleLimits *arg = NULL;
> +
> + qemu_mutex_lock(&tg->lock);
> + throttle_get_config(&tg->ts, &cfg);
> + qemu_mutex_unlock(&tg->lock);
> +
> + arg = g_new0(ThrottleLimits, 1);
> + throttle_config_to_limits(&cfg, arg);
> +
> + visit_type_ThrottleLimits(v, name, &arg, errp);
> + g_free(arg);
Same question here.
Berto
