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

[Zhenyu Ye]

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

>> {
>> 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
> 
> 
> .
>