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