Re: [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Wed, Mar 04, 2015 at 02:53:42PM +0100, Alberto Garcia wrote:
> On Tue, Mar 03, 2015 at 03:00:06PM -0600, Stefan Hajnoczi wrote:
> 
> > > +    throttle_group_lock(bs->throttle_state);
> > > +    bdrv_throttle_group_remove(bs);
> > > +    throttle_group_unlock(bs->throttle_state);
> > > +
> > > +    throttle_group_unref(bs->throttle_state);
> > > +    bs->throttle_state = NULL;
> > > +
> > >      throttle_timers_destroy(&bs->throttle_timers);
> > >  }
> > >  
> > >  static void bdrv_throttle_read_timer_cb(void *opaque)
> > >  {
> > >      BlockDriverState *bs = opaque;
> > > -    throttle_timer_fired(&bs->throttle_state, false);
> > > +
> > > +    throttle_group_lock(bs->throttle_state);
> > > +    throttle_timer_fired(bs->throttle_state, false);
> > > +    throttle_group_unlock(bs->throttle_state);
> > 
> > This pattern suggests throttle_timer_fired() should acquire the lock
> > internally instead.
> 
> The idea is that the ThrottleState code itself doesn't know anything
> about locks or groups. As I understood it Benoît designed the
> ThrottleState code to be independent from the block layer and reusable
> for other things (that's why it's in util/).

Then ThrottleGroup could offer an API throttle_group_timer_fired() that
does the locking.

The advantage of encapsulating locking in ThrottleGroup is that callers
don't have to remember the take the lock.  (But they must still be
careful about sequences of calls which will not be atomic.)

Stefan

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