Re: [Qemu-devel] [PATCH v2 1/3] cpu: Provide vcpu throttling interface

["Jason J. Herne" <jjherne@linux.vnet.ibm.com>]

On 06/09/2015 04:06 AM, Dr. David Alan Gilbert wrote:
> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>> On 06/03/2015 02:11 PM, Jason J. Herne wrote:
>>> On 06/03/2015 02:03 PM, Dr. David Alan Gilbert wrote:
>>>> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>>>>> On 06/03/2015 03:56 AM, Juan Quintela wrote:
>>>>>> "Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote:
>>> ...
>>>>>> We are checking for throotling on each cpu each 10ms.
>>>>>> But on patch 2 we can see that we only change the throotling each
>>>>>> time that we call migration_bitmap_sync(), that only happens each round
>>>>>> through all the pages. Normally auto-converge only matters for machines
>>>>>> with lots of memory, so this is going to happen each more than 10ms (we
>>>>>> change it each 4 passes).  You changed it to each 2 passes, and you add
>>>>>> it a 0.2.  I think  that I would preffer to just have it each single
>>>>>> pass, but add a 0.1 each pass?  simpler and end result would be the
>>>>>> same?
>>>>>>
>>>>>>
>>>>>
>>>>> Well, we certainly could make it run every pass but I think it would get
>>>>> a little too aggressive then. The reason is, we do not increment the
>>>>> throttle
>>>>> rate by adding 0.2 each time. We increment it by multiplying the current
>>>>> rate
>>>>> by 2. So by doing that every pass we are doubling the exponential growth
>>>>> rate. I will admit the numbers I chose are hardly scientific... I
>>>>> chose them
>>>>> because they seemed to provide a decent balance of "throttling
>>>>> aggression"
>>>>> in
>>>>> my workloads.
>>>>
>>>> That's the advantage of making them parameters.
>>>
>>> I see your point. Expecting the user to configure these parameters
>>> seems a bit much. But I guess, in theory, it is better to have the
>>> ability to change them and not need it, than need it and not have it
>>> right?
>>>
>>> So, as you stated earlier these should hook into MigrationParams
>>> somehow? I'll admit this is the first I've seen this construct. If
>>> this is the optimal location for the two controls (x-throttle-initial,
>>> x-throttle-multiplier?) I can add them there. Will keep defaults of
>>> 0.2 for initial and 2.0 for multiplier(is there a better name?)?
>>>
>>
>> So I'm attempting add the initial throttle value and the multiplier to
>> MigrationParameters and I've come across a problem.
>> hmp_migrate_set_parameter assumes all parameters are ints. Apparently
>> floating point is not allowed...
>>
>>      void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>>      {
>>          const char *param = qdict_get_str(qdict, "parameter");
>>          int value = qdict_get_int(qdict, "value");
>>
>> Also from hmp-commands.hx
>>
>>      {
>>          .name       = "migrate_set_parameter",
>>          .args_type  = "parameter:s,value:i",
>>          .params     = "parameter value",
>>          .help       = "Set the parameter for migration",
>>          .mhandler.cmd = hmp_migrate_set_parameter,
>>          .command_completion = migrate_set_parameter_completion,
>>      },
>>
>> I'm hoping someone already has an idea for dealing with this problem? If
>> not, I suppose this is a good add-on for Dave's discussion on redesigning
>> MigrationParameters.
>
> Oh, that's yet another problem; hadn't thought about this one.
> I don't think the suggestions I had in the previous mail would help that one
> either;   It might work if you flipped the type to 's' and then parsed
> that in the hmp code.
>

I could change it to a string, and then parse the data on a case-by-case 
basis in the switch/case logic. I feel like this is making a bad 
situation worse... But I don't see an easy way around it.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)