* Zhenyu Ye (yezhen...@huawei.com) wrote: > > > On 2019/10/22 16:51, Dr. David Alan Gilbert wrote: > > * yezhenyu (A) (yezhen...@huawei.com) wrote: > >> Since qemu2.9, QEMU added three AioContext poll parameters to struct > >> IOThread: poll_max_ns, poll_grow and poll_shrink. These properties are > >> used to control iothread polling time. > >> > >> However, there isn't properly hmp commands to adjust them when the VM is > >> alive. It's useful to adjust them online when observing the impact of > >> different property value on performance. > >> > >> This patch add three hmp commands to adjust iothread poll-* properties > >> for special iothread: > >> > >> set_iothread_poll_max_ns: set the maximum polling time in ns; > >> set_iothread_poll_grow: set how many ns will be added to polling time; > >> set_iothread_poll_shrink: set how many ns will be removed from polling > >> time. > > > > Thanks; I don't know much about iothread, so I'll answer just from the > > HMP side. > > > > Thanks for your review. I will update my patch according to your advice, and > submit a NEW PATCH. Some of my answers are below... > > >> Signed-off-by: Zhenyu Ye <yezhen...@huawei.com> > >> --- > >> hmp-commands.hx | 42 ++++++++++++++++++++ > >> hmp.c | 30 +++++++++++++++ > >> hmp.h | 3 ++ > >> include/sysemu/iothread.h | 6 +++ > >> iothread.c | 80 +++++++++++++++++++++++++++++++++++++++ > >> qapi/misc.json | 23 +++++++++++ > >> 6 files changed, 184 insertions(+) > >> > >> diff --git a/hmp-commands.hx b/hmp-commands.hx > >> index a2c3ffc218..6fa0c5227a 100644 > >> --- a/hmp-commands.hx > >> +++ b/hmp-commands.hx > >> @@ -74,6 +74,48 @@ VM initialization using configuration data provided on > >> the command line > >> and via the QMP monitor during the preconfig state. The command is only > >> available during the preconfig state (i.e. when the --preconfig command > >> line option was in use). > >> +ETEXI > >> + > >> + { > >> + .name = "set_iothread_poll_max_ns", > >> + .args_type = "iothread_id:s,poll_max_ns:i", > >> + .params = "iothread_id poll_max_ns", > >> + .help = "set poll_max_ns for a special iothread", > >> + .cmd = hmp_set_iothread_poll_max_ns, > >> + }, > >> + > >> +STEXI > >> +@item set_iothread_poll_max_ns > >> +@findex set_iothread_poll_max_ns > >> +set poll_max_ns property for a special iothread. > >> +ETEXI > >> + > >> + { > >> + .name = "set_iothread_poll_grow", > >> + .args_type = "iothread_id:s,poll_grow:i", > >> + .params = "iothread_id poll_grow", > >> + .help = "set poll_grow for a special iothread", > >> + .cmd = hmp_set_iothread_poll_grow, > >> + }, > >> + > >> +STEXI > >> +@item set_iothread_poll_grow > >> +@findex set_iothread_poll_grow > >> +set poll_grow property for a special iothread. > >> +ETEXI > >> + > >> + { > >> + .name = "set_iothread_poll_shrink", > >> + .args_type = "iothread_id:s,poll_shrink:i", > >> + .params = "iothread_id poll_shrink", > >> + .help = "set poll_shrink for a special iothread", > >> + .cmd = hmp_set_iothread_poll_shrink, > >> + }, > >> + > >> +STEXI > >> +@item set_iothread_poll_shrink > >> +@findex set_iothread_poll_shrink > >> +set poll_shrink property for a special iothread. > >> ETEXI > > > > I think I'd prefer one command with a parameter to select > > the value to be set; in migration we used to have lots of commands > > but are moving to migrate_set_parameter which then handles all of them. > > > > So something like, > > > > iothread set_parameter poll_shrink 10 > > > > The code is a little more complex, but a lot less repetative! > > > > OK, I will combine these in one command such as, > > set_iothread_parameter iothread_id parameter_name value
OK, one thing, please use: iothread_set_parameter instead of set_iothread_parameter. Dave > >> { > >> diff --git a/hmp.c b/hmp.c > >> index 56a3ed7375..87520bcc85 100644 > >> --- a/hmp.c > >> +++ b/hmp.c > >> @@ -2711,6 +2711,36 @@ void hmp_info_iothreads(Monitor *mon, const QDict > >> *qdict) > >> qapi_free_IOThreadInfoList(info_list); > >> } > >> > >> +void hmp_set_iothread_poll_max_ns(Monitor *mon, const QDict *qdict) > >> +{ > >> + const char *iothread_id = qdict_get_str(qdict, "iothread_id"); > >> + int64_t poll_max_ns = qdict_get_int(qdict, "poll_max_ns"); > >> + Error *err = NULL; > >> + > >> + qmp_set_iothread_poll_param(iothread_id, "poll_max_ns", poll_max_ns, > >> &err); > >> + hmp_handle_error(mon, &err); > >> +} > >> + > >> +void hmp_set_iothread_poll_grow(Monitor *mon, const QDict *qdict) > >> +{ > >> + const char *iothread_id = qdict_get_str(qdict, "iothread_id"); > >> + int64_t poll_grow = qdict_get_int(qdict, "poll_grow"); > >> + Error *err = NULL; > >> + > >> + qmp_set_iothread_poll_param(iothread_id, "poll_grow", poll_grow, &err); > >> + hmp_handle_error(mon, &err); > >> +} > >> + > >> +void hmp_set_iothread_poll_shrink(Monitor *mon, const QDict *qdict) > >> +{ > >> + const char *iothread_id = qdict_get_str(qdict, "iothread_id"); > >> + int64_t poll_shrink = qdict_get_int(qdict, "poll_shrink"); > >> + Error *err = NULL; > >> + > >> + qmp_set_iothread_poll_param(iothread_id, "poll_shrink", poll_shrink, > >> &err); > >> + hmp_handle_error(mon, &err); > >> +} > >> + > >> void hmp_qom_list(Monitor *mon, const QDict *qdict) > >> { > >> const char *path = qdict_get_try_str(qdict, "path"); > >> diff --git a/hmp.h b/hmp.h > >> index 43617f2646..8ce3b53556 100644 > >> --- a/hmp.h > >> +++ b/hmp.h > >> @@ -41,6 +41,9 @@ void hmp_info_pci(Monitor *mon, const QDict *qdict); > >> void hmp_info_block_jobs(Monitor *mon, const QDict *qdict); > >> void hmp_info_tpm(Monitor *mon, const QDict *qdict); > >> void hmp_info_iothreads(Monitor *mon, const QDict *qdict); > >> +void hmp_set_iothread_poll_max_ns(Monitor *mon, const QDict *qdict); > >> +void hmp_set_iothread_poll_grow(Monitor *mon, const QDict *qdict); > >> +void hmp_set_iothread_poll_shrink(Monitor *mon, const QDict *qdict); > >> void hmp_quit(Monitor *mon, const QDict *qdict); > >> void hmp_stop(Monitor *mon, const QDict *qdict); > >> void hmp_sync_profile(Monitor *mon, const QDict *qdict); > >> diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h > >> index 5f6240d5cb..7ebeb51f37 100644 > >> --- a/include/sysemu/iothread.h > >> +++ b/include/sysemu/iothread.h > >> @@ -45,6 +45,12 @@ char *iothread_get_id(IOThread *iothread); > >> IOThread *iothread_by_id(const char *id); > >> AioContext *iothread_get_aio_context(IOThread *iothread); > >> GMainContext *iothread_get_g_main_context(IOThread *iothread); > >> +void iothread_set_poll_max_ns(IOThread *iothread, int64_t poll_max_ns); > >> +int64_t iothread_get_poll_max_ns(IOThread *iothread); > >> +void iothread_set_poll_grow(IOThread *iothread, int64_t poll_grow); > >> +int64_t iothread_get_poll_grow(IOThread *iothread); > >> +void iothread_set_poll_shrink(IOThread *iothread, int64_t poll_shrink); > >> +int64_t iothread_get_poll_shrink(IOThread *iothread); > >> > >> /* > >> * Helpers used to allocate iothreads for internal use. These > >> diff --git a/iothread.c b/iothread.c > >> index 7130be58e3..4ab6128c5f 100644 > >> --- a/iothread.c > >> +++ b/iothread.c > >> @@ -384,3 +384,83 @@ IOThread *iothread_by_id(const char *id) > >> { > >> return IOTHREAD(object_resolve_path_type(id, TYPE_IOTHREAD, NULL)); > >> } > >> + > >> +void iothread_set_poll_max_ns(IOThread *iothread, int64_t poll_max_ns) > >> +{ > >> + iothread->poll_max_ns = poll_max_ns; > >> +} > > > > Is the indentation correct here? It looks a bit off. > > > > I will correct this. > > >> +int64_t iothread_get_poll_max_ns(IOThread *iothread) > >> +{ > >> + return iothread->poll_max_ns; > >> +} > >> + > >> +void iothread_set_poll_grow(IOThread *iothread, int64_t poll_grow) > >> +{ > >> + iothread->poll_grow = poll_grow; > >> +} > >> + > >> +int64_t iothread_get_poll_grow(IOThread *iothread) > >> +{ > >> + return iothread->poll_grow; > >> +} > >> + > >> +void iothread_set_poll_shrink(IOThread *iothread, int64_t poll_shrink) > >> +{ > >> + iothread->poll_shrink = poll_shrink; > >> +} > >> + > >> +int64_t iothread_get_poll_shrink(IOThread *iothread) > >> +{ > >> + return iothread->poll_shrink; > >> +} > >> + > >> +void qmp_set_iothread_poll_param(const char *iothread_id, const char > >> *name, > >> + int64_t value, Error **errp) > > > > OK, so you should split the patch into a qmp patch and and then an HMP > > patch. > > I'll leave the QMP review to others, but I'm guessing they'd prefer > > to take a QAPI enum here rather than the name. > > > > I will split the patch according your advice. > > > Are there some docs somewhere that describe what grow and shrink > > actually are - are they % ? If so, then you should range check > > them to make sure someone doesn't set them to something silly like -5. > > > > > > They should be positive number. I will add type check in function. > > >> +{ > >> + Error *local_error = NULL; > >> + int64_t old_value = 0; > >> + IOThread *iothread = iothread_by_id(iothread_id); > >> + if (!iothread) { > >> + error_setg(errp, "can not find iothread: %s", iothread_id); > >> + return; > >> + } > >> + > >> + if (strcmp(name, "poll_max_ns") == 0) { > >> + old_value = iothread_get_poll_max_ns(iothread); > >> + iothread_set_poll_max_ns(iothread, value); > >> + } else if (strcmp(name, "poll_grow") == 0) { > >> + old_value = iothread_get_poll_grow(iothread); > >> + iothread_set_poll_grow(iothread, value); > >> + } else if (strcmp(name, "poll_shrink") == 0) { > >> + old_value = iothread_get_poll_shrink(iothread); > >> + iothread_set_poll_shrink(iothread, value); > >> + } else { > >> + error_setg(errp, "can not find param name: %s", name); > >> + return; > >> + } > >> + > >> + /* update the value in context */ > >> + aio_context_set_poll_params(iothread->ctx, > >> + iothread->poll_max_ns, > >> + iothread->poll_grow, > >> + iothread->poll_shrink, > >> + &local_error); > >> + > >> + if (local_error) { > >> + error_propagate(errp, local_error); > >> + > >> + /* reset the property to old value */ > >> + if (strcmp(name, "poll_max_ns") == 0) { > >> + iothread_set_poll_max_ns(iothread, old_value); > >> + } else if (strcmp(name, "poll_grow") == 0) { > >> + iothread_set_poll_grow(iothread, old_value); > >> + } else if (strcmp(name, "poll_shrink") == 0) { > >> + iothread_set_poll_shrink(iothread, old_value); > >> + } > >> + > >> + return; > >> + } > >> + > >> + return; > >> +} > >> diff --git a/qapi/misc.json b/qapi/misc.json > >> index 8b3ca4fdd3..43d3f4351c 100644 > >> --- a/qapi/misc.json > >> +++ b/qapi/misc.json > >> @@ -675,6 +675,29 @@ > >> { 'command': 'query-iothreads', 'returns': ['IOThreadInfo'], > >> 'allow-preconfig': true } > >> > >> +## > >> +# @set-iothread-poll-param: > >> +# > >> +# Set poll-* properties for a special iothread. > >> +# The poll-* name can be poll_max_ns/poll_grow/poll_shrink. > >> +# > >> +# Notes: can not set the QEMU main loop thread, which is not declared > >> +# using the -object iothread command-line option. The poll_ns property > >> can not > >> +# be set manually, which is auto-adjust. > >> +# > >> +# Example: > >> +# > >> +# -> { "execute": "set-iothread-poll-param", > >> +# "arguments": { "iothread_id": "1", > >> +# "name": "poll_max_ns", > >> +# "value": 1000 } } > >> +# <- { "return": {} } > >> +# > >> +# Since 3.0 > > > > 3.0 was long long ago; needs updating. > > > > I will update this to 4.1.0 > > >> +## > >> +{ 'command': 'set-iothread-poll-param', > >> + 'data': {'iothread_id': 'str', 'name': 'str', 'value': 'int'} } > >> + > >> ## > >> # @BalloonInfo: > >> # > >> -- > >> 2.22.0.windows.1 > >> > > -- > > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > > > > > > . > > > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK