From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43651) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSpqP-0002td-3k for qemu-devel@nongnu.org; Tue, 03 Mar 2015 11:38:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YSpqM-00044w-ED for qemu-devel@nongnu.org; Tue, 03 Mar 2015 11:38:53 -0500 Received: from mail-wi0-x229.google.com ([2a00:1450:400c:c05::229]:40259) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSpqM-00044h-1q for qemu-devel@nongnu.org; Tue, 03 Mar 2015 11:38:50 -0500 Received: by wiwl15 with SMTP id l15so24285545wiw.5 for ; Tue, 03 Mar 2015 08:38:49 -0800 (PST) Date: Tue, 3 Mar 2015 10:38:45 -0600 From: Stefan Hajnoczi Message-ID: <20150303163845.GD15846@stefanha-thinkpad.redhat.com> References: <06fbcac6a26a64cce978e54195452846496323c6.1423842044.git.berto@igalia.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="OaZoDhBhXzo6bW1J" Content-Disposition: inline In-Reply-To: <06fbcac6a26a64cce978e54195452846496323c6.1423842044.git.berto@igalia.com> Subject: Re: [Qemu-devel] [PATCH 2/9] throttle: Add throttle group infrastructure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: Kevin Wolf , Stefan Hajnoczi , qemu-devel@nongnu.org, =?iso-8859-1?Q?Beno=EEt?= Canet --OaZoDhBhXzo6bW1J Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Feb 13, 2015 at 06:06:10PM +0200, Alberto Garcia wrote: > From: Beno=EEt Canet >=20 > The throttle_group_incref increment the refcount of a throttle group give= n it's s/it's/its/ http://www.its-not-its.info/ > +typedef struct ThrottleGroup { > + char name[32]; > + ThrottleState ts; > + uint64_t refcount; > + QTAILQ_ENTRY(ThrottleGroup) list; > + QLIST_HEAD(, BlockDriverState) head; > + BlockDriverState *tokens[2]; /* current round-robin tokens */ > + QemuMutex lock; /* Used to synchronize all elements belonging to a g= roup */ Not sure what this comment means. Which fields are protected by lock? I think refcount is not protected by lock, since it is incremented in throttle_group_incref() without holding the lock. > +} ThrottleGroup; > + > +static QTAILQ_HEAD(, ThrottleGroup) throttle_groups =3D > + QTAILQ_HEAD_INITIALIZER(throttle_groups); Is throttle_groups protected by the QEMU global mutex? It would be helpful to add a comment. > + > +/* increments a ThrottleGroup reference count given it's name s/it's/its/ > + * > + * If no ThrottleGroup is found with the given name a new one is created. > + * > + * @name: the name of the ThrottleGroup > + * @ret: the ThrottleGroup's ThrottleState address > + */ > +ThrottleState *throttle_group_incref(const char *name) > +{ > + ThrottleGroup *tg; > + > + /* return the correct ThrottleState if a group with this name exists= */ > + QTAILQ_FOREACH(tg, &throttle_groups, list) { > + /* group not found -> continue */ > + if (strcmp(name, tg->name)) { > + continue; > + } > + /* group found -> increment it's refcount and return ThrottleSta= te */ > + tg->refcount++; > + return &tg->ts; > + } > + > + /* throttle group not found -> prepare new entry */ > + tg =3D g_new0(ThrottleGroup, 1); > + pstrcpy(tg->name, sizeof(tg->name), name); Silently truncating to 32 chars is confusing. Please use g_strdup() and g_free() the string when refcount reaches 0. > + qemu_mutex_init(&tg->lock); > + throttle_init(&tg->ts); > + QLIST_INIT(&tg->head); > + tg->refcount =3D 1; > + > + /* insert new entry in the list */ > + QTAILQ_INSERT_TAIL(&throttle_groups, tg, list); It is safest to hold tg->lock before adding the group to the list. This way there is a memory barrier and other threads will not access the group until we've finished adding it to the list. The memory barrier is important so other threads don't see old memory values for the group's fields. > + > + /* return newly allocated ThrottleState */ > + return &tg->ts; > +} > + > +/* decrement a ThrottleGroup given it's ThrottleState address s/it's/its/ > + * > + * When the refcount reach zero the ThrottleGroup is destroyed > + * > + * @ts: The ThrottleState address belonging to the ThrottleGroup to unr= ef > + * @ret: true on success else false > + */ > +bool throttle_group_unref(ThrottleState *ts) > +{ > + ThrottleGroup *tg; > + bool found =3D false; > + > + /* Find the ThrottleGroup of the given ThrottleState */ > + QTAILQ_FOREACH(tg, &throttle_groups, list) { > + /* correct group found stop iterating */ > + if (&tg->ts =3D=3D ts) { > + qemu_mutex_lock(&tg->lock); > + found =3D true; > + break; > + } > + } > + > + /* If the ThrottleState was not found something is seriously broken = */ > + if (!found) { > + return false; > + } Please correct me if I'm wrong but I suggest: Make this function void and replace this statement with assert(found). This case should never happen and I doubt callers will be able to handle the error case. > +/* Compare a name with a given ThrottleState group name > + * > + * @ts: the throttle state whose group we are inspecting > + * @name: the name to compare > + * @ret: true if names are equal else false > + */ > +bool throttle_group_compare(ThrottleState *ts, const char *name) Normally a plain "compare" function checks if two values of the same type are equal. The name throttle_group_compare_name() would be clearer. > +{ > + ThrottleGroup *tg =3D container_of(ts, ThrottleGroup, ts); Why can this function use container_of() while throttle_group_unref() has to loop over all ThrottleGroups to find ts? > +/* Used to lock a ThrottleState's ThrottleGroup > + * > + * @ts: the ThrottleState the code is working on > + */ > +void throttle_group_lock(ThrottleState *ts) > +{ > + ThrottleGroup *tg =3D container_of(ts, ThrottleGroup, ts); > + qemu_mutex_lock(&tg->lock); > +} lock/unlock functions are sometimes an indication that the calling code should really be moved into this file. I'm not sure yet since I haven't read all the patches, but it is a little suspicious. --OaZoDhBhXzo6bW1J Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJU9eOVAAoJEJykq7OBq3PI7wUH/iaKJdwQy2FGjAvStoESnh4w RwR7znI6BZfxXBQj7kaVCqTk/sHxBI5sfth30EJFLOLzIssLdUeg5Br9AJ0QqAJP 0k3PScc2R5IgSiECv96147EajeFiwiUZe9SA4f7J5foWhZ6iYGKFoEDXVA9Izn4h xtpz0iPaKObWkIvajLDkgaD7/wexu0OiH/twHoeH82nEUuOQoKs4MPsK8sW2qDDD eHdZOgP17ryc0U7vys9/Mw2iEaNjC9wFzP6VE3kJoRvrs61/9hgIOjT+AVYT/le3 n76ZvzK2mgF/fd/EJ75/dFNaGwJpjm9z1DpSy9DnNGFv4n1KLXkYSe8F9e+BZGs= =E23P -----END PGP SIGNATURE----- --OaZoDhBhXzo6bW1J--