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

[Alberto Garcia <berto@igalia.com>]

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