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

[Alberto Garcia <berto@igalia.com>]

> > > > +typedef struct ThrottleGroup {
> > > > +    char *name; /* This is constant during the lifetime of the group */
> > > 
> > > Is this also protected by throttle_groups_lock?
> > > 
> > > I guess throttle_groups_lock must be held in order to read this
> > > field - otherwise there is a risk that the object is freed
> > > unless you have already incremented the refcount.
> > 
> > The creation and destruction of ThrottleGroup objects are
> > protected by throttle_groups_lock. That includes handling the
> > memory for that field and its contents.
> > 
> > Once the ThrottleGroup is created the 'name' field doesn't need
> > any additional locking since it remains constant during the
> > lifetime of the group.
> > 
> > The only way to read it from the outside is
> > throttle_group_get_name() and that's safe (until you release the
> > reference to the group, that is).
> 
> Right, the race condition is when the group is released.
> 
> Looking at this again, the assumption isn't that
> throttle_groups_lock is held.  The AioContext lock is held by
> throttle_group_get_name() users and that's why there is no race when
> releasing the reference.
> 
> If someone adds a throttle_group_get_name() caller in the future
> without holding AioContext, then we'd be in trouble.  That is why
> documenting the locking constraints is useful.

But that would only happen if you access bs->throttle_state without
holding bs's AioContext. I understand that it's implicit that you
should hold the context there.

Maybe I can update the throttle_group_* API to use BlockDriverState
in all cases instead of ThrottleState, it's probably more clear like
that.

Berto