From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:45548)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <berto@igalia.com>) id 1YT9kC-00059Q-VN
	for qemu-devel@nongnu.org; Wed, 04 Mar 2015 08:53:49 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <berto@igalia.com>) id 1YT9k9-0004rd-Fk
	for qemu-devel@nongnu.org; Wed, 04 Mar 2015 08:53:48 -0500
Received: from smtp3.mundo-r.com ([212.51.32.191]:64847 helo=smtp4.mundo-r.com)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <berto@igalia.com>) id 1YT9k9-0004rE-9P
	for qemu-devel@nongnu.org; Wed, 04 Mar 2015 08:53:45 -0500
Date: Wed, 4 Mar 2015 14:53:42 +0100
From: Alberto Garcia <berto@igalia.com>
Message-ID: <20150304135341.GA11084@igalia.com>
References: <cover.1423842044.git.berto@igalia.com>
	<142929b6b5adbf371309043d23864c22b0afdec4.1423842044.git.berto@igalia.com>
	<20150303210006.GG15846@stefanha-thinkpad.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20150303210006.GG15846@stefanha-thinkpad.redhat.com>
Subject: Re: [Qemu-devel] [PATCH 7/9] throttle: Add throttle group support
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>

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/).

Berto