[PATCH v2 4/6] qemu_driver: Add mode option for qemuDomainStartDirtyRateCalc
Hyman Huang
huangy81 at chinatelecom.cn
Thu Jan 27 09:26:12 UTC 2022
在 2022/1/27 16:34, Peter Krempa 写道:
> On Thu, Jan 27, 2022 at 15:25:20 +0800, huangy81 at chinatelecom.cn wrote:
>> From: Hyman Huang(黄勇) <huangy81 at chinatelecom.cn>
>>
>> Add mode option to extend qemuDomainStartDirtyRateCalc API,
>> which is introduced since qemu >= 6.2.
>>
>> Signed-off-by: Hyman Huang(黄勇) <huangy81 at chinatelecom.cn>
>> ---
>> include/libvirt/libvirt-domain.h | 13 +++++++++++++
>> src/qemu/qemu_driver.c | 33 +++++++++++++++++++++++++++++++--
>> src/qemu/qemu_monitor.c | 5 +++--
>> src/qemu/qemu_monitor.h | 10 +++++++++-
>> src/qemu/qemu_monitor_json.c | 26 +++++++++++++++++++++-----
>> src/qemu/qemu_monitor_json.h | 3 ++-
>> 6 files changed, 79 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
>> index 4da1a63..54bb23b 100644
>> --- a/include/libvirt/libvirt-domain.h
>> +++ b/include/libvirt/libvirt-domain.h
>> @@ -5257,6 +5257,19 @@ typedef enum {
>> # endif
>> } virDomainDirtyRateStatus;
>>
>> +/**
>> + * virDomainDirtyRateCalcFlags:
>> + *
>> + * Flags OR'ed together to provide specific behaviour when calculating dirty page
>> + * rate for a Domain
>> + *
>> + */
>> +typedef enum {
>> + VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING = 0, /* default mode - page-sampling */
>> + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP = 1 << 0, /* dirty-bitmap mode */
>> + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING = 1 << 1, /* dirty-ring mode */
>> +} virDomainDirtyRateCalcFlags;
>> +
>> int virDomainStartDirtyRateCalc(virDomainPtr domain,
>> int seconds,
>> unsigned int flags);
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 0e8e9b1..feebfc4 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -20648,9 +20648,13 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
>> virDomainObj *vm = NULL;
>> qemuDomainObjPrivate *priv;
>> g_autoptr(virQEMUCaps) qemucaps = NULL;
>> + qemuMonitorDirtyRateCalcMode calcmode = VIR_DOMAIN_DIRTYRATE_CALC_MODE_PAGE_SAMPLING;
>> + bool mode = false;
>> int ret = -1;
>>
>> - virCheckFlags(0, -1);
>> + virCheckFlags(VIR_DOMAIN_DIRTYRATE_MODE_PAGE_SAMPLING |
>> + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP |
>> + VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING, -1);
>>
>> if (!(qemucaps = virQEMUCapsCacheLookupDefault(driver->qemuCapsCache,
>> NULL, NULL, NULL, NULL,
>> @@ -20663,6 +20667,15 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
>> return -1;
>> }
>>
>> + mode = virQEMUCapsGet(qemucaps, QEMU_CAPS_DIRTYRATE_MODE);
>
> Same issue as I've pointed out previously, you must use priv->qemuCaps.
>
Ok, i'll fix it
> Also you can then merge the two blocks which are guarded by 'mode' and
> get rid of the extra variable.
>
>> +
>> + if (!mode && (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP ||
>> + (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING))) {
>> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>> + _("QEMU does not support calculating dirty page rate"
>> + "with specified mode"));
>
> Here you are missing a goto cleanup or similar jump-out statement.
>
> Also please observe error messages in new code should not be line-broken
> even if the line will exceed 80 columns:
>
> https://www.libvirt.org/coding-style.html#error-message-format
>
>> + }
>
>> +
>> if (seconds < MIN_DIRTYRATE_CALC_PERIOD ||
>> seconds > MAX_DIRTYRATE_CALC_PERIOD) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> @@ -20676,6 +20689,22 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
>> if (!(vm = qemuDomainObjFromDomain(dom)))
>> return -1;
>>
>> + if (mode) {
>> + /* libvirt-domain.c already guaranteed these two flags are exclusive. */
>
> There isn't any code being added which would guarantee that, so the
> comment is either to be removed or something's missing in this commit.
>
>> + if (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_BITMAP) {
>> + calcmode = VIR_DOMAIN_DIRTYRATE_CALC_MODE_DIRTY_BITMAP;
>> + } else if (flags & VIR_DOMAIN_DIRTYRATE_MODE_DIRTY_RING) {
>> + if (vm->def->features[VIR_DOMAIN_FEATURE_KVM] != VIR_TRISTATE_SWITCH_ON ||
>> + vm->def->kvm_features->features[VIR_DOMAIN_KVM_DIRTY_RING] != VIR_TRISTATE_SWITCH_ON) {
>> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>
> All of this code must be moved below the ACL check or otherwise it could
> leak information about the VM object to users unauthorized to even see
> the VM object.
>
>
>> + _("Calculating dirty page rate with dirty-ring requires"
>> + "dirty-ring feature enabled."));
>> + goto cleanup;
>> + }
>> + calcmode = VIR_DOMAIN_DIRTYRATE_CALC_MODE_DIRTY_RING;
>> + }
>> + }
>> +
>> if (virDomainStartDirtyRateCalcEnsureACL(dom->conn, vm->def) < 0)
>> goto cleanup;
>>
>> @@ -20692,7 +20721,7 @@ qemuDomainStartDirtyRateCalc(virDomainPtr dom,
>>
>> priv = vm->privateData;
>> qemuDomainObjEnterMonitor(driver, vm);
>> - ret = qemuMonitorStartDirtyRateCalc(priv->mon, seconds);
>> + ret = qemuMonitorStartDirtyRateCalc(priv->mon, seconds, calcmode);
>>
>> qemuDomainObjExitMonitor(driver, vm);
>>
>
> [...]
>
>> struct _qemuMonitorDirtyRateInfo {
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index b0b5136..afbd721 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -8695,18 +8695,34 @@ qemuMonitorJSONGetCPUMigratable(qemuMonitor *mon,
>> migratable);
>> }
>>
>> +VIR_ENUM_DECL(qemuMonitorDirtyRateCalcMode);
>> +VIR_ENUM_IMPL(qemuMonitorDirtyRateCalcMode,
>> + VIR_DOMAIN_DIRTYRATE_CALC_MODE_LAST,
>> + "page-sampling",
>> + "dirty-bitmap",
>> + "dirty-ring");
>>
>> int
>> qemuMonitorJSONStartDirtyRateCalc(qemuMonitor *mon,
>> - int seconds)
>> + int seconds,
>> + qemuMonitorDirtyRateCalcMode mode)
>> {
>> g_autoptr(virJSONValue) cmd = NULL;
>> g_autoptr(virJSONValue) reply = NULL;
>>
>> - if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate",
>> - "i:calc-time", seconds,
>> - NULL)))
>> - return -1;
>> + if (mode == VIR_DOMAIN_DIRTYRATE_CALC_MODE_PAGE_SAMPLING) {
>> + if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate",
>> + "i:calc-time", seconds,
>> + NULL)))
>> + return -1;
>> + } else {
>> + const char *modestr = qemuMonitorDirtyRateCalcModeTypeToString(mode);
>> + if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate",
>> + "i:calc-time", seconds,
>> + "s:mode", modestr,
>> + NULL)))
>> + return -1;
>> + }
>
> You don't have to use two invocations of qemuMonitorJSONMakeCommand, but
> rather use 'S' for formatting the string which doesn't output the field
> if the string is null, so the code should look like:
>
> const char *modestr = NULL;
>
> if (mode != VIR_DOMAIN_DIRTYRATE_CALC_MODE_PAGE_SAMPLING)
> modestr = qemuMonitorDirtyRateCalcModeTypeToString(mode);
>
> if (!(cmd = qemuMonitorJSONMakeCommand("calc-dirty-rate",
> "i:calc-time", seconds,
> "S:mode", modestr,
> NULL)))
>
Sound good, :)
--
Best regard
Hyman Huang(黄勇)
More information about the libvir-list
mailing list