From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46906) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YakDB-0002pk-2P for qemu-devel@nongnu.org; Wed, 25 Mar 2015 08:15:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YakD7-0008TT-0i for qemu-devel@nongnu.org; Wed, 25 Mar 2015 08:15:04 -0400 Received: from smtp3.mundo-r.com ([212.51.32.191]:48704 helo=smtp4.mundo-r.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YakD6-0008RO-Pn for qemu-devel@nongnu.org; Wed, 25 Mar 2015 08:15:00 -0400 Date: Wed, 25 Mar 2015 13:14:58 +0100 From: Alberto Garcia Message-ID: <20150325121458.GA29748@igalia.com> References: <3c960428503cfd363a55f72678a2f6a710d94a3e.1426000801.git.berto@igalia.com> <20150324150307.GD1493@stefanha-thinkpad.redhat.com> <20150324153348.GA20218@igalia.com> <20150325120130.GA22940@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150325120130.GA22940@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/6] throttle: Add throttle group infrastructure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Stefan Hajnoczi , qemu-devel@nongnu.org > > > > +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