From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57355) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XcYTJ-00020T-GK for qemu-devel@nongnu.org; Fri, 10 Oct 2014 07:35:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XcYTD-0008UF-BA for qemu-devel@nongnu.org; Fri, 10 Oct 2014 07:34:57 -0400 Received: from dew.nodalink.com ([95.130.14.197]:40423) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XcYTD-0008Tt-5b for qemu-devel@nongnu.org; Fri, 10 Oct 2014 07:34:51 -0400 Date: Fri, 10 Oct 2014 11:35:10 +0000 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20141010113510.GC22893@nodalink.com> References: <1412688279-8312-1-git-send-email-benoit.canet@nodalink.com> <1412688279-8312-8-git-send-email-benoit.canet@nodalink.com> <20141008065338.GD15829@fam-t430.nay.redhat.com> <20141008110553.GA32479@nodalink.com> <20141009085822.GA4692@fam-t430.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20141009085822.GA4692@fam-t430.nay.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v1 7/8] throttle: Add throttle group support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, stefanha@redhat.com, qemu-devel@nongnu.org, =?iso-8859-1?Q?Beno=EEt?= Canet On Thu, Oct 09, 2014 at 04:58:22PM +0800, Fam Zheng wrote: > On Wed, 10/08 11:05, Beno=EEt Canet wrote: > > On Wed, Oct 08, 2014 at 02:53:38PM +0800, Fam Zheng wrote: > > >=20 > > > Does this mean that after this series, all the throttle_states must= be > > > contained inside its own throttle group? If so, we could embed Thro= ttleGroup > > > fields in ThrottleState. > > >=20 > > > It's weird when a function called throttle_group_compare takes a pa= rameter of > > > ThrottleState pointer, and cast it back to ThrottleGroup with conta= iner_of. > >=20 > > It's done like this to fullfill a design goal: the throttle should be= reusable > > without the groups and any reference to block related stuff. > > So it's just a way to split the responsabilities. >=20 > I see. >=20 > Having both ThrottleGroup and ThrottleState interfaces is more complica= ted than > just use ThrottleGroup, where a one-member group is exactly the same as > ThrottleState. Here my interest conflict between a short term strategy (comply and getti= ng these patches merged asap) and the technical advantage of keeping ThrottleState= and ThrottleGroup separated. orthogonality ------------- An advantage to keep ThrottleState and ThrottleGroup separated is that the two roles of implementing the throttle itself and implementing the be= havior of a group are keep separated. As a result each piece of code is simpler to write, review and test. genericity ---------- When writing the initial throttle code I carefully designed it to be inde= pendant of BlockDriverState. As a result the throttle code lives in util/ and is easilly reusable into= whatever we want (a device model or network throttling). It is mean, designed and written to be as generic as possible. So yes merging ThrottleState and ThrottleGroup would result in the seemli= ngly nice thing that one structure is simpler than two structure. But it would also imply the fact that I really hate that this pesky BlockDriverState structure would become tied to ThrottleState. If we do this ThrottleState would move out of util/ to block/ and we woul= d have lost the ability to reuse this infrastructure. It would be a short t= erm gain and a long term loss. Another fact is that the rationale "1 structure is simpler than 2" is not= alway relevant: look for BlockBackend that we are desesperately trying to move = out of BlockDriverState. Best regards Beno=EEt >=20 > Fam