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

[Markus Armbruster]

"Jason J. Herne" <address@hidden> writes:

> On 06/10/2015 04:45 AM, Dr. David Alan Gilbert wrote:
>> * Jason J. Herne (address@hidden) wrote:
>>> On 06/09/2015 04:06 AM, Dr. David Alan Gilbert wrote:
>>>> * Jason J. Herne (address@hidden) 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 (address@hidden) wrote:
>>>>>>>> On 06/03/2015 03:56 AM, Juan Quintela wrote:
>>>>>>>>> "Jason J. Herne" <address@hidden> 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.

Ugh.

Differently ugh: make value's declared type a double, then reject
unwanted values depending on the parameter string.

>> I think the easiest 'solution' for this is to make the parameter an
>> integer percentage
>> rather than as a float.  Not that pretty but easier than fighting the
>> interface code.

If you can make do with int, you don't have to change the HMP command.

If I understand your problem correctly (I only skimmed, sorry), we're
talking about two parameters: an initial throttling factor (range (0,1],
percentage should do) and a "throttling multiplier", but I didn't quite
get what it's supposed to do, or what its acceptable range would be.

> Yes, I'm starting to feel this way :). Another option I'd like to
> collect opinions on it to change hmp's migrate_set_parameter to accept
> argument type O. As per monitor.c:
>
>  * 'O'          option string of the form NAME=VALUE,...
>  *              parsed according to QemuOptsList given by its name
>  *              Example: 'device:O' uses qemu_device_opts.
>  *              Restriction: only lists with empty desc are supported
>
> This would allow arbitrary data types and allow several parameters to
> be set at once right? It looks like it would be a relatively
> straightforward change to make. Opinions?

Incompatible change.  "migrate_set_parameter P V" becomes
"migrate_set_parameter P=V".  I guess we could accept that.

I'd prefer that to doing the parsing yourself, because it has a better
chance at keeping the parsing consistent.