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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]