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

[Stefan Hajnoczi]

On Tue, Mar 24, 2015 at 04:33:48PM +0100, Alberto Garcia wrote:
> On Tue, Mar 24, 2015 at 03:03:07PM +0000, Stefan Hajnoczi wrote:
> 
> > > +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.

Stefan

pgpRE_xtjklmb.pgp
Description: PGP signature