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.

Signed-off-by: Zhenyu Ye <yezhenyu2@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

{
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;
+}
+
+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)
+{
+ 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
+##
+{ 'command': 'set-iothread-poll-param',
+ 'data': {'iothread_id': 'str', 'name': 'str', 'value': 'int'} }
+
##
# @BalloonInfo:
#
-- 
2.22.0.windows.1

* yezhenyu (A) (yezhenyu2@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.

> Signed-off-by: Zhenyu Ye <yezhenyu2@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!

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

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

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.


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

> +##
> +{ 'command': 'set-iothread-poll-param',
> + 'data': {'iothread_id': 'str', 'name': 'str', 'value': 'int'} }
> +
> ##
> # @BalloonInfo:
> #
> -- 
> 2.22.0.windows.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

>> {
>> 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 / dgilbert@redhat.com / Manchester, UK
> 
> 
> .
> 

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

On 10/22/19 3:12 AM, yezhenyu (A) 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.
> 
> Signed-off-by: Zhenyu Ye <yezhenyu2@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(+)

Looking at just the QMP...

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

This should be an enum.

> +#
> +# 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.

You failed to document the parameters (iothread_id, name, value).

> +#
> +# Example:
> +#
> +# -> { "execute": "set-iothread-poll-param",
> +# "arguments": { "iothread_id": "1",
> +# "name": "poll_max_ns",
> +# "value": 1000 } }
> +# <- { "return": {} }
> +#
> +# Since 3.0

4.2 is the earliest this can make it in.

> +##
> +{ 'command': 'set-iothread-poll-param',
> + 'data': {'iothread_id': 'str', 'name': 'str', 'value': 'int'} }

Our naming convention prefers 'iothread-id'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 2019/10/23 4:40, Eric Blake wrote:
> On 10/22/19 3:12 AM, yezhenyu (A) 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.
>>
>> Signed-off-by: Zhenyu Ye <yezhenyu2@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(+)
> 
> Looking at just the QMP...
> 

Thanks for your review. I will split this patch to QMP and HMP.

>> +++ 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.
> 
> This should be an enum.

I will change the name argument to ENUM, such as,

    { 'enum': 'IothreadPollProperty',
      'data': [ 'max-ns', 'grow', '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.
> 
> You failed to document the parameters (iothread_id, name, value).
> 

I will add these documents.

>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "set-iothread-poll-param",
>> +# "arguments": { "iothread_id": "1",
>> +# "name": "poll_max_ns",
>> +# "value": 1000 } }
>> +# <- { "return": {} }
>> +#
>> +# Since 3.0
> 
> 4.2 is the earliest this can make it in.
> 

OK, I will change this to 4.2.

>> +##
>> +{ 'command': 'set-iothread-poll-param',
>> + 'data': {'iothread_id': 'str', 'name': 'str', 'value': 'int'} }
> 
> Our naming convention prefers 'iothread-id'.
> 

ok, I will correct it, such as,

    { 'command': 'set-iothread-poll-param',
    'data': {'iothread-id': 'str', 'name': 'IothreadPollProperty', 'value': 'int'} }

On 10/22/19 9:28 PM, Zhenyu Ye wrote:

> I will change the name argument to ENUM, such as,
> 
>      { 'enum': 'IothreadPollProperty',
>        'data': [ 'max-ns', 'grow', 'shrink' ] }
> 

> 
> ok, I will correct it, such as,
> 
>      { 'command': 'set-iothread-poll-param',
>      'data': {'iothread-id': 'str', 'name': 'IothreadPollProperty', 'value': 'int'} }
> 

That's one approach.  But another approach would be:

{ 'command': 'set-iothread-poll-params',
   'data': { 'iothread-id': 'str',
             '*max-ns': 'int', '*grow': 'int', '*shrink': 'int' } }

The difference on the wire is between:

{ "execute": "set-iothread-poll-param",
   "arguments": { "iothread-id": "thr1",
     "name": "max-ns", "value": 1000 } }
{ "execute": "set-iothread-poll-param",
   "arguments": { "iothread-id": "thr1",
     "name": "grow", "value": 100 } }

vs.

{ "execute": "set-iothread-poll-params",
   "arguments': { "iothread-id": "thr1",
      "max-ns": 1000, "grow": 100 } }

I'll leave it up to Markus to give guidance on which style is nicer.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Oct 22, 2019 at 04:12:03PM +0800, yezhenyu (A) 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.
> 
> Signed-off-by: Zhenyu Ye <yezhenyu2@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(+)

poll-max-ns, poll-grow, poll-shrink are properties of IOThread objects.
They can already be modified at runtime using:

  $ qemu -object iothread,id=iothread1
  (qemu) qom-set /objects/iothread1 poll-max-ns 100000

I think there is no need for a patch.

Stefan

On 2019/10/23 23:19, Stefan Hajnoczi wrote:
> On Tue, Oct 22, 2019 at 04:12:03PM +0800, yezhenyu (A) 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.
>>
>> Signed-off-by: Zhenyu Ye <yezhenyu2@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(+)
> 
> poll-max-ns, poll-grow, poll-shrink are properties of IOThread objects.
> They can already be modified at runtime using:
> 
>   $ qemu -object iothread,id=iothread1
>   (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> 
> I think there is no need for a patch.
> 
> Stefan
> 

Thanks for your review. I have considered using the `qom-set` command to modify
IOThread object's properties, however, this command is not friendly to primary
users. The help info for this command is only:

    qom-set path property value -- set QOM property

It's almost impossible to get the correct `path` parameter for primary user.

This patch provides a more convenient and easy-use hmp&qmp interface to modify
these IOThread properties. I think this patch still has a little value.

And I can implement this patch compactly by reusing your code.

Waiting for your reply.

* Zhenyu Ye (yezhenyu2@huawei.com) wrote:
> 
> 
> On 2019/10/23 23:19, Stefan Hajnoczi wrote:
> > On Tue, Oct 22, 2019 at 04:12:03PM +0800, yezhenyu (A) 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.
> >>
> >> Signed-off-by: Zhenyu Ye <yezhenyu2@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(+)
> > 
> > poll-max-ns, poll-grow, poll-shrink are properties of IOThread objects.
> > They can already be modified at runtime using:
> > 
> >   $ qemu -object iothread,id=iothread1
> >   (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> > 
> > I think there is no need for a patch.
> > 
> > Stefan
> > 
> 
> Thanks for your review. I have considered using the `qom-set` command to modify
> IOThread object's properties, however, this command is not friendly to primary
> users. The help info for this command is only:
> 
>     qom-set path property value -- set QOM property
> 
> It's almost impossible to get the correct `path` parameter for primary user.

Is this just a matter of documenting how to do it?

It sounds like there's no need for a new QMP command though;  if you
want an easier HMP command I'd probably still take it (because HMP is ok
at having things for convenience) - but not if it turns out that just
adding a paragraph of documentation is enough.

Dave

> This patch provides a more convenient and easy-use hmp&qmp interface to modify
> these IOThread properties. I think this patch still has a little value.
> 
> And I can implement this patch compactly by reusing your code.
> 
> Waiting for your reply.
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 2019/10/24 21:56, Dr. David Alan Gilbert wrote:
> * Zhenyu Ye (yezhenyu2@huawei.com) wrote:
>>
>>
>> On 2019/10/23 23:19, Stefan Hajnoczi wrote:
>>> On Tue, Oct 22, 2019 at 04:12:03PM +0800, yezhenyu (A) 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.
>>>>
>>>> Signed-off-by: Zhenyu Ye <yezhenyu2@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(+)
>>>
>>> poll-max-ns, poll-grow, poll-shrink are properties of IOThread objects.
>>> They can already be modified at runtime using:
>>>
>>>   $ qemu -object iothread,id=iothread1
>>>   (qemu) qom-set /objects/iothread1 poll-max-ns 100000
>>>
>>> I think there is no need for a patch.
>>>
>>> Stefan
>>>
>>
>> Thanks for your review. I have considered using the `qom-set` command to modify
>> IOThread object's properties, however, this command is not friendly to primary
>> users. The help info for this command is only:
>>
>>     qom-set path property value -- set QOM property
>>
>> It's almost impossible to get the correct `path` parameter for primary user.
> 
> Is this just a matter of documenting how to do it?
> 
> It sounds like there's no need for a new QMP command though;  if you
> want an easier HMP command I'd probably still take it (because HMP is ok
> at having things for convenience) - but not if it turns out that just
> adding a paragraph of documentation is enough.
> 
> Dave
> 

I will show the differences in QMP and HMP:
If I want to set iothread1.poll-max-ns=1000 and iothread1.poll-grow=2:

Without this patch:
QMP command:

    qom-set /objects/iothread1 poll-max-ns 1000
    qom-set /objects/iothread1 poll-grow 2

HMP command:

    { "execute": "qom-set", "arguments": { "path": "/objects/iothread1",
                                           "property": "poll-max-ns", "value": 1000 } }
    { "execute": "qom-set", "arguments": { "path": "/objects/iothread1",
                                           "property": "poll-grow", "value": 2} }

with this patch:
QMP command:

    iothread_set_parameter iothread1 max-ns 1000
    iothread_set_parameter iothread1 grow 2

HMP command:

    { "execute": "set-iothread-poll-params", "arguments': { "iothread-id": "iothread1",
                                                            "max-ns": 1000, "grow": 2 } }


I think the inconvenience of qom-set is how to get the correct `path` parameter.
Anyway, I will consider your advice.


>> This patch provides a more convenient and easy-use hmp&qmp interface to modify
>> these IOThread properties. I think this patch still has a little value.
>>
>> And I can implement this patch compactly by reusing your code.
>>
>> Waiting for your reply.
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> .
> 

* Zhenyu Ye (yezhenyu2@huawei.com) wrote:
> 
> 
> On 2019/10/24 21:56, Dr. David Alan Gilbert wrote:
> > * Zhenyu Ye (yezhenyu2@huawei.com) wrote:
> >>
> >>
> >> On 2019/10/23 23:19, Stefan Hajnoczi wrote:
> >>> On Tue, Oct 22, 2019 at 04:12:03PM +0800, yezhenyu (A) 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.
> >>>>
> >>>> Signed-off-by: Zhenyu Ye <yezhenyu2@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(+)
> >>>
> >>> poll-max-ns, poll-grow, poll-shrink are properties of IOThread objects.
> >>> They can already be modified at runtime using:
> >>>
> >>>   $ qemu -object iothread,id=iothread1
> >>>   (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> >>>
> >>> I think there is no need for a patch.
> >>>
> >>> Stefan
> >>>
> >>
> >> Thanks for your review. I have considered using the `qom-set` command to modify
> >> IOThread object's properties, however, this command is not friendly to primary
> >> users. The help info for this command is only:
> >>
> >>     qom-set path property value -- set QOM property
> >>
> >> It's almost impossible to get the correct `path` parameter for primary user.
> > 
> > Is this just a matter of documenting how to do it?
> > 
> > It sounds like there's no need for a new QMP command though;  if you
> > want an easier HMP command I'd probably still take it (because HMP is ok
> > at having things for convenience) - but not if it turns out that just
> > adding a paragraph of documentation is enough.
> > 
> > Dave
> > 
> 
> I will show the differences in QMP and HMP:
> If I want to set iothread1.poll-max-ns=1000 and iothread1.poll-grow=2:
> 
> Without this patch:
> QMP command:
> 
>     qom-set /objects/iothread1 poll-max-ns 1000
>     qom-set /objects/iothread1 poll-grow 2
> 
> HMP command:
> 
>     { "execute": "qom-set", "arguments": { "path": "/objects/iothread1",
>                                            "property": "poll-max-ns", "value": 1000 } }
>     { "execute": "qom-set", "arguments": { "path": "/objects/iothread1",
>                                            "property": "poll-grow", "value": 2} }
> 
> with this patch:
> QMP command:
> 
>     iothread_set_parameter iothread1 max-ns 1000
>     iothread_set_parameter iothread1 grow 2
> 
> HMP command:
> 
>     { "execute": "set-iothread-poll-params", "arguments': { "iothread-id": "iothread1",
>                                                             "max-ns": 1000, "grow": 2 } }
> 
> 
> I think the inconvenience of qom-set is how to get the correct `path` parameter.
> Anyway, I will consider your advice.

So it depends how obvious the path is;  if it's just   /objects/
followed by whatever you used with id=  when you created the iothread
then I think it's easy - we just need to update the docs.
Is there a case where it's harder to know?

Dave

> 
> >> This patch provides a more convenient and easy-use hmp&qmp interface to modify
> >> these IOThread properties. I think this patch still has a little value.
> >>
> >> And I can implement this patch compactly by reusing your code.
> >>
> >> Waiting for your reply.
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> > 
> > .
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 2019/10/24 22:38, Dr. David Alan Gilbert wrote:
> * Zhenyu Ye (yezhenyu2@huawei.com) wrote:
>>
>>
>> On 2019/10/24 21:56, Dr. David Alan Gilbert wrote:
>>> * Zhenyu Ye (yezhenyu2@huawei.com) wrote:
>>>>
>>>>
>>>> On 2019/10/23 23:19, Stefan Hajnoczi wrote:
>>>>> On Tue, Oct 22, 2019 at 04:12:03PM +0800, yezhenyu (A) 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.
>>>>>>
>>>>>> Signed-off-by: Zhenyu Ye <yezhenyu2@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(+)
>>>>>
>>>>> poll-max-ns, poll-grow, poll-shrink are properties of IOThread objects.
>>>>> They can already be modified at runtime using:
>>>>>
>>>>>   $ qemu -object iothread,id=iothread1
>>>>>   (qemu) qom-set /objects/iothread1 poll-max-ns 100000
>>>>>
>>>>> I think there is no need for a patch.
>>>>>
>>>>> Stefan
>>>>>
>>>>
>>>> Thanks for your review. I have considered using the `qom-set` command to modify
>>>> IOThread object's properties, however, this command is not friendly to primary
>>>> users. The help info for this command is only:
>>>>
>>>>     qom-set path property value -- set QOM property
>>>>
>>>> It's almost impossible to get the correct `path` parameter for primary user.
>>>
>>> Is this just a matter of documenting how to do it?
>>>
>>> It sounds like there's no need for a new QMP command though;  if you
>>> want an easier HMP command I'd probably still take it (because HMP is ok
>>> at having things for convenience) - but not if it turns out that just
>>> adding a paragraph of documentation is enough.
>>>
>>> Dave
>>>
>>
>> I will show the differences in QMP and HMP:
>> If I want to set iothread1.poll-max-ns=1000 and iothread1.poll-grow=2:
>>
>> Without this patch:
>> QMP command:
>>
>>     qom-set /objects/iothread1 poll-max-ns 1000
>>     qom-set /objects/iothread1 poll-grow 2
>>
>> HMP command:
>>
>>     { "execute": "qom-set", "arguments": { "path": "/objects/iothread1",
>>                                            "property": "poll-max-ns", "value": 1000 } }
>>     { "execute": "qom-set", "arguments": { "path": "/objects/iothread1",
>>                                            "property": "poll-grow", "value": 2} }
>>
>> with this patch:
>> QMP command:
>>
>>     iothread_set_parameter iothread1 max-ns 1000
>>     iothread_set_parameter iothread1 grow 2
>>
>> HMP command:
>>
>>     { "execute": "set-iothread-poll-params", "arguments': { "iothread-id": "iothread1",
>>                                                             "max-ns": 1000, "grow": 2 } }
>>
>>
>> I think the inconvenience of qom-set is how to get the correct `path` parameter.
>> Anyway, I will consider your advice.
> 
> So it depends how obvious the path is;  if it's just   /objects/
> followed by whatever you used with id=  when you created the iothread
> then I think it's easy - we just need to update the docs.
> Is there a case where it's harder to know?
> 
> Dave
> 

You are right, it's just /objects/ followed by the id. Maybe we just need
to update the docs for qom-set.

>>
>>>> This patch provides a more convenient and easy-use hmp&qmp interface to modify
>>>> these IOThread properties. I think this patch still has a little value.
>>>>
>>>> And I can implement this patch compactly by reusing your code.
>>>>
>>>> Waiting for your reply.
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
>>>
>>> .
>>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> .
> 

On Fri, Oct 25, 2019 at 10:07:56AM +0800, Zhenyu Ye wrote:
> 
> 
> On 2019/10/24 22:38, Dr. David Alan Gilbert wrote:
> > * Zhenyu Ye (yezhenyu2@huawei.com) wrote:
> >>
> >>
> >> On 2019/10/24 21:56, Dr. David Alan Gilbert wrote:
> >>> * Zhenyu Ye (yezhenyu2@huawei.com) wrote:
> >>>>
> >>>>
> >>>> On 2019/10/23 23:19, Stefan Hajnoczi wrote:
> >>>>> On Tue, Oct 22, 2019 at 04:12:03PM +0800, yezhenyu (A) 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.
> >>>>>>
> >>>>>> Signed-off-by: Zhenyu Ye <yezhenyu2@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(+)
> >>>>>
> >>>>> poll-max-ns, poll-grow, poll-shrink are properties of IOThread objects.
> >>>>> They can already be modified at runtime using:
> >>>>>
> >>>>>   $ qemu -object iothread,id=iothread1
> >>>>>   (qemu) qom-set /objects/iothread1 poll-max-ns 100000
> >>>>>
> >>>>> I think there is no need for a patch.
> >>>>>
> >>>>> Stefan
> >>>>>
> >>>>
> >>>> Thanks for your review. I have considered using the `qom-set` command to modify
> >>>> IOThread object's properties, however, this command is not friendly to primary
> >>>> users. The help info for this command is only:
> >>>>
> >>>>     qom-set path property value -- set QOM property
> >>>>
> >>>> It's almost impossible to get the correct `path` parameter for primary user.
> >>>
> >>> Is this just a matter of documenting how to do it?
> >>>
> >>> It sounds like there's no need for a new QMP command though;  if you
> >>> want an easier HMP command I'd probably still take it (because HMP is ok
> >>> at having things for convenience) - but not if it turns out that just
> >>> adding a paragraph of documentation is enough.
> >>>
> >>> Dave
> >>>
> >>
> >> I will show the differences in QMP and HMP:
> >> If I want to set iothread1.poll-max-ns=1000 and iothread1.poll-grow=2:
> >>
> >> Without this patch:
> >> QMP command:
> >>
> >>     qom-set /objects/iothread1 poll-max-ns 1000
> >>     qom-set /objects/iothread1 poll-grow 2
> >>
> >> HMP command:
> >>
> >>     { "execute": "qom-set", "arguments": { "path": "/objects/iothread1",
> >>                                            "property": "poll-max-ns", "value": 1000 } }
> >>     { "execute": "qom-set", "arguments": { "path": "/objects/iothread1",
> >>                                            "property": "poll-grow", "value": 2} }
> >>
> >> with this patch:
> >> QMP command:
> >>
> >>     iothread_set_parameter iothread1 max-ns 1000
> >>     iothread_set_parameter iothread1 grow 2
> >>
> >> HMP command:
> >>
> >>     { "execute": "set-iothread-poll-params", "arguments': { "iothread-id": "iothread1",
> >>                                                             "max-ns": 1000, "grow": 2 } }
> >>
> >>
> >> I think the inconvenience of qom-set is how to get the correct `path` parameter.
> >> Anyway, I will consider your advice.
> > 
> > So it depends how obvious the path is;  if it's just   /objects/
> > followed by whatever you used with id=  when you created the iothread
> > then I think it's easy - we just need to update the docs.
> > Is there a case where it's harder to know?
> > 
> > Dave
> > 
> 
> You are right, it's just /objects/ followed by the id. Maybe we just need
> to update the docs for qom-set.

The documentation for qom-set will become very large and unwieldy if the
properties of all objects are documented there.

I will send a patch documenting -object iothread and CC you.

Stefan