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

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

On 06/10/2015 04:45 AM, Dr. David Alan Gilbert wrote:
> * Jason J. Herne (jjherne@linux.vnet.ibm.com) wrote:
>> 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.
>
> 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.
>

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?

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