Re: [Qemu-devel] [PATCH 2/9] throttle: Add throttle group infrastructure

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- 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 --]