Message ID | 1595646669-109310-9-git-send-email-zhengchuan@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | *** A Method for evaluating dirty page rate *** | expand |
On 7/24/20 10:11 PM, Chuan Zheng wrote: > From: Zheng Chuan <zhengchuan@huawei.com> > > Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called > by libvirt api. > > Signed-off-by: Zheng Chuan <zhengchuan@huawei.com> > Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com> > --- > +++ b/qapi/migration.json > @@ -1621,3 +1621,27 @@ > ## > { 'event': 'UNPLUG_PRIMARY', > 'data': { 'device-id': 'str' } } > + > +## > +# @cal_dirty_rate: New QMP commands should be named favoring '-' over '_'; also, it doesn't hurt to spell it out: calculate-dirty-rate > +# > +# start calculating dirty rate for vm > +# > +# @value: time for sample dirty pages In what unit? > +# > +# Since: 5.1 We've missed 5.1; this will need to be updated to 5.2. > +# > +# Example: > +# {"command": "cal_dirty_rate", "data": {"value": 1} } > +# > +## > +{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} } > + > +## > +# @get_dirty_rate: get-dirty-rate, except that we tend to use 'query-' as the prefix for commands that read values. > +# > +# get dirty rate for vm > +# > +# Since: 5.1 5.2 What units is the rate expressed in? > +## > +{ 'command': 'get_dirty_rate', 'returns': 'int64' } > diff --git a/qapi/pragma.json b/qapi/pragma.json > index cffae27..ecd294b 100644 > --- a/qapi/pragma.json > +++ b/qapi/pragma.json > @@ -10,7 +10,8 @@ > 'query-migrate-cache-size', > 'query-tpm-models', > 'query-tpm-types', > - 'ringbuf-read' ], > + 'ringbuf-read', > + 'get_dirty_rate' ], > 'name-case-whitelist': [ > 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status > 'CpuInfoMIPS', # PC, visible through query-cpu >
On 7/24/20 10:11 PM, Chuan Zheng wrote: > From: Zheng Chuan <zhengchuan@huawei.com> > > Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called > by libvirt api. > > Signed-off-by: Zheng Chuan <zhengchuan@huawei.com> > Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com> > +## > +{ 'command': 'get_dirty_rate', 'returns': 'int64' } > diff --git a/qapi/pragma.json b/qapi/pragma.json > index cffae27..ecd294b 100644 > --- a/qapi/pragma.json > +++ b/qapi/pragma.json > @@ -10,7 +10,8 @@ > 'query-migrate-cache-size', > 'query-tpm-models', > 'query-tpm-types', > - 'ringbuf-read' ], > + 'ringbuf-read', > + 'get_dirty_rate' ], Nack. You should not have to change the whitelist; this is evidence that your command is returning the wrong type. Instead, you should be using: { 'command': 'get-dirty-rate', 'returns': { 'rate': 'int64' } } and populating a struct, so that if we ever want to return more than just a single rate, we can extend the command in-place by adding to the struct.
* Chuan Zheng (zhengchuan@huawei.com) wrote: > From: Zheng Chuan <zhengchuan@huawei.com> > > Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called > by libvirt api. > > Signed-off-by: Zheng Chuan <zhengchuan@huawei.com> > Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com> > --- > migration/Makefile.objs | 1 + > migration/dirtyrate.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > qapi/migration.json | 24 ++++++++++++++++++++++++ > qapi/pragma.json | 3 ++- > 4 files changed, 72 insertions(+), 1 deletion(-) > > diff --git a/migration/Makefile.objs b/migration/Makefile.objs > index 0fc619e..12ae98c 100644 > --- a/migration/Makefile.objs > +++ b/migration/Makefile.objs > @@ -6,6 +6,7 @@ common-obj-y += qemu-file.o global_state.o > common-obj-y += qemu-file-channel.o > common-obj-y += xbzrle.o postcopy-ram.o > common-obj-y += qjson.o > +common-obj-y += dirtyrate.o > common-obj-y += block-dirty-bitmap.o > common-obj-y += multifd.o > common-obj-y += multifd-zlib.o > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c > index d87e16d..5717521 100644 > --- a/migration/dirtyrate.c > +++ b/migration/dirtyrate.c > @@ -31,6 +31,21 @@ CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT; > INTERNAL_RAMBLOCK_FOREACH(block) \ > if (!qemu_ram_is_migratable(block)) {} else > > +static int64_t get_dirty_rate_info(void) > +{ > + int64_t dirty_rate = dirty_stat.dirty_rate; > + /* > + * Return dirty_rate only when we get CAL_DIRTY_RATE_END. > + * And we must initial calculating_dirty_rate_stage. > + */ > + if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_END) { > + calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT; > + return dirty_rate; I don't think a 'get' should change state - in particular I think we should be able to call 'get' multiple times and get the result back. Also, instead of just returning -1 you should probably give an error; e.g. 'Still measuring' or 'not yet measured' > + } else { > + return -1; > + } > +} > + > static void reset_dirtyrate_stat(void) > { > dirty_stat.total_dirty_samples = 0; > @@ -377,3 +392,33 @@ void *get_dirtyrate_thread(void *arg) > > return NULL; > } > + > +void qmp_cal_dirty_rate(int64_t value, Error **errp) > +{ > + static struct dirtyrate_config dirtyrate_config; > + QemuThread thread; > + > + /* > + * We don't begin calculating thread only when it's in calculating status. > + */ > + if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_ING) { > + return; > + } > + > + /* > + * If we get CAL_DIRTY_RATE_END here, libvirtd may be restarted at last round, > + * we need to initial it. > + */ > + if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_END) > + calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT; Note please run the patches through scripts/checkpatch.pl - you need some {'s here and that script tells you about most of these things. > + dirtyrate_config.sample_period_seconds = value; Ah, this is where the config comes from - OK, I think you said there was a max, so you should probably check that the maximum is sensible here and give an error if it's not. Dave > + dirtyrate_config.sample_pages_per_gigabytes = sample_pages_per_gigabytes; > + qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread, > + (void *)&dirtyrate_config, QEMU_THREAD_DETACHED); > +} > + > +int64_t qmp_get_dirty_rate(Error **errp) > +{ > + return get_dirty_rate_info(); > +} > diff --git a/qapi/migration.json b/qapi/migration.json > index d500055..59f35bb 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1621,3 +1621,27 @@ > ## > { 'event': 'UNPLUG_PRIMARY', > 'data': { 'device-id': 'str' } } > + > +## > +# @cal_dirty_rate: > +# > +# start calculating dirty rate for vm > +# > +# @value: time for sample dirty pages > +# > +# Since: 5.1 > +# > +# Example: > +# {"command": "cal_dirty_rate", "data": {"value": 1} } > +# > +## > +{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} } I'm OK with abbreviating 'calculate' but prefer calc. > +## > +# @get_dirty_rate: > +# > +# get dirty rate for vm > +# > +# Since: 5.1 > +## > +{ 'command': 'get_dirty_rate', 'returns': 'int64' } > diff --git a/qapi/pragma.json b/qapi/pragma.json > index cffae27..ecd294b 100644 > --- a/qapi/pragma.json > +++ b/qapi/pragma.json > @@ -10,7 +10,8 @@ > 'query-migrate-cache-size', > 'query-tpm-models', > 'query-tpm-types', > - 'ringbuf-read' ], > + 'ringbuf-read', > + 'get_dirty_rate' ], > 'name-case-whitelist': [ > 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status > 'CpuInfoMIPS', # PC, visible through query-cpu > -- > 1.8.3.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 2020/8/5 0:28, Eric Blake wrote: > On 7/24/20 10:11 PM, Chuan Zheng wrote: >> From: Zheng Chuan <zhengchuan@huawei.com> >> >> Implement qmp_cal_dirty_rate()/qmp_get_dirty_rate() function which could be called >> by libvirt api. >> >> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com> >> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com> >> --- > >> +++ b/qapi/migration.json >> @@ -1621,3 +1621,27 @@ >> ## >> { 'event': 'UNPLUG_PRIMARY', >> 'data': { 'device-id': 'str' } } >> + >> +## >> +# @cal_dirty_rate: > > New QMP commands should be named favoring '-' over '_'; also, it doesn't hurt to spell it out: > > calculate-dirty-rate > Hi, Eric. Thank you for your review. I'll fix that in V2. >> +# >> +# start calculating dirty rate for vm >> +# >> +# @value: time for sample dirty pages > > In what unit? > The unit is second, i'll make comments in V2:) >> +# >> +# Since: 5.1 > > We've missed 5.1; this will need to be updated to 5.2. > >> +# >> +# Example: >> +# {"command": "cal_dirty_rate", "data": {"value": 1} } >> +# >> +## >> +{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} } >> + >> +## >> +# @get_dirty_rate: > > get-dirty-rate, except that we tend to use 'query-' as the prefix for commands that read values. > >> +# >> +# get dirty rate for vm >> +# >> +# Since: 5.1 > > 5.2 > > What units is the rate expressed in? > The unit is MB/s, i'll make comments in V2:) > >> +## >> +{ 'command': 'get_dirty_rate', 'returns': 'int64' } >> diff --git a/qapi/pragma.json b/qapi/pragma.json >> index cffae27..ecd294b 100644 >> --- a/qapi/pragma.json >> +++ b/qapi/pragma.json >> @@ -10,7 +10,8 @@ >> 'query-migrate-cache-size', >> 'query-tpm-models', >> 'query-tpm-types', >> - 'ringbuf-read' ], >> + 'ringbuf-read', >> + 'get_dirty_rate' ], >> 'name-case-whitelist': [ >> 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status >> 'CpuInfoMIPS', # PC, visible through query-cpu >> >
diff --git a/migration/Makefile.objs b/migration/Makefile.objs index 0fc619e..12ae98c 100644 --- a/migration/Makefile.objs +++ b/migration/Makefile.objs @@ -6,6 +6,7 @@ common-obj-y += qemu-file.o global_state.o common-obj-y += qemu-file-channel.o common-obj-y += xbzrle.o postcopy-ram.o common-obj-y += qjson.o +common-obj-y += dirtyrate.o common-obj-y += block-dirty-bitmap.o common-obj-y += multifd.o common-obj-y += multifd-zlib.o diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c index d87e16d..5717521 100644 --- a/migration/dirtyrate.c +++ b/migration/dirtyrate.c @@ -31,6 +31,21 @@ CalculatingDirtyRateStage calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT; INTERNAL_RAMBLOCK_FOREACH(block) \ if (!qemu_ram_is_migratable(block)) {} else +static int64_t get_dirty_rate_info(void) +{ + int64_t dirty_rate = dirty_stat.dirty_rate; + /* + * Return dirty_rate only when we get CAL_DIRTY_RATE_END. + * And we must initial calculating_dirty_rate_stage. + */ + if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_END) { + calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT; + return dirty_rate; + } else { + return -1; + } +} + static void reset_dirtyrate_stat(void) { dirty_stat.total_dirty_samples = 0; @@ -377,3 +392,33 @@ void *get_dirtyrate_thread(void *arg) return NULL; } + +void qmp_cal_dirty_rate(int64_t value, Error **errp) +{ + static struct dirtyrate_config dirtyrate_config; + QemuThread thread; + + /* + * We don't begin calculating thread only when it's in calculating status. + */ + if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_ING) { + return; + } + + /* + * If we get CAL_DIRTY_RATE_END here, libvirtd may be restarted at last round, + * we need to initial it. + */ + if (calculating_dirty_rate_stage == CAL_DIRTY_RATE_END) + calculating_dirty_rate_stage = CAL_DIRTY_RATE_INIT; + + dirtyrate_config.sample_period_seconds = value; + dirtyrate_config.sample_pages_per_gigabytes = sample_pages_per_gigabytes; + qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread, + (void *)&dirtyrate_config, QEMU_THREAD_DETACHED); +} + +int64_t qmp_get_dirty_rate(Error **errp) +{ + return get_dirty_rate_info(); +} diff --git a/qapi/migration.json b/qapi/migration.json index d500055..59f35bb 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1621,3 +1621,27 @@ ## { 'event': 'UNPLUG_PRIMARY', 'data': { 'device-id': 'str' } } + +## +# @cal_dirty_rate: +# +# start calculating dirty rate for vm +# +# @value: time for sample dirty pages +# +# Since: 5.1 +# +# Example: +# {"command": "cal_dirty_rate", "data": {"value": 1} } +# +## +{ 'command': 'cal_dirty_rate', 'data': {'value': 'int64'} } + +## +# @get_dirty_rate: +# +# get dirty rate for vm +# +# Since: 5.1 +## +{ 'command': 'get_dirty_rate', 'returns': 'int64' } diff --git a/qapi/pragma.json b/qapi/pragma.json index cffae27..ecd294b 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -10,7 +10,8 @@ 'query-migrate-cache-size', 'query-tpm-models', 'query-tpm-types', - 'ringbuf-read' ], + 'ringbuf-read', + 'get_dirty_rate' ], 'name-case-whitelist': [ 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status 'CpuInfoMIPS', # PC, visible through query-cpu