From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
Implement dirtyrate calculation periodically basing on
dirty-ring and throttle virtual CPU until it reachs the quota
dirty page rate given by user.
Introduce qmp commands "vcpu-dirty-limit", "query-vcpu-dirty-limit"
to enable, disable, query dirty page limit for virtual CPU.
Meanwhile, introduce corresponding hmp commands "vcpu_dirty_limit",
"info vcpu_dirty_limit" so developers can play with them easier.
Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
---
cpus-common.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++
hmp-commands-info.hx | 13 +++++
hmp-commands.hx | 17 ++++++
include/monitor/hmp.h | 2 +
qapi/migration.json | 44 ++++++++++++++
5 files changed, 231 insertions(+)
diff --git a/cpus-common.c b/cpus-common.c
index 6e73d3e..37c3584 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -23,6 +23,15 @@
#include "hw/core/cpu.h"
#include "sysemu/cpus.h"
#include "qemu/lockable.h"
+#include "sysemu/dirtylimit.h"
+#include "sysemu/cpu-throttle.h"
+#include "sysemu/kvm.h"
+#include "monitor/hmp.h"
+#include "monitor/monitor.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-migration.h"
+#include "hw/boards.h"
static QemuMutex qemu_cpu_list_lock;
static QemuCond exclusive_cond;
@@ -352,3 +361,149 @@ void process_queued_cpu_work(CPUState *cpu)
qemu_mutex_unlock(&cpu->work_mutex);
qemu_cond_broadcast(&qemu_work_cond);
}
+
+void qmp_vcpu_dirty_limit(bool enable,
+ bool has_cpu_index,
+ uint64_t cpu_index,
+ bool has_dirty_rate,
+ uint64_t dirty_rate,
+ Error **errp)
+{
+ static bool initialized;
+
+ if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
+ if (enable) {
+ error_setg(errp, "dirty page limit feature requires KVM with"
+ " accelerator property 'dirty-ring-size' set'");
+ }
+ return;
+ }
+
+ if (!enable && !dirtylimit_in_service()) {
+ return;
+ }
+
+ if (!initialized) {
+ MachineState *ms = MACHINE(qdev_get_machine());
+ dirtylimit_calc_state_init(ms->smp.max_cpus);
+ dirtylimit_state_init(ms->smp.max_cpus);
+ initialized = true;
+ }
+
+ if (enable && !has_dirty_rate) {
+ error_setg(errp, "enable dirty page limit feature requires"
+ " 'dirty-rate' set'");
+ return;
+ }
+
+ if (has_cpu_index && !dirtylimit_is_vcpu_index_valid(cpu_index)) {
+ error_setg(errp, "incorrect cpu index specified");
+ return;
+ }
+
+ if (enable) {
+ dirtylimit_calc_start();
+ if (has_cpu_index) {
+ dirtylimit_vcpu(cpu_index, dirty_rate);
+ } else {
+ dirtylimit_all(dirty_rate);
+ }
+ } else {
+ if (has_cpu_index) {
+ dirtylimit_cancel_vcpu(cpu_index);
+ } else {
+ dirtylimit_cancel_all();
+ }
+
+ if (!dirtylimit_in_service()) {
+ dirtylimit_calc_quit();
+ dirtylimit_state_finalize();
+ dirtylimit_calc_state_finalize();
+ initialized = false;
+ }
+ }
+}
+
+void hmp_vcpu_dirty_limit(Monitor *mon, const QDict *qdict)
+{
+ bool global = qdict_get_try_bool(qdict, "global", false);
+ bool enable = qdict_get_bool(qdict, "enable");
+ int64_t cpu_index = qdict_get_try_int(qdict, "cpu_index", -1);
+ int64_t dirty_rate = qdict_get_try_int(qdict, "dirty_rate", -1);
+ Error *err = NULL;
+
+ if (enable && dirty_rate < 0) {
+ monitor_printf(mon, "Dirty page limit requires dirty_rate set!\n");
+ return;
+ }
+
+ if (enable && !global && cpu_index < 0) {
+ monitor_printf(mon, "Dirty page limit requires cpu_index set!\n");
+ return;
+ }
+
+ if (global && cpu_index != -1) {
+ monitor_printf(mon, "Either global option or cpu_index can be set!\n");
+ return;
+ }
+
+ if (global) {
+ if (enable) {
+ qmp_vcpu_dirty_limit(true, false, -1, true, dirty_rate, &err);
+ } else {
+ qmp_vcpu_dirty_limit(false, false, -1, false, -1, &err);
+ }
+ } else {
+ if (enable) {
+ qmp_vcpu_dirty_limit(true, true, cpu_index, true, dirty_rate, &err);
+ } else {
+ qmp_vcpu_dirty_limit(false, true, cpu_index, false, -1, &err);
+ }
+ }
+
+ if (err) {
+ hmp_handle_error(mon, err);
+ return;
+ }
+
+ monitor_printf(mon, "[Please use 'info vcpu_dirty_limit' to query "
+ "dirty limit for virtual CPU]\n");
+}
+
+struct DirtyLimitInfoList *qmp_query_vcpu_dirty_limit(Error **errp)
+{
+ if (!dirtylimit_in_service()) {
+ error_setg(errp, "dirty page limit not enabled");
+ return NULL;
+ }
+
+ return dirtylimit_query_all();
+}
+
+void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict)
+{
+ DirtyLimitInfoList *limit, *head, *info = NULL;
+ Error *err = NULL;
+
+ if (!dirtylimit_in_service()) {
+ monitor_printf(mon, "Dirty page limit not enabled!\n");
+ return;
+ }
+
+ info = qmp_query_vcpu_dirty_limit(&err);
+ if (err) {
+ hmp_handle_error(mon, err);
+ return;
+ }
+
+ head = info;
+ for (limit = head; limit != NULL; limit = limit->next) {
+ monitor_printf(mon, "vcpu[%"PRIi64"], limit rate %"PRIi64 " (MB/s),"
+ " current rate %"PRIi64 " (MB/s)\n",
+ limit->value->cpu_index,
+ limit->value->limit_rate,
+ limit->value->current_rate);
+ }
+
+ g_free(info);
+}
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 407a1da..5dd3001 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -863,6 +863,19 @@ SRST
Display the vcpu dirty rate information.
ERST
+ {
+ .name = "vcpu_dirty_limit",
+ .args_type = "",
+ .params = "",
+ .help = "show dirty page limit information of all vCPU",
+ .cmd = hmp_info_vcpu_dirty_limit,
+ },
+
+SRST
+ ``info vcpu_dirty_limit``
+ Display the vcpu dirty page limit information.
+ERST
+
#if defined(TARGET_I386)
{
.name = "sgx",
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 70a9136..ef0f7cc 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1744,3 +1744,20 @@ ERST
"\n\t\t\t -b to specify dirty bitmap as method of calculation)",
.cmd = hmp_calc_dirty_rate,
},
+
+SRST
+``vcpu_dirty_limit``
+ Start dirty page rate limit on a virtual CPU, the information about all the
+ virtual CPU dirty limit status can be observed with ``info vcpu_dirty_limit``
+ command.
+ERST
+
+ {
+ .name = "vcpu_dirty_limit",
+ .args_type = "global:-g,enable:b,cpu_index:l?,dirty_rate:l?",
+ .params = "[-g] on|off [cpu_index] [dirty_rate]",
+ .help = "turn on,off dirty page rate limit"
+ "\n\t\t (use -g to affect all vCPU, cpu_index required be set to -1 if"
+ "\n\t\t enable all vCPU. dirty_rate should be specified if turned on)",
+ .cmd = hmp_vcpu_dirty_limit,
+ },
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 96d0148..04879a2 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -131,6 +131,8 @@ void hmp_replay_delete_break(Monitor *mon, const QDict *qdict);
void hmp_replay_seek(Monitor *mon, const QDict *qdict);
void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict);
void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
+void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
+void hmp_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
void hmp_human_readable_text_helper(Monitor *mon,
HumanReadableText *(*qmp_handler)(Error **));
diff --git a/qapi/migration.json b/qapi/migration.json
index ac5fa56..7d8da4f 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1869,6 +1869,50 @@
'current-rate': 'int64' } }
##
+# @vcpu-dirty-limit:
+#
+# Set or cancel the upper limit of dirty page rate for a virtual CPU.
+#
+# Requires KVM with accelerator property "dirty-ring-size" set.
+# A virtual CPU's dirty page rate is a measure of its memory load.
+# To observe dirty page rates, use @calc-dirty-rate.
+#
+# @enable: true to enable, false to disable.
+#
+# @cpu-index: index of virtual CPU, default is all.
+#
+# @dirty-rate: upper limit of dirty page rate for virtual CPU, to
+# cancel dirty limit, this field will be ignored.
+#
+# Since: 7.0
+#
+# Example:
+# {"execute": "vcpu-dirty-limit"}
+# "arguments": { "enable": true,
+# "cpu-index": 1,
+# "dirty-rate": 200 } }
+#
+##
+{ 'command': 'vcpu-dirty-limit',
+ 'data': { 'enable': 'bool',
+ '*cpu-index': 'uint64',
+ '*dirty-rate': 'uint64'} }
+
+##
+# @query-vcpu-dirty-limit:
+#
+# Returns information about all virtual CPU dirty limit if enabled.
+#
+# Since: 7.0
+#
+# Example:
+# {"execute": "query-vcpu-dirty-limit"}
+#
+##
+{ 'command': 'query-vcpu-dirty-limit',
+ 'returns': [ 'DirtyLimitInfo' ] }
+
+##
# @snapshot-save:
#
# Save a VM snapshot
--
1.8.3.1
huangy81@chinatelecom.cn writes:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>
> Implement dirtyrate calculation periodically basing on
> dirty-ring and throttle virtual CPU until it reachs the quota
> dirty page rate given by user.
>
> Introduce qmp commands "vcpu-dirty-limit", "query-vcpu-dirty-limit"
> to enable, disable, query dirty page limit for virtual CPU.
>
> Meanwhile, introduce corresponding hmp commands "vcpu_dirty_limit",
> "info vcpu_dirty_limit" so developers can play with them easier.
>
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
[...]
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index 96d0148..04879a2 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -131,6 +131,8 @@ void hmp_replay_delete_break(Monitor *mon, const QDict *qdict);
> void hmp_replay_seek(Monitor *mon, const QDict *qdict);
> void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict);
> void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
> +void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
> +void hmp_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
> void hmp_human_readable_text_helper(Monitor *mon,
> HumanReadableText *(*qmp_handler)(Error **));
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index ac5fa56..7d8da4f 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1869,6 +1869,50 @@
> 'current-rate': 'int64' } }
>
> ##
> +# @vcpu-dirty-limit:
> +#
> +# Set or cancel the upper limit of dirty page rate for a virtual CPU.
> +#
> +# Requires KVM with accelerator property "dirty-ring-size" set.
> +# A virtual CPU's dirty page rate is a measure of its memory load.
> +# To observe dirty page rates, use @calc-dirty-rate.
> +#
> +# @enable: true to enable, false to disable.
> +#
> +# @cpu-index: index of virtual CPU, default is all.
Suggest
# @cpu-index: The vCPU to act upon (all by default).
(I'm stealing this from trace.json)
> +#
> +# @dirty-rate: upper limit of dirty page rate for virtual CPU, to
> +# cancel dirty limit, this field will be ignored.
> +#
> +# Since: 7.0
> +#
> +# Example:
> +# {"execute": "vcpu-dirty-limit"}
> +# "arguments": { "enable": true,
> +# "cpu-index": 1,
> +# "dirty-rate": 200 } }
> +#
> +##
> +{ 'command': 'vcpu-dirty-limit',
> + 'data': { 'enable': 'bool',
> + '*cpu-index': 'uint64',
> + '*dirty-rate': 'uint64'} }
Drop @enable, please.
If @dirty-rate is present, set the limit to its value.
If it's absent, cancel the limit.
> +
> +##
> +# @query-vcpu-dirty-limit:
> +#
> +# Returns information about all virtual CPU dirty limit if enabled.
> +#
> +# Since: 7.0
> +#
> +# Example:
> +# {"execute": "query-vcpu-dirty-limit"}
> +#
> +##
> +{ 'command': 'query-vcpu-dirty-limit',
> + 'returns': [ 'DirtyLimitInfo' ] }
> +
> +##
> # @snapshot-save:
> #
> # Save a VM snapshot
在 2021/12/15 15:37, Markus Armbruster 写道:
> huangy81@chinatelecom.cn writes:
>
>> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>>
>> Implement dirtyrate calculation periodically basing on
>> dirty-ring and throttle virtual CPU until it reachs the quota
>> dirty page rate given by user.
>>
>> Introduce qmp commands "vcpu-dirty-limit", "query-vcpu-dirty-limit"
>> to enable, disable, query dirty page limit for virtual CPU.
>>
>> Meanwhile, introduce corresponding hmp commands "vcpu_dirty_limit",
>> "info vcpu_dirty_limit" so developers can play with them easier.
>>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>
> [...]
>
>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>> index 96d0148..04879a2 100644
>> --- a/include/monitor/hmp.h
>> +++ b/include/monitor/hmp.h
>> @@ -131,6 +131,8 @@ void hmp_replay_delete_break(Monitor *mon, const QDict *qdict);
>> void hmp_replay_seek(Monitor *mon, const QDict *qdict);
>> void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict);
>> void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
>> +void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>> +void hmp_vcpu_dirty_limit(Monitor *mon, const QDict *qdict);
>> void hmp_human_readable_text_helper(Monitor *mon,
>> HumanReadableText *(*qmp_handler)(Error **));
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index ac5fa56..7d8da4f 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1869,6 +1869,50 @@
>> 'current-rate': 'int64' } }
>>
>> ##
>> +# @vcpu-dirty-limit:
>> +#
>> +# Set or cancel the upper limit of dirty page rate for a virtual CPU.
>> +#
>> +# Requires KVM with accelerator property "dirty-ring-size" set.
>> +# A virtual CPU's dirty page rate is a measure of its memory load.
>> +# To observe dirty page rates, use @calc-dirty-rate.
>> +#
>> +# @enable: true to enable, false to disable.
>> +#
>> +# @cpu-index: index of virtual CPU, default is all.
>
> Suggest
>
> # @cpu-index: The vCPU to act upon (all by default).
>
> (I'm stealing this from trace.json)
>
>> +#
>> +# @dirty-rate: upper limit of dirty page rate for virtual CPU, to
>> +# cancel dirty limit, this field will be ignored.
>> +#
>> +# Since: 7.0
>> +#
>> +# Example:
>> +# {"execute": "vcpu-dirty-limit"}
>> +# "arguments": { "enable": true,
>> +# "cpu-index": 1,
>> +# "dirty-rate": 200 } }
>> +#
>> +##
>> +{ 'command': 'vcpu-dirty-limit',
>> + 'data': { 'enable': 'bool',
>> + '*cpu-index': 'uint64',
>> + '*dirty-rate': 'uint64'} }
>
> Drop @enable, please.
>
> If @dirty-rate is present, set the limit to its value.
>
> If it's absent, cancel the limit.
>
Ok. Indeed, this is the simplest style. :)
So the final qmp format should be like:
case 1: setup vcpu 0 dirty page limit 100MB/s
vcpu-dirty-limit cpu-index=0 dirty-rate=100MB/s
case 2: cancle vcpu 0 dirty page limit
vcpu-dirty-limit cpu-index=0
I'll do that next version.
Thanks Markus very much
>> +
>> +##
>> +# @query-vcpu-dirty-limit:
>> +#
>> +# Returns information about all virtual CPU dirty limit if enabled.
>> +#
>> +# Since: 7.0
>> +#
>> +# Example:
>> +# {"execute": "query-vcpu-dirty-limit"}
>> +#
>> +##
>> +{ 'command': 'query-vcpu-dirty-limit',
>> + 'returns': [ 'DirtyLimitInfo' ] }
>> +
>> +##
>> # @snapshot-save:
>> #
>> # Save a VM snapshot
>
--
Best regard
Hyman Huang(黄勇)
On Wed, Dec 15, 2021 at 03:56:55PM +0800, Hyman Huang wrote:
> > > +{ 'command': 'vcpu-dirty-limit',
> > > + 'data': { 'enable': 'bool',
> > > + '*cpu-index': 'uint64',
> > > + '*dirty-rate': 'uint64'} }
> >
> > Drop @enable, please.
> >
> > If @dirty-rate is present, set the limit to its value.
> >
> > If it's absent, cancel the limit.
> >
> Ok. Indeed, this is the simplest style. :)
>
> So the final qmp format should be like:
>
> case 1: setup vcpu 0 dirty page limit 100MB/s
> vcpu-dirty-limit cpu-index=0 dirty-rate=100MB/s
>
> case 2: cancle vcpu 0 dirty page limit
> vcpu-dirty-limit cpu-index=0
I actually agree with what you said... for human beings no one will read it as
"disable vcpu throttling", instead people could consider it enables vcpu
throttle with a default dirty rate from a gut feeling.
I think what Markus suggested is the simplest solution for computers, but it
can confuse human beings. So it turns out to be a general question to QMP
scheme design: should we always assume QMP client to be a piece of software, or
should we still consider the feeling of human beings operating on QMP
interfaces using qmp-shell.
IMHO we should still consider the latter, if we don't lose much, anyway. But I
don't have a strong opinion.
Thanks,
--
Peter Xu
Peter Xu <peterx@redhat.com> writes:
> On Wed, Dec 15, 2021 at 03:56:55PM +0800, Hyman Huang wrote:
>> > > +{ 'command': 'vcpu-dirty-limit',
>> > > + 'data': { 'enable': 'bool',
>> > > + '*cpu-index': 'uint64',
>> > > + '*dirty-rate': 'uint64'} }
>> >
>> > Drop @enable, please.
>> >
>> > If @dirty-rate is present, set the limit to its value.
>> >
>> > If it's absent, cancel the limit.
>> >
>> Ok. Indeed, this is the simplest style. :)
>>
>> So the final qmp format should be like:
>>
>> case 1: setup vcpu 0 dirty page limit 100MB/s
>> vcpu-dirty-limit cpu-index=0 dirty-rate=100MB/s
>>
>> case 2: cancle vcpu 0 dirty page limit
>> vcpu-dirty-limit cpu-index=0
>
> I actually agree with what you said... for human beings no one will read it as
> "disable vcpu throttling", instead people could consider it enables vcpu
> throttle with a default dirty rate from a gut feeling.
>
> I think what Markus suggested is the simplest solution for computers, but it
> can confuse human beings. So it turns out to be a general question to QMP
> scheme design: should we always assume QMP client to be a piece of software, or
> should we still consider the feeling of human beings operating on QMP
> interfaces using qmp-shell.
>
> IMHO we should still consider the latter, if we don't lose much, anyway. But I
> don't have a strong opinion.
If you want a more explicit interface, then I'd recommend to go right
back to v7:
{"execute": "set-vcpu-dirty-limit",
"arguments": {"cpu-index": 0, "dirtyrate": 200}}
{"execute": "cancel-vcpu-dirty-limit",
"arguments": {"cpu-index": 0}}
Bonus: it already has my Acked-by.
On Wed, Dec 15, 2021 at 02:41:32PM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
>
> > On Wed, Dec 15, 2021 at 03:56:55PM +0800, Hyman Huang wrote:
> >> > > +{ 'command': 'vcpu-dirty-limit',
> >> > > + 'data': { 'enable': 'bool',
> >> > > + '*cpu-index': 'uint64',
> >> > > + '*dirty-rate': 'uint64'} }
> >> >
> >> > Drop @enable, please.
> >> >
> >> > If @dirty-rate is present, set the limit to its value.
> >> >
> >> > If it's absent, cancel the limit.
> >> >
> >> Ok. Indeed, this is the simplest style. :)
> >>
> >> So the final qmp format should be like:
> >>
> >> case 1: setup vcpu 0 dirty page limit 100MB/s
> >> vcpu-dirty-limit cpu-index=0 dirty-rate=100MB/s
> >>
> >> case 2: cancle vcpu 0 dirty page limit
> >> vcpu-dirty-limit cpu-index=0
> >
> > I actually agree with what you said... for human beings no one will read it as
> > "disable vcpu throttling", instead people could consider it enables vcpu
> > throttle with a default dirty rate from a gut feeling.
> >
> > I think what Markus suggested is the simplest solution for computers, but it
> > can confuse human beings. So it turns out to be a general question to QMP
> > scheme design: should we always assume QMP client to be a piece of software, or
> > should we still consider the feeling of human beings operating on QMP
> > interfaces using qmp-shell.
> >
> > IMHO we should still consider the latter, if we don't lose much, anyway. But I
> > don't have a strong opinion.
>
> If you want a more explicit interface, then I'd recommend to go right
> back to v7:
>
> {"execute": "set-vcpu-dirty-limit",
> "arguments": {"cpu-index": 0, "dirtyrate": 200}}
>
> {"execute": "cancel-vcpu-dirty-limit",
> "arguments": {"cpu-index": 0}}
>
> Bonus: it already has my Acked-by.
Fair enough. :) That looks good to me too.
Yong, please hold-off a bit on reposting (if there's a plan) - I'll read the
other parts soon..
Thanks,
--
Peter Xu
在 2021/12/16 14:22, Peter Xu 写道:
> On Wed, Dec 15, 2021 at 02:41:32PM +0100, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>>
>>> On Wed, Dec 15, 2021 at 03:56:55PM +0800, Hyman Huang wrote:
>>>>>> +{ 'command': 'vcpu-dirty-limit',
>>>>>> + 'data': { 'enable': 'bool',
>>>>>> + '*cpu-index': 'uint64',
>>>>>> + '*dirty-rate': 'uint64'} }
>>>>>
>>>>> Drop @enable, please.
>>>>>
>>>>> If @dirty-rate is present, set the limit to its value.
>>>>>
>>>>> If it's absent, cancel the limit.
>>>>>
>>>> Ok. Indeed, this is the simplest style. :)
>>>>
>>>> So the final qmp format should be like:
>>>>
>>>> case 1: setup vcpu 0 dirty page limit 100MB/s
>>>> vcpu-dirty-limit cpu-index=0 dirty-rate=100MB/s
>>>>
>>>> case 2: cancle vcpu 0 dirty page limit
>>>> vcpu-dirty-limit cpu-index=0
>>>
>>> I actually agree with what you said... for human beings no one will read it as
>>> "disable vcpu throttling", instead people could consider it enables vcpu
>>> throttle with a default dirty rate from a gut feeling.
>>>
>>> I think what Markus suggested is the simplest solution for computers, but it
>>> can confuse human beings. So it turns out to be a general question to QMP
>>> scheme design: should we always assume QMP client to be a piece of software, or
>>> should we still consider the feeling of human beings operating on QMP
>>> interfaces using qmp-shell.
>>>
>>> IMHO we should still consider the latter, if we don't lose much, anyway. But I
>>> don't have a strong opinion.
>>
>> If you want a more explicit interface, then I'd recommend to go right
>> back to v7:
>>
>> {"execute": "set-vcpu-dirty-limit",
>> "arguments": {"cpu-index": 0, "dirtyrate": 200}}
>>
>> {"execute": "cancel-vcpu-dirty-limit",
>> "arguments": {"cpu-index": 0}}
>>
>> Bonus: it already has my Acked-by.
>
> Fair enough. :) That looks good to me too.
>
> Yong, please hold-off a bit on reposting (if there's a plan) - I'll read the
> other parts soon..
>
Ok, i'm not going to repost the next version untill the consensus is
achieved.
So the final format of qmp we conclude are:
case 1: setup vcpu 0 dirty page limit 100MB/s
set-vcpu-dirty-limit cpu-index=0 dirty-rate=100
case 2: setup all vcpu dirty page limit 100MB/s
set-vcpu-dirty-limit dirty-rate=100
case 3: cancel vcpu 0 dirty page limit
cancel-vcpu-dirty-limit cpu-index=0
case 4: cancel all vcpu dirty page limit
cancel-vcpu-dirty-limit
case 5: query limit infomatioin of all vcpu enabled
query-vcpu-dirty-limit
And the corresponding hmp format keep the same style:
Is there any advice? :)
> Thanks,
>
--
Best regard
Hyman Huang(黄勇)
Hyman Huang <huangy81@chinatelecom.cn> writes: [...] > So the final format of qmp we conclude are: > > case 1: setup vcpu 0 dirty page limit 100MB/s > set-vcpu-dirty-limit cpu-index=0 dirty-rate=100 > > case 2: setup all vcpu dirty page limit 100MB/s > set-vcpu-dirty-limit dirty-rate=100 > > case 3: cancel vcpu 0 dirty page limit > cancel-vcpu-dirty-limit cpu-index=0 > > case 4: cancel all vcpu dirty page limit > cancel-vcpu-dirty-limit > > case 5: query limit infomatioin of all vcpu enabled > query-vcpu-dirty-limit > > And the corresponding hmp format keep the same style: > > Is there any advice? :) Looks okay to me.
On Thu, Dec 16, 2021 at 11:23:37AM +0100, Markus Armbruster wrote: > Hyman Huang <huangy81@chinatelecom.cn> writes: > > [...] > > > So the final format of qmp we conclude are: > > > > case 1: setup vcpu 0 dirty page limit 100MB/s > > set-vcpu-dirty-limit cpu-index=0 dirty-rate=100 > > > > case 2: setup all vcpu dirty page limit 100MB/s > > set-vcpu-dirty-limit dirty-rate=100 > > > > case 3: cancel vcpu 0 dirty page limit > > cancel-vcpu-dirty-limit cpu-index=0 > > > > case 4: cancel all vcpu dirty page limit > > cancel-vcpu-dirty-limit > > > > case 5: query limit infomatioin of all vcpu enabled > > query-vcpu-dirty-limit > > > > And the corresponding hmp format keep the same style: > > > > Is there any advice? :) > > Looks okay to me. Same here. -- Peter Xu
在 2021/12/15 16:09, Peter Xu 写道:
> On Wed, Dec 15, 2021 at 03:56:55PM +0800, Hyman Huang wrote:
>>>> +{ 'command': 'vcpu-dirty-limit',
>>>> + 'data': { 'enable': 'bool',
>>>> + '*cpu-index': 'uint64',
>>>> + '*dirty-rate': 'uint64'} }
>>>
>>> Drop @enable, please.
>>>
>>> If @dirty-rate is present, set the limit to its value.
>>>
>>> If it's absent, cancel the limit.
>>>
>> Ok. Indeed, this is the simplest style. :)
>>
>> So the final qmp format should be like:
>>
>> case 1: setup vcpu 0 dirty page limit 100MB/s
>> vcpu-dirty-limit cpu-index=0 dirty-rate=100MB/s
>>
>> case 2: cancle vcpu 0 dirty page limit
>> vcpu-dirty-limit cpu-index=0
>
> I actually agree with what you said... for human beings no one will read it as
> "disable vcpu throttling", instead people could consider it enables vcpu
> throttle with a default dirty rate from a gut feeling.
>
> I think what Markus suggested is the simplest solution for computers, but it
> can confuse human beings. So it turns out to be a general question to QMP
> scheme design: should we always assume QMP client to be a piece of software, or
> should we still consider the feeling of human beings operating on QMP
> interfaces using qmp-shell.
>
> IMHO we should still consider the latter, if we don't lose much, anyway. But I
> don't have a strong opinion.
> > Thanks,
>
So, how do you think about it, Markus？
--
Best regard
Hyman Huang(黄勇)
在 2021/12/15 16:29, Hyman Huang 写道:
>
>
> 在 2021/12/15 16:09, Peter Xu 写道:
>> On Wed, Dec 15, 2021 at 03:56:55PM +0800, Hyman Huang wrote:
>>>>> +{ 'command': 'vcpu-dirty-limit',
>>>>> + 'data': { 'enable': 'bool',
>>>>> + '*cpu-index': 'uint64',
>>>>> + '*dirty-rate': 'uint64'} }
>>>>
>>>> Drop @enable, please.
>>>>
>>>> If @dirty-rate is present, set the limit to its value.
>>>>
>>>> If it's absent, cancel the limit.
>>>>
>>> Ok. Indeed, this is the simplest style. :)
>>>
>>> So the final qmp format should be like:
>>>
>>> case 1: setup vcpu 0 dirty page limit 100MB/s
>>> vcpu-dirty-limit cpu-index=0 dirty-rate=100MB/s
>>>
>>> case 2: cancle vcpu 0 dirty page limit
>>> vcpu-dirty-limit cpu-index=0
>>
>> I actually agree with what you said... for human beings no one will
>> read it as
>> "disable vcpu throttling", instead people could consider it enables vcpu
>> throttle with a default dirty rate from a gut feeling.
>>
>> I think what Markus suggested is the simplest solution for computers,
>> but it
>> can confuse human beings. So it turns out to be a general question to
>> QMP
>> scheme design: should we always assume QMP client to be a piece of
>> software, or
>> should we still consider the feeling of human beings operating on QMP
>> interfaces using qmp-shell.
>>
>> IMHO we should still consider the latter, if we don't lose much,
>> anyway. But I
>> don't have a strong opinion.
>> > Thanks,
>>
> So, how do you think about it, Markus？
I prefer Peter's advice and there is another reason:
In current implementation, we introduced the global on/off switch,
enable dirty page rate limit on specified vcpu only if @cpu-index is
passed, otherwise, all vcpu will be affected.
If we remove the @enable and use @dirty-rate to indicate the
enabled/disable switch. The qmp format should be like the following:
case 1: setup vcpu 0 dirty page limit 100MB/s
vcpu-dirty-limit cpu-index=0 dirty-rate=100MB/s
case 2: setup all vcpu dirty page limit 100MB/s
vcpu-dirty-limit dirty-rate=100MB/s
case 3: cancel vcpu 0 dirty page limit
vcpu-dirty-limit cpu-index=0
case 4: cancel all vcpu dirty page limit
vcpu-dirty-limit
For case 4, it seems to be weired.
--
Best regard
Hyman Huang(黄勇)
On Tue, Dec 14, 2021 at 07:07:34PM +0800, huangy81@chinatelecom.cn wrote:
> +void qmp_vcpu_dirty_limit(bool enable,
> + bool has_cpu_index,
> + uint64_t cpu_index,
> + bool has_dirty_rate,
> + uint64_t dirty_rate,
> + Error **errp)
> +{
> + static bool initialized;
IMHO this is not needed; if we're with a global state pointer then it's the
same to check against that.
The rest looks mostly good (besides the last proposal on API design which you
got confirmation from Markus).
--
Peter Xu
在 2021/12/24 13:14, Peter Xu 写道:
> On Tue, Dec 14, 2021 at 07:07:34PM +0800, huangy81@chinatelecom.cn wrote:
>> +void qmp_vcpu_dirty_limit(bool enable,
>> + bool has_cpu_index,
>> + uint64_t cpu_index,
>> + bool has_dirty_rate,
>> + uint64_t dirty_rate,
>> + Error **errp)
>> +{
>> + static bool initialized;
>
> IMHO this is not needed; if we're with a global state pointer then it's the
> same to check against that.
Sound good, this make code simpler.
>
> The rest looks mostly good (besides the last proposal on API design which you
> got confirmation from Markus).
>
© 2016 - 2026 Red Hat, Inc.