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

[Benoît Canet]

On Thu, Oct 09, 2014 at 04:58:22PM +0800, Fam Zheng wrote:
> On Wed, 10/08 11:05, Benoît Canet wrote:
> > On Wed, Oct 08, 2014 at 02:53:38PM +0800, Fam Zheng wrote:
> > > 
> > > Does this mean that after this series, all the throttle_states must be
> > > contained inside its own throttle group? If so, we could embed 
> > > ThrottleGroup
> > > fields in ThrottleState.
> > > 
> > > It's weird when a function called throttle_group_compare takes a 
> > > parameter of
> > > ThrottleState pointer, and cast it back to ThrottleGroup with 
> > > container_of.
> > 
> > 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.
> 
> I see.
> 
> Having both ThrottleGroup and ThrottleState interfaces is more complicated 
> 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 getting 
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 behavior
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 independant
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 seemlingly 
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 would
have lost the ability to reuse this infrastructure. It would be a short term 
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ît
> 
> Fam