[PATCH v1 0/3] Introduce qemuDomainSetVcpuTuneParameters API
From: Hyman Huang <yong.huang@smartx.com> This patchset is the prerequisite for the vCPU dirty-limit feature: https://patchew.org/Libvirt/cover.1703135535.git.yong.huang@smartx.com/ As suggested by Daniel: We've generally tried to avoid adding single purpose APIs for tunable parameters, instead using APIs with virTypedParameter arrays to allow bulk updates. I note that we don't appear to have any mechanism currently to set the VCPU scheduler tunables either Perhaps we should have a more general virDomainSetVCPUTuneParameters(virDomainPtr domain, int vcpu, virTypedParameterPtr params, unsigned int params, unsigned int flags); Refer the following link to see more details: https://patchew.org/Libvirt/169397083100.4628.15196043252714532301-0@git.sr.... We present the qemuDomainSetVcpuTuneParameters API separately because the patchset is somewhat self-contained. Please review, Yong Hyman Huang (3): libvirt: Add virDomainSetVcpuTuneParameters API qemu_driver: Implement qemuDomainSetVcpuTuneParameters virsh: Use the new API to implement cmdSetvcpu include/libvirt/libvirt-domain.h | 25 ++++++++++++++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 56 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ src/qemu/qemu_driver.c | 29 +++++++++++++++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 20 +++++++++++- tools/virsh-domain.c | 10 +++++- 8 files changed, 152 insertions(+), 2 deletions(-) -- 2.27.0
From: Hyman Huang <yong.huang@smartx.com> Introduce virDomainSetVcpuTuneParameters API to support tunables of virtual CPUs. Signed-off-by: Hyman Huang <yong.huang@smartx.com> --- include/libvirt/libvirt-domain.h | 13 ++++++++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 56 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 20 +++++++++++- 6 files changed, 102 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index f5420bca6e..ae1e07b7b6 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -6612,4 +6612,17 @@ virDomainGraphicsReload(virDomainPtr domain, unsigned int type, unsigned int flags); +/** + * virDomainSetVcpuTuneParameters: + * + * Set virtual CPU tunables for the domain + * + * Since: 11.1.0 + */ +int +virDomainSetVcpuTuneParameters(virDomainPtr domain, + const char *vcpumap, + virTypedParameterPtr params, + int nparams, + unsigned int flags); #endif /* LIBVIRT_DOMAIN_H */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index 4ce8da078d..b8b8d53311 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1453,6 +1453,13 @@ typedef int unsigned int type, unsigned int flags); +typedef int +(*virDrvDomainSetVcpuTuneParameters)(virDomainPtr domain, + const char *vcpumap, + virTypedParameterPtr params, + int nparams, + unsigned int flags); + typedef struct _virHypervisorDriver virHypervisorDriver; /** @@ -1726,4 +1733,5 @@ struct _virHypervisorDriver { virDrvDomainStartDirtyRateCalc domainStartDirtyRateCalc; virDrvDomainFDAssociate domainFDAssociate; virDrvDomainGraphicsReload domainGraphicsReload; + virDrvDomainSetVcpuTuneParameters domainSetVcpuTuneParameters; }; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 072cc32255..90d02a2f06 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -14162,3 +14162,59 @@ virDomainGraphicsReload(virDomainPtr domain, virDispatchError(domain->conn); return -1; } + + +/** + * virDomainSetVcpuTuneParameters: + * @domain: pointer to domain object + * @vcpumap: text representation of a bitmap of vcpus to set + * @params: pointer to virtual CPU parameter objects + * @nparams: number of virtual CPU tunable parameter + * @flags: bitwise-OR of virDomainModificationImpact + * + * Change all or a subset of the virtual CPU tunables. + * + * Returns 0 in case of success, -1 in case of failure. + * + * Since: 11.1.0 + */ +int +virDomainSetVcpuTuneParameters(virDomainPtr domain, + const char *vcpumap, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "vcpumap='%s', params=%p, nparams=%d, flags=0x%x", + vcpumap, params, nparams, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + conn = domain->conn; + + virCheckReadOnlyGoto(conn->flags, error); + virCheckNonNullArgGoto(vcpumap, error); + virCheckNonNullArgGoto(params, error); + virCheckPositiveArgGoto(nparams, error); + + if (virTypedParameterValidateSet(conn, params, nparams) < 0) + goto error; + + if (conn->driver->domainSetVcpuTuneParameters) { + int ret; + ret = conn->driver->domainSetVcpuTuneParameters(domain, vcpumap, params, + nparams, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(domain->conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 7a3492d9d7..65d684b58b 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -948,4 +948,9 @@ LIBVIRT_10.2.0 { virDomainGraphicsReload; } LIBVIRT_10.1.0; +LIBVIRT_11.1.0 { + global: + virDomainSetVcpuTuneParameters; +} LIBVIRT_10.2.0; + # .... define new API here using predicted next version number .... diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 307f9ca945..eb3a47ef75 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7835,6 +7835,7 @@ static virHypervisorDriver hypervisor_driver = { .domainSetLaunchSecurityState = remoteDomainSetLaunchSecurityState, /* 8.0.0 */ .domainFDAssociate = remoteDomainFDAssociate, /* 9.0.0 */ .domainGraphicsReload = remoteDomainGraphicsReload, /* 10.2.0 */ + .domainSetVcpuTuneParameters = remoteDomainSetVcpuTuneParameters, /* 11.1.0 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 41c045ff78..f99f44c9b5 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -292,6 +292,9 @@ const REMOTE_DOMAIN_AUTHORIZED_SSH_KEYS_MAX = 2048; /* Upper limit on number of messages */ const REMOTE_DOMAIN_MESSAGES_MAX = 2048; +/* Upper limit on VCPU tunable parameters. */ +const REMOTE_DOMAIN_SET_VCPU_TUNE_PARAMETERS_MAX = 16; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN]; @@ -3973,6 +3976,13 @@ struct remote_domain_fd_associate_args { remote_nonnull_string name; unsigned int flags; }; + +struct remote_domain_set_vcpu_tune_parameters_args { + remote_nonnull_domain dom; + remote_nonnull_string cpumap; + remote_typed_param params<REMOTE_DOMAIN_SET_VCPU_TUNE_PARAMETERS_MAX>; + unsigned int flags; +}; /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -7048,5 +7058,13 @@ enum remote_procedure { * @generate: both * @acl: domain:write */ - REMOTE_PROC_DOMAIN_GRAPHICS_RELOAD = 448 + REMOTE_PROC_DOMAIN_GRAPHICS_RELOAD = 448, + + /** + * @generate: both + * @acl: domain:write + * @acl: domain:save:!VIR_DOMAIN_AFFECT_CONFIG|VIR_DOMAIN_AFFECT_LIVE + * @acl: domain:save:VIR_DOMAIN_AFFECT_CONFIG + */ + REMOTE_PROC_DOMAIN_SET_VCPU_TUNE_PARAMETERS = 449 }; -- 2.27.0
From: Hyman Huang <yong.huang@smartx.com> Support hotplug/hotunplug of virtual CPU by wrapping the existing interface qemuDomainSetVcpu. Signed-off-by: Hyman Huang <yong.huang@smartx.com> --- include/libvirt/libvirt-domain.h | 12 ++++++++++++ src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index ae1e07b7b6..16e3140e09 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -6612,6 +6612,18 @@ virDomainGraphicsReload(virDomainPtr domain, unsigned int type, unsigned int flags); +/* Virtual CPU set parameters */ + +/** + * VIR_DOMAIN_VCPU_STATE: + * + * The tunable enable/disable individual vcpus described by @vcpumap + * in the hypervisor* + * + * Since: 11.1.0 + */ +# define VIR_DOMAIN_VCPU_STATE "state" + /** * virDomainSetVcpuTuneParameters: * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 772cb405d6..59d94c6dd6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20081,6 +20081,34 @@ qemuDomainGraphicsReload(virDomainPtr domain, return ret; } +static int +qemuDomainSetVcpuTuneParameters(virDomainPtr dom, + const char *vcpumap, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + int state; + int rc; + + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_VCPU_STATE, + VIR_TYPED_PARAM_INT, + NULL) < 0) + return -1; + + if ((rc = virTypedParamsGetInt(params, nparams, + VIR_DOMAIN_VCPU_STATE, + &state)) < 0) + return -1; + + if (rc == 1) { + return qemuDomainSetVcpu(dom, vcpumap, state, flags); + } + + return 0; +} + static virHypervisorDriver qemuHypervisorDriver = { .name = QEMU_DRIVER_NAME, .connectURIProbe = qemuConnectURIProbe, @@ -20331,6 +20359,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainSetLaunchSecurityState = qemuDomainSetLaunchSecurityState, /* 8.0.0 */ .domainFDAssociate = qemuDomainFDAssociate, /* 9.0.0 */ .domainGraphicsReload = qemuDomainGraphicsReload, /* 10.2.0 */ + .domainSetVcpuTuneParameters = qemuDomainSetVcpuTuneParameters, /* 11.1.0 */ }; -- 2.27.0
On Sat, Feb 15, 2025 at 16:35:52 +0800, yong.huang@smartx.com wrote:
From: Hyman Huang <yong.huang@smartx.com>
Support hotplug/hotunplug of virtual CPU by wrapping the existing interface qemuDomainSetVcpu.
Signed-off-by: Hyman Huang <yong.huang@smartx.com> --- include/libvirt/libvirt-domain.h | 12 ++++++++++++ src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index ae1e07b7b6..16e3140e09 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -6612,6 +6612,18 @@ virDomainGraphicsReload(virDomainPtr domain, unsigned int type, unsigned int flags);
+/* Virtual CPU set parameters */ + +/** + * VIR_DOMAIN_VCPU_STATE: + * + * The tunable enable/disable individual vcpus described by @vcpumap + * in the hypervisor* + * + * Since: 11.1.0 + */ +# define VIR_DOMAIN_VCPU_STATE "state" + /** * virDomainSetVcpuTuneParameters: * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 772cb405d6..59d94c6dd6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20081,6 +20081,34 @@ qemuDomainGraphicsReload(virDomainPtr domain, return ret; }
+static int +qemuDomainSetVcpuTuneParameters(virDomainPtr dom, + const char *vcpumap, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + int state; + int rc; + + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_VCPU_STATE, + VIR_TYPED_PARAM_INT, + NULL) < 0) + return -1; + + if ((rc = virTypedParamsGetInt(params, nparams, + VIR_DOMAIN_VCPU_STATE, + &state)) < 0) + return -1; + + if (rc == 1) { + return qemuDomainSetVcpu(dom, vcpumap, state, flags);
I don't think we want to expose cpu hotplug via another API. Especially this has weird semantics once you try to combine it with other "parameter" setting. E.g. what if you set CPU to disabled, but set some other parameters? It has even ordering implications because enabling CPU needs to happen first. I don't think this should be done. CPU state doesn't even feel like a 'tunable' variable. NACK
+ } + + return 0; +}
On Thu, Mar 6, 2025 at 11:57 PM Peter Krempa <pkrempa@redhat.com> wrote:
On Sat, Feb 15, 2025 at 16:35:52 +0800, yong.huang@smartx.com wrote:
From: Hyman Huang <yong.huang@smartx.com>
Support hotplug/hotunplug of virtual CPU by wrapping the existing interface qemuDomainSetVcpu.
Signed-off-by: Hyman Huang <yong.huang@smartx.com> --- include/libvirt/libvirt-domain.h | 12 ++++++++++++ src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index ae1e07b7b6..16e3140e09 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -6612,6 +6612,18 @@ virDomainGraphicsReload(virDomainPtr domain, unsigned int type, unsigned int flags);
+/* Virtual CPU set parameters */ + +/** + * VIR_DOMAIN_VCPU_STATE: + * + * The tunable enable/disable individual vcpus described by @vcpumap + * in the hypervisor* + * + * Since: 11.1.0 + */ +# define VIR_DOMAIN_VCPU_STATE "state" + /** * virDomainSetVcpuTuneParameters: * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 772cb405d6..59d94c6dd6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20081,6 +20081,34 @@ qemuDomainGraphicsReload(virDomainPtr domain, return ret; }
+static int +qemuDomainSetVcpuTuneParameters(virDomainPtr dom, + const char *vcpumap, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + int state; + int rc; + + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_VCPU_STATE, + VIR_TYPED_PARAM_INT, + NULL) < 0) + return -1; + + if ((rc = virTypedParamsGetInt(params, nparams, + VIR_DOMAIN_VCPU_STATE, + &state)) < 0) + return -1; + + if (rc == 1) { + return qemuDomainSetVcpu(dom, vcpumap, state, flags);
I don't think we want to expose cpu hotplug via another API. Especially this has weird semantics once you try to combine it with other "parameter" setting.
E.g. what if you set CPU to disabled, but set some other parameters?
It has even ordering implications because enabling CPU needs to happen first.
I don't think this should be done. CPU state doesn't even feel like a 'tunable' variable.
Yes, I also feel kind of weird. What about the dirty rate limit(dirty-limit) for CPU, I think that is a 'tunable' variable.
NACK
+ } + + return 0; +}
-- Best regards
From: Hyman Huang <yong.huang@smartx.com> Signed-off-by: Hyman Huang <yong.huang@smartx.com> --- tools/virsh-domain.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f3da2f903f..83db56460b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7345,6 +7345,7 @@ cmdSetvcpu(vshControl *ctl, const vshCmd *cmd) const char *vcpulist = NULL; int state = 0; unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + virTypedParameterPtr params = NULL; VSH_EXCLUSIVE_OPTIONS_VAR(enable, disable); @@ -7370,7 +7371,14 @@ cmdSetvcpu(vshControl *ctl, const vshCmd *cmd) if (enable) state = 1; - if (virDomainSetVcpu(dom, vcpulist, state, flags) < 0) + params = g_new0(virTypedParameter, 1); + if (virTypedParameterAssign(¶ms[0], + VIR_DOMAIN_VCPU_STATE, + VIR_TYPED_PARAM_INT, + state) < 0) + return false; + + if (virDomainSetVcpuTuneParameters(dom, vcpulist, params, 1, flags) < 0) return false; return true; -- 2.27.0
On Sat, Feb 15, 2025 at 04:35:50PM +0800, yong.huang@smartx.com wrote:
From: Hyman Huang <yong.huang@smartx.com>
This patchset is the prerequisite for the vCPU dirty-limit feature: https://patchew.org/Libvirt/cover.1703135535.git.yong.huang@smartx.com/
Is the dirty limit migration strategy dependent on setting the dirty limit beforehand?
As suggested by Daniel:
We've generally tried to avoid adding single purpose APIs for tunable parameters, instead using APIs with virTypedParameter arrays to allow bulk updates.
I note that we don't appear to have any mechanism currently to set the VCPU scheduler tunables either
Perhaps we should have a more general
virDomainSetVCPUTuneParameters(virDomainPtr domain, int vcpu, virTypedParameterPtr params, unsigned int params, unsigned int flags);
Refer the following link to see more details: https://patchew.org/Libvirt/169397083100.4628.15196043252714532301-0@git.sr....
We present the qemuDomainSetVcpuTuneParameters API separately because the patchset is somewhat self-contained.
Are you suggesting adding this API for now and then adding the dirty limit and others into it? Please send it together with at least the other typed parameters. If you want to wait with the migration stuff and rest of the things, that's fine.
Please review,
Yong
Hyman Huang (3): libvirt: Add virDomainSetVcpuTuneParameters API qemu_driver: Implement qemuDomainSetVcpuTuneParameters virsh: Use the new API to implement cmdSetvcpu
Also this last patch will make `virsh setvcpu` work only with new daemon, but it should be able to work with older versions as well.
include/libvirt/libvirt-domain.h | 25 ++++++++++++++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 56 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ src/qemu/qemu_driver.c | 29 +++++++++++++++++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 20 +++++++++++- tools/virsh-domain.c | 10 +++++- 8 files changed, 152 insertions(+), 2 deletions(-)
-- 2.27.0
participants (4)
-
Martin Kletzander -
Peter Krempa -
Yong Huang -
yong.huang＠smartx.com