Re: [RFC PATCH] iothread: add set_iothread_poll_* commands

[Dr. David Alan Gilbert]

* Zhenyu Ye (address@hidden) wrote:
> 
> 
> On 2019/10/22 16:51, Dr. David Alan Gilbert wrote:
> > * yezhenyu (A) (address@hidden) 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 <address@hidden>
> >> ---
> >> 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 / address@hidden / Manchester, UK
> > 
> > 
> > .
> > 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK