[libvirt] [PATCH 0/7] Implement Set IOThreads Command
These patches will implement the Set IOThreads command to complete the IOThreads family of calls/settings. There are 3 patches in the middle (3, 4, & 5) which are just refactoring the SetVcpus code so that the SetIOThreads code can make use of the same functionality rather than cut-n-paste John Ferlan (7): Implement public API for virDomainIOThreadsSet remote: Plumbing for virDomainSetIOThreads qemu: qemuDomainHotplugVcpus - separate out the add cgroup qemu: qemuDomainHotplugVcpus - separate out the del cgroup and pin qemu: qemuDomainHotplugVcpus - separate out pin adjustment code qemu: Add support to set IOThreads count virsh: Add setiothreads command include/libvirt/libvirt-domain.h | 3 + src/conf/domain_audit.c | 9 + src/conf/domain_audit.h | 6 + src/driver-hypervisor.h | 6 + src/libvirt-domain.c | 60 +++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 1 + src/qemu/qemu_driver.c | 544 +++++++++++++++++++++++++++++++++------ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +- src/remote_protocol-structs | 6 + tools/virsh-domain.c | 82 ++++++ tools/virsh.pod | 17 ++ 13 files changed, 676 insertions(+), 76 deletions(-) -- 2.1.0
Add virDomainIOThreadsSet to allow setting the number of IOThreads for a domain. The API supports updating both the --live domain and the --config data. Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 3 ++ src/driver-hypervisor.h | 6 ++++ src/libvirt-domain.c | 60 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 70 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 5d1d868..d7ce6f8 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1615,6 +1615,9 @@ int virDomainPinIOThread(virDomainPtr domain, unsigned char *cpumap, int maplen, unsigned int flags); +int virDomainSetIOThreads(virDomainPtr domain, + unsigned int niothreads, + unsigned int flags); /** * VIR_USE_CPU: diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index 3f9bf02..2e4104b 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -393,6 +393,11 @@ typedef int unsigned int flags); typedef int +(*virDrvDomainSetIOThreads)(virDomainPtr domain, + unsigned int iothreads, + unsigned int flags); + +typedef int (*virDrvDomainGetSecurityLabel)(virDomainPtr domain, virSecurityLabelPtr seclabel); @@ -1273,6 +1278,7 @@ struct _virHypervisorDriver { virDrvDomainGetMaxVcpus domainGetMaxVcpus; virDrvDomainGetIOThreadsInfo domainGetIOThreadsInfo; virDrvDomainPinIOThread domainPinIOThread; + virDrvDomainSetIOThreads domainSetIOThreads; virDrvDomainGetSecurityLabel domainGetSecurityLabel; virDrvDomainGetSecurityLabelList domainGetSecurityLabelList; virDrvNodeGetSecurityModel nodeGetSecurityModel; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 0bd9274..7176301 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8042,6 +8042,66 @@ virDomainPinIOThread(virDomainPtr domain, /** + * virDomainSetIOThreads: + * @domain: pointer to domain object + * @niothreads: the new number of IOThreads for this domain + * @flags: bitwise-OR of virDomainModificationImpact + * + * Dynamically change the number of IOThreads used by the domain. + * Note that this call faiy fail if the underlying virtualization hypervisor + * does not support it or if growing the number is arbitrarily limited. + * This function may require privileged access to the hypervisor. + * + * @flags may include VIR_DOMAIN_AFFECT_LIVE to affect a running + * domain (which may fail if domain is not active), or + * VIR_DOMAIN_AFFECT_CONFIG to affect the next boot via the XML + * description of the domain. Both flags may be set. + * If neither flag is specified (that is, @flags is VIR_DOMAIN_AFFECT_CURRENT), + * then an inactive domain modifies persistent setup, while an active domain + * is hypervisor-dependent on whether just live or both live and persistent + * state is changed. + * + * Not all hypervisors can support all flag combinations. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainSetIOThreads(virDomainPtr domain, + unsigned int niothreads, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "niothreads=%u, flags=%x", niothreads, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckReadOnlyGoto(domain->conn->flags, error); + + if ((unsigned short) niothreads != niothreads) { + virReportError(VIR_ERR_OVERFLOW, _("input too large: %u"), niothreads); + goto error; + } + conn = domain->conn; + + if (conn->driver->domainSetIOThreads) { + int ret; + ret = conn->driver->domainSetIOThreads(domain, niothreads, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(domain->conn); + return -1; +} + + +/** * virDomainGetSecurityLabel: * @domain: a domain object * @seclabel: pointer to a virSecurityLabel structure diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index e4cf7ed..fdcbd6f 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -702,6 +702,7 @@ LIBVIRT_1.2.14 { virDomainPinIOThread; virDomainInterfaceAddresses; virDomainInterfaceFree; + virDomainSetIOThreads; } LIBVIRT_1.2.12; # .... define new API here using predicted next version number .... -- 2.1.0
On Thu, Mar 19, 2015 at 01:08:22PM -0400, John Ferlan wrote:
Add virDomainIOThreadsSet to allow setting the number of IOThreads for a domain.
The API supports updating both the --live domain and the --config data.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 3 ++ src/driver-hypervisor.h | 6 ++++ src/libvirt-domain.c | 60 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 70 insertions(+)
This one has been siting there for a while, I'll think out loud just to get a discussion going.
--- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8042,6 +8042,66 @@ virDomainPinIOThread(virDomainPtr domain,
/** + * virDomainSetIOThreads: + * @domain: pointer to domain object + * @niothreads: the new number of IOThreads for this domain + * @flags: bitwise-OR of virDomainModificationImpact + * + * Dynamically change the number of IOThreads used by the domain. + * Note that this call faiy fail if the underlying virtualization hypervisor + * does not support it or if growing the number is arbitrarily limited. + * This function may require privileged access to the hypervisor. + * + * @flags may include VIR_DOMAIN_AFFECT_LIVE to affect a running + * domain (which may fail if domain is not active), or + * VIR_DOMAIN_AFFECT_CONFIG to affect the next boot via the XML + * description of the domain. Both flags may be set. + * If neither flag is specified (that is, @flags is VIR_DOMAIN_AFFECT_CURRENT), + * then an inactive domain modifies persistent setup, while an active domain + * is hypervisor-dependent on whether just live or both live and persistent + * state is changed. + * + * Not all hypervisors can support all flag combinations. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainSetIOThreads(virDomainPtr domain, + unsigned int niothreads, + unsigned int flags) +{
This seems ok for the use case where we just give a domain a few iothreads and all the disks will share them. But if you go for one iothread per disk, creating holes in iothread indexes could make sense - we allow unplugging disks from the middle. I can't think of a case where hotplugging a thread with id=6 would be useful if there are only 3 threads. Maybe there should be separate APIs for adding and deleting threads? Jan
On Wed, Mar 25, 2015 at 16:29:22 +0100, Ján Tomko wrote:
On Thu, Mar 19, 2015 at 01:08:22PM -0400, John Ferlan wrote:
Add virDomainIOThreadsSet to allow setting the number of IOThreads for a domain.
The API supports updating both the --live domain and the --config data.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 3 ++ src/driver-hypervisor.h | 6 ++++ src/libvirt-domain.c | 60 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 70 insertions(+)
This one has been siting there for a while, I'll think out loud just to get a discussion going.
--- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8042,6 +8042,66 @@ virDomainPinIOThread(virDomainPtr domain,
/** + * virDomainSetIOThreads: + * @domain: pointer to domain object + * @niothreads: the new number of IOThreads for this domain + * @flags: bitwise-OR of virDomainModificationImpact + * + * Dynamically change the number of IOThreads used by the domain. + * Note that this call faiy fail if the underlying virtualization hypervisor + * does not support it or if growing the number is arbitrarily limited. + * This function may require privileged access to the hypervisor. + * + * @flags may include VIR_DOMAIN_AFFECT_LIVE to affect a running + * domain (which may fail if domain is not active), or + * VIR_DOMAIN_AFFECT_CONFIG to affect the next boot via the XML + * description of the domain. Both flags may be set. + * If neither flag is specified (that is, @flags is VIR_DOMAIN_AFFECT_CURRENT), + * then an inactive domain modifies persistent setup, while an active domain + * is hypervisor-dependent on whether just live or both live and persistent + * state is changed. + * + * Not all hypervisors can support all flag combinations. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainSetIOThreads(virDomainPtr domain, + unsigned int niothreads, + unsigned int flags) +{
This seems ok for the use case where we just give a domain a few iothreads and all the disks will share them.
But if you go for one iothread per disk, creating holes in iothread indexes could make sense - we allow unplugging disks from the middle.
I can't think of a case where hotplugging a thread with id=6 would be useful if there are only 3 threads.
Maybe there should be separate APIs for adding and deleting threads?
I have to agree here. Separate API for adding and removing (or perhaps just a parameter to switch?) specific threads is the way to go. Why? Well this function looks like it was copied from virDomainSetVcpus which I'm currently planning to replace with specific vcpu hotplug api. The reasons there are the same as Jan pointed out just translated into the context of vCPUS. At the point where there is no other mapping or relationship of belonging the API is fine. This was true for vCPUs in the pre-NUMA era. With NUMA, (or disks IO executed by iothreads in this case) you have a relationship where a thread (or vCPU) belongs to a dis (or NUMA node) and you need to be able to select them individually.
Jan
Peter
On 03/25/2015 11:37 AM, Peter Krempa wrote:
On Wed, Mar 25, 2015 at 16:29:22 +0100, Ján Tomko wrote:
On Thu, Mar 19, 2015 at 01:08:22PM -0400, John Ferlan wrote:
Add virDomainIOThreadsSet to allow setting the number of IOThreads for a domain.
The API supports updating both the --live domain and the --config data.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 3 ++ src/driver-hypervisor.h | 6 ++++ src/libvirt-domain.c | 60 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 70 insertions(+)
This one has been siting there for a while, I'll think out loud just to get a discussion going.
--- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8042,6 +8042,66 @@ virDomainPinIOThread(virDomainPtr domain,
/** + * virDomainSetIOThreads: + * @domain: pointer to domain object + * @niothreads: the new number of IOThreads for this domain + * @flags: bitwise-OR of virDomainModificationImpact + * + * Dynamically change the number of IOThreads used by the domain. + * Note that this call faiy fail if the underlying virtualization hypervisor + * does not support it or if growing the number is arbitrarily limited. + * This function may require privileged access to the hypervisor. + * + * @flags may include VIR_DOMAIN_AFFECT_LIVE to affect a running + * domain (which may fail if domain is not active), or + * VIR_DOMAIN_AFFECT_CONFIG to affect the next boot via the XML + * description of the domain. Both flags may be set. + * If neither flag is specified (that is, @flags is VIR_DOMAIN_AFFECT_CURRENT), + * then an inactive domain modifies persistent setup, while an active domain + * is hypervisor-dependent on whether just live or both live and persistent + * state is changed. + * + * Not all hypervisors can support all flag combinations. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainSetIOThreads(virDomainPtr domain, + unsigned int niothreads, + unsigned int flags) +{
This seems ok for the use case where we just give a domain a few iothreads and all the disks will share them.
But if you go for one iothread per disk, creating holes in iothread indexes could make sense - we allow unplugging disks from the middle.
I can't think of a case where hotplugging a thread with id=6 would be useful if there are only 3 threads.
Maybe there should be separate APIs for adding and deleting threads?
I have to agree here. Separate API for adding and removing (or perhaps just a parameter to switch?) specific threads is the way to go. Why?
Well this function looks like it was copied from virDomainSetVcpus which I'm currently planning to replace with specific vcpu hotplug api.
The reasons there are the same as Jan pointed out just translated into the context of vCPUS. At the point where there is no other mapping or relationship of belonging the API is fine. This was true for vCPUs in the pre-NUMA era. With NUMA, (or disks IO executed by iothreads in this case) you have a relationship where a thread (or vCPU) belongs to a dis (or NUMA node) and you need to be able to select them individually.
hmmm... well yes, the model is the Vcpu algorithms and specifically SetVcpus. Allowing unordered IOThread id's will require some rework of other algorithms. Talking through this ... If I start with 0 IOThreads and want 2, is it easier to "SetIOThreads" once or "AddIOThread" twice? Similarly, if I have 2 IOThreads and want 0, is it easier to SetIOThreads once or "DelIOThread" twice? If I start with 2 IOThreads, I add 2 IOThreads, then want to remove IOThread 3, then does that mean this has similar logic to Vcpus where I have to introduce a 'current' value. Then at (re)startup, I have to determine which IOThreads are assigned disks in order to determine which of the 3 current to start. The add iothread to disk logic would need adjustment to know that IOThread#3 doesn't exist. It's easy to start one in the middle with its own number, I just wonder the 'real world' use case (not talking the varying array of test matrix patterns that need to be validated). Hence the KISS method of managing IOThreads. If I add an IOThread to a configuration with 3 current and 4 total, I reuse the currently empty entry as the place to add (and current goes away). Same setup, if I added 2 threads, then current goes away, I fill in the 3rd entry, and add a 5th entry. Moving a disk from one IOThread to another is certainly different than moving NUMA nodes and vCPUs around. An IOThread is just a qemu object... a disk is assigned to use that object... In order to stop using that object, the disk needs to be unplugged. Sure we can choose any disk and thus logically should be able to choose any IOThread. But it's still a process to unplug, "unnecessarily" remove IOThread, and hutplug to a different IOThread. How many real world customers will do that? I don't know. My view is someone is more likely to add IOThreads, add their disks to them, and be done. I see removing disks, removing iothreads, etc. as mostly a testing exercise. In some ways I wonder if we're over-engineering this? IOThreads are different than Vcpus, but having a starting place is good. I'm open to ideas and suggestions, but I also am concerned the "rules" become more complex introducing more chances for bugs/errors. John
On Wed, Mar 25, 2015 at 03:22:03PM -0400, John Ferlan wrote:
On 03/25/2015 11:37 AM, Peter Krempa wrote:
On Wed, Mar 25, 2015 at 16:29:22 +0100, Ján Tomko wrote:
On Thu, Mar 19, 2015 at 01:08:22PM -0400, John Ferlan wrote:
Add virDomainIOThreadsSet to allow setting the number of IOThreads for a domain.
The API supports updating both the --live domain and the --config data.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- include/libvirt/libvirt-domain.h | 3 ++ src/driver-hypervisor.h | 6 ++++ src/libvirt-domain.c | 60 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + 4 files changed, 70 insertions(+)
This one has been siting there for a while, I'll think out loud just to get a discussion going.
--- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8042,6 +8042,66 @@ virDomainPinIOThread(virDomainPtr domain,
/** + * virDomainSetIOThreads: + * @domain: pointer to domain object + * @niothreads: the new number of IOThreads for this domain + * @flags: bitwise-OR of virDomainModificationImpact + * + * Dynamically change the number of IOThreads used by the domain. + * Note that this call faiy fail if the underlying virtualization hypervisor + * does not support it or if growing the number is arbitrarily limited. + * This function may require privileged access to the hypervisor. + * + * @flags may include VIR_DOMAIN_AFFECT_LIVE to affect a running + * domain (which may fail if domain is not active), or + * VIR_DOMAIN_AFFECT_CONFIG to affect the next boot via the XML + * description of the domain. Both flags may be set. + * If neither flag is specified (that is, @flags is VIR_DOMAIN_AFFECT_CURRENT), + * then an inactive domain modifies persistent setup, while an active domain + * is hypervisor-dependent on whether just live or both live and persistent + * state is changed. + * + * Not all hypervisors can support all flag combinations. + * + * Returns 0 in case of success, -1 in case of failure. + */ +int +virDomainSetIOThreads(virDomainPtr domain, + unsigned int niothreads, + unsigned int flags) +{
This seems ok for the use case where we just give a domain a few iothreads and all the disks will share them.
But if you go for one iothread per disk, creating holes in iothread indexes could make sense - we allow unplugging disks from the middle.
I can't think of a case where hotplugging a thread with id=6 would be useful if there are only 3 threads.
Maybe there should be separate APIs for adding and deleting threads?
I have to agree here. Separate API for adding and removing (or perhaps just a parameter to switch?) specific threads is the way to go. Why?
Well this function looks like it was copied from virDomainSetVcpus which I'm currently planning to replace with specific vcpu hotplug api.
The reasons there are the same as Jan pointed out just translated into the context of vCPUS. At the point where there is no other mapping or relationship of belonging the API is fine. This was true for vCPUs in the pre-NUMA era. With NUMA, (or disks IO executed by iothreads in this case) you have a relationship where a thread (or vCPU) belongs to a dis (or NUMA node) and you need to be able to select them individually.
hmmm... well yes, the model is the Vcpu algorithms and specifically SetVcpus. Allowing unordered IOThread id's will require some rework of other algorithms.
Talking through this ...
If I start with 0 IOThreads and want 2, is it easier to "SetIOThreads" once or "AddIOThread" twice?
Similarly, if I have 2 IOThreads and want 0, is it easier to SetIOThreads once or "DelIOThread" twice?
If I start with 2 IOThreads, I add 2 IOThreads, then want to remove IOThread 3, then does that mean this has similar logic to Vcpus where I have to introduce a 'current' value. Then at (re)startup, I have to determine which IOThreads are assigned disks in order to determine which of the 3 current to start. The add iothread to disk logic would need adjustment to know that IOThread#3 doesn't exist. It's easy to start one in the middle with its own number, I just wonder the 'real world' use case (not talking the varying array of test matrix patterns that need to be validated). Hence the KISS method of managing IOThreads.
Right, so holes in iothreads don't make much sense if we're specifying them in the domain XML as: <iothreads>n</iothreads> And if we ever need to invent an API that does that, new XML will be needed as well. It was just a thought that came to mind when looking at the iothread_id returned in the virDomainIOThreadInfo struct. Jan
This seems ok for the use case where we just give a domain a few iothreads and all the disks will share them.
But if you go for one iothread per disk, creating holes in iothread indexes could make sense - we allow unplugging disks from the middle.
I can't think of a case where hotplugging a thread with id=6 would be useful if there are only 3 threads.
Maybe there should be separate APIs for adding and deleting threads?
I have to agree here. Separate API for adding and removing (or perhaps just a parameter to switch?) specific threads is the way to go. Why?
Well this function looks like it was copied from virDomainSetVcpus which I'm currently planning to replace with specific vcpu hotplug api.
The reasons there are the same as Jan pointed out just translated into the context of vCPUS. At the point where there is no other mapping or relationship of belonging the API is fine. This was true for vCPUs in the pre-NUMA era. With NUMA, (or disks IO executed by iothreads in this case) you have a relationship where a thread (or vCPU) belongs to a dis (or NUMA node) and you need to be able to select them individually.
Obviously at this point these changes won't make 1.2.14... Would have been nice to have all IOThreads changes in one release, but better to get things right. I think the following is close, but I'm still stuck on how to handle deleting a thread in the middle. Compared to the vCPU algorithm - if you remove a vCPU you're not given the choice of which one to remove. The last one is removed. The problem is how to or whether to represent/save the thread_id array values in the event of a "hole" in the sequence. Sure we could create/save a bitmap of 'iothreads' count entries in use, but is there a 'max' value for that? Technically there's no restriction to the number of IOThreads (theoretically there is ;-)) Here's some rough thoughts for an Add and Del API : * virDomainAddIOThread: * @domain: a domain object * @count: the number of IOThread's to add the domain * @flags: bitwise-OR of virDomainModificationImpact * virDomainDelIOThread: * @domain: a domain object * @iothread_id: the specific IOThread ID value to delete * @flags: bitwise-OR of virDomainModificationImpact A "live" domain has 'n' IOThreads in an iothreads array[]. We "add" to or "refill" the array and adjust the count of iothreads. The refill option would be if we delete in the middle of the array. NB: Deletion will always fail if a disk is assigned to a thread, so discount that. It would be nice if deletion only allowed deletion off the end of the array, but that's not guaranteed, so per When "del"eting entries, things get tricky, especially in the middle. At the end it's easy... The middle just presents problems which I need to think about or get some feedback... Thus... start with 4 threads, live domain: - Add 'n' threads - Search live iothreads array for empty entries to refill - If an entry is found, refill it, reduce 'n' keep going - If none or we reach array size, add 'n' to the live and config count - Del Thr#4 - The live iothreads array entry is zeroed - The live & config number of threads reduced to 3 - Del Thr#1 (or 2 or 3) - The live iothreads array entry is zeroed - The live and config iothreads entry count remains at 4 For an inactive domain with 4 threads - Add 'n' threads - Just adjust iothreads array count - Del 'Thr#4' - Adjust count to 3 - Del 'Thr#1' (or 2 or 3) - Fail OR - If subsequent threads don't have disks assigned, then threads from '#' to end are removed. e.g, Del 3 would then remove 3 & 4... Del 2 would remove 2, 3, & 4. Tough to document, but enforcing that the config always can create 'live' threads of some numbered count without having some sort of hole. Other bright ideas welcome - Tks - John
Add the necessary remote plumbing Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +++++++++++++++- src/remote_protocol-structs | 6 ++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e69f235..fed001c 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8210,6 +8210,7 @@ static virHypervisorDriver hypervisor_driver = { .domainGetMaxVcpus = remoteDomainGetMaxVcpus, /* 0.3.0 */ .domainGetIOThreadsInfo = remoteDomainGetIOThreadsInfo, /* 1.2.14 */ .domainPinIOThread = remoteDomainPinIOThread, /* 1.2.14 */ + .domainSetIOThreads = remoteDomainSetIOThreads, /* 1.2.14 */ .domainGetSecurityLabel = remoteDomainGetSecurityLabel, /* 0.6.1 */ .domainGetSecurityLabelList = remoteDomainGetSecurityLabelList, /* 0.10.0 */ .nodeGetSecurityModel = remoteNodeGetSecurityModel, /* 0.6.1 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index eb862e1..19282ed 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1212,6 +1212,12 @@ struct remote_domain_pin_iothread_args { unsigned int flags; }; +struct remote_domain_set_iothreads_args { + remote_nonnull_domain dom; + unsigned int niothreads; + unsigned int flags; +}; + struct remote_domain_get_security_label_args { remote_nonnull_domain dom; }; @@ -5643,5 +5649,13 @@ enum remote_procedure { * @generate: none * @acl: domain:read */ - REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES = 353 + REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES = 353, + + /** + * @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_IOTHREADS = 354 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index b3e2e40..e046076 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -834,6 +834,11 @@ struct remote_domain_pin_iothread_args { } cpumap; u_int flags; }; +struct remote_domain_set_iothreads_args { + remote_nonnull_domain dom; + u_int niothreads; + u_int flags; +}; struct remote_domain_get_security_label_args { remote_nonnull_domain dom; }; @@ -3017,4 +3022,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_IOTHREADS_INFO = 351, REMOTE_PROC_DOMAIN_PIN_IOTHREAD = 352, REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES = 353, + REMOTE_PROC_DOMAIN_SET_IOTHREADS = 354, }; -- 2.1.0
Impending IOThread setting patches would copy the code anyway, so create and generalize the add the vcpu to a cgroup into its own API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 69 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6d9217b..b7ddca3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4630,9 +4630,49 @@ static void qemuProcessEventHandler(void *data, void *opaque) VIR_FREE(processEvent); } -static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, - virDomainObjPtr vm, - unsigned int nvcpus) +typedef int cgroupNewFunc(virCgroupPtr domain, + int id, + bool create, + virCgroupPtr *group); + +static virCgroupPtr +qemuDomainHotplugAddCgroup(virCgroupPtr cgroup, + cgroupNewFunc func, + int index, + char *mem_mask, + pid_t pid) +{ + virCgroupPtr new_cgroup = NULL; + int rv = -1; + + /* Create cgroup */ + if (func(cgroup, index, true, &new_cgroup) < 0) + return NULL; + + if (mem_mask && virCgroupSetCpusetMems(new_cgroup, mem_mask) < 0) + goto error; + + /* Add pid/thread to the cgroup */ + rv = virCgroupAddTask(new_cgroup, pid); + if (rv < 0) { + virReportSystemError(-rv, + _("unable to add id %d task %d to cgroup"), + index, pid); + virCgroupRemove(new_cgroup); + goto error; + } + + return new_cgroup; + + error: + virCgroupFree(&new_cgroup); + return NULL; +} + +static int +qemuDomainHotplugVcpus(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int nvcpus) { qemuDomainObjPrivatePtr priv = vm->privateData; size_t i; @@ -4721,25 +4761,12 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, if (nvcpus > oldvcpus) { for (i = oldvcpus; i < nvcpus; i++) { if (priv->cgroup) { - int rv = -1; - /* Create cgroup for the onlined vcpu */ - if (virCgroupNewVcpu(priv->cgroup, i, true, &cgroup_vcpu) < 0) + cgroup_vcpu = qemuDomainHotplugAddCgroup(priv->cgroup, + virCgroupNewVcpu, + i, mem_mask, + cpupids[i]); + if (!cgroup_vcpu) goto cleanup; - - if (mem_mask && - virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0) - goto cleanup; - - /* Add vcpu thread to the cgroup */ - rv = virCgroupAddTask(cgroup_vcpu, cpupids[i]); - if (rv < 0) { - virReportSystemError(-rv, - _("unable to add vcpu %zu task %d to cgroup"), - i, cpupids[i]); - virCgroupRemove(cgroup_vcpu); - goto cleanup; - } - } /* Inherit def->cpuset */ -- 2.1.0
Impending IOThread setting patches would copy the code anyway, so create and generalize a delete cgroup and pindef for the vcpu into its own API. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b7ddca3..673b95e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4670,6 +4670,30 @@ qemuDomainHotplugAddCgroup(virCgroupPtr cgroup, } static int +qemuDomainHotplugDelCgroupPin(virCgroupPtr cgroup, + cgroupNewFunc func, + int index, + virDomainPinDefPtr **pindef_list, + size_t *npin) +{ + virCgroupPtr new_cgroup = NULL; + + if (cgroup) { + if (func(cgroup, index, false, &new_cgroup) < 0) + return -1; + + /* Remove the offlined cgroup */ + virCgroupRemove(new_cgroup); + virCgroupFree(&new_cgroup); + } + + /* Free pindef setting */ + virDomainPinDel(pindef_list, npin, index); + + return 0; +} + +static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver, virDomainObjPtr vm, unsigned int nvcpus) @@ -4825,19 +4849,11 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver, } } else { for (i = oldvcpus - 1; i >= nvcpus; i--) { - if (priv->cgroup) { - if (virCgroupNewVcpu(priv->cgroup, i, false, &cgroup_vcpu) < 0) - goto cleanup; - - /* Remove cgroup for the offlined vcpu */ - virCgroupRemove(cgroup_vcpu); - virCgroupFree(&cgroup_vcpu); - } + if (qemuDomainHotplugDelCgroupPin(priv->cgroup, virCgroupNewVcpu, i, + &vm->def->cputune.vcpupin, + &vm->def->cputune.nvcpupin) < 0) + goto cleanup; - /* Free vcpupin setting */ - virDomainPinDel(&vm->def->cputune.vcpupin, - &vm->def->cputune.nvcpupin, - i); } } -- 2.1.0
Impending IOThread setting patches would copy the code anyway, so create and generalize the adding of pindef for the vcpu into its own API Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 104 +++++++++++++++++++++++++++++-------------------- 1 file changed, 62 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 673b95e..1fca43c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4669,6 +4669,63 @@ qemuDomainHotplugAddCgroup(virCgroupPtr cgroup, return NULL; } +typedef int cgroupSetupFunc(virCgroupPtr cgroup, + virDomainPinDefPtr *pindef, + int npin, + int id); + +static int +qemuDomainHotplugAddPin(virBitmapPtr cpumask, + int index, + pid_t pid, + virDomainPinDefPtr **pindef_list, + size_t *npin, + cgroupSetupFunc func, + virCgroupPtr cgroup) +{ + int ret = -1; + virDomainPinDefPtr pindef = NULL; + + /* vm->def->cputune.* arrays can't be NULL if + * vm->def->cpumask is not NULL. + */ + if (VIR_ALLOC(pindef) < 0) + goto cleanup; + + if (!(pindef->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) { + VIR_FREE(pindef); + goto cleanup; + } + virBitmapCopy(pindef->cpumask, cpumask); + pindef->id = index; + if (VIR_APPEND_ELEMENT_COPY(*pindef_list, *npin, pindef) < 0) { + virBitmapFree(pindef->cpumask); + VIR_FREE(pindef); + goto cleanup; + } + + if (cgroup) { + if (func(cgroup, *pindef_list, *npin, index) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("failed to set cpuset.cpus in cgroup for id %d"), + index); + goto cleanup; + } + } else { + if (virProcessSetAffinity(pid, pindef->cpumask) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to set cpu affinity for id %d"), + index); + goto cleanup; + } + } + + ret = 0; + + cleanup: + return ret; +} + static int qemuDomainHotplugDelCgroupPin(virCgroupPtr cgroup, cgroupNewFunc func, @@ -4795,51 +4852,14 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver, /* Inherit def->cpuset */ if (vm->def->cpumask) { - /* vm->def->cputune.vcpupin can't be NULL if - * vm->def->cpumask is not NULL. - */ - virDomainPinDefPtr vcpupin = NULL; - - if (VIR_ALLOC(vcpupin) < 0) - goto cleanup; - - if (!(vcpupin->cpumask = - virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) { - VIR_FREE(vcpupin); - goto cleanup; - } - virBitmapCopy(vcpupin->cpumask, vm->def->cpumask); - vcpupin->id = i; - if (VIR_APPEND_ELEMENT_COPY(vm->def->cputune.vcpupin, - vm->def->cputune.nvcpupin, vcpupin) < 0) { - virBitmapFree(vcpupin->cpumask); - VIR_FREE(vcpupin); + if (qemuDomainHotplugAddPin(vm->def->cpumask, i, cpupids[i], + &vm->def->cputune.vcpupin, + &vm->def->cputune.nvcpupin, + qemuSetupCgroupVcpuPin, + cgroup_vcpu) < 0) ret = -1; goto cleanup; - } - - if (cgroup_vcpu) { - if (qemuSetupCgroupVcpuPin(cgroup_vcpu, - vm->def->cputune.vcpupin, - vm->def->cputune.nvcpupin, i) < 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("failed to set cpuset.cpus in cgroup" - " for vcpu %zu"), i); - ret = -1; - goto cleanup; - } - } else { - if (virProcessSetAffinity(cpupids[i], - vcpupin->cpumask) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("failed to set cpu affinity for vcpu %zu"), - i); - ret = -1; - goto cleanup; - } - } } - virCgroupFree(&cgroup_vcpu); if (qemuProcessSetSchedParams(i, cpupids[i], -- 2.1.0
On 03/19/2015 01:08 PM, John Ferlan wrote:
Impending IOThread setting patches would copy the code anyway, so create and generalize the adding of pindef for the vcpu into its own API
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 104 +++++++++++++++++++++++++++++-------------------- 1 file changed, 62 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 673b95e..1fca43c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4669,6 +4669,63 @@ qemuDomainHotplugAddCgroup(virCgroupPtr cgroup, return NULL; }
+typedef int cgroupSetupFunc(virCgroupPtr cgroup, + virDomainPinDefPtr *pindef, + int npin, + int id); + +static int +qemuDomainHotplugAddPin(virBitmapPtr cpumask, + int index, + pid_t pid, + virDomainPinDefPtr **pindef_list, + size_t *npin, + cgroupSetupFunc func, + virCgroupPtr cgroup) +{ + int ret = -1; + virDomainPinDefPtr pindef = NULL; + + /* vm->def->cputune.* arrays can't be NULL if + * vm->def->cpumask is not NULL. + */ + if (VIR_ALLOC(pindef) < 0) + goto cleanup; + + if (!(pindef->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) { + VIR_FREE(pindef); + goto cleanup; + } + virBitmapCopy(pindef->cpumask, cpumask); + pindef->id = index; + if (VIR_APPEND_ELEMENT_COPY(*pindef_list, *npin, pindef) < 0) { + virBitmapFree(pindef->cpumask); + VIR_FREE(pindef); + goto cleanup; + } + + if (cgroup) { + if (func(cgroup, *pindef_list, *npin, index) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("failed to set cpuset.cpus in cgroup for id %d"), + index); + goto cleanup; + } + } else { + if (virProcessSetAffinity(pid, pindef->cpumask) < 0) { + virReportError(VIR_ERR_SYSTEM_ERROR, + _("failed to set cpu affinity for id %d"), + index); + goto cleanup; + } + } + + ret = 0; + + cleanup: + return ret; +} + static int qemuDomainHotplugDelCgroupPin(virCgroupPtr cgroup, cgroupNewFunc func, @@ -4795,51 +4852,14 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
/* Inherit def->cpuset */ if (vm->def->cpumask) { - /* vm->def->cputune.vcpupin can't be NULL if - * vm->def->cpumask is not NULL. - */ - virDomainPinDefPtr vcpupin = NULL; - - if (VIR_ALLOC(vcpupin) < 0) - goto cleanup; - - if (!(vcpupin->cpumask = - virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) { - VIR_FREE(vcpupin); - goto cleanup; - } - virBitmapCopy(vcpupin->cpumask, vm->def->cpumask); - vcpupin->id = i; - if (VIR_APPEND_ELEMENT_COPY(vm->def->cputune.vcpupin, - vm->def->cputune.nvcpupin, vcpupin) < 0) { - virBitmapFree(vcpupin->cpumask); - VIR_FREE(vcpupin); + if (qemuDomainHotplugAddPin(vm->def->cpumask, i, cpupids[i], + &vm->def->cputune.vcpupin, + &vm->def->cputune.nvcpupin, + qemuSetupCgroupVcpuPin, + cgroup_vcpu) < 0) ret = -1; goto cleanup;
Ran my changes againt Coverity - looks like I missed something right here - consider the following squashed in: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1fca43c..496a4af 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4856,9 +4856,10 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver, &vm->def->cputune.vcpupin, &vm->def->cputune.nvcpupin, qemuSetupCgroupVcpuPin, - cgroup_vcpu) < 0) + cgroup_vcpu) < 0) { ret = -1; goto cleanup; + } } virCgroupFree(&cgroup_vcpu);
- } - - if (cgroup_vcpu) { - if (qemuSetupCgroupVcpuPin(cgroup_vcpu, - vm->def->cputune.vcpupin, - vm->def->cputune.nvcpupin, i) < 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("failed to set cpuset.cpus in cgroup" - " for vcpu %zu"), i); - ret = -1; - goto cleanup; - } - } else { - if (virProcessSetAffinity(cpupids[i], - vcpupin->cpumask) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("failed to set cpu affinity for vcpu %zu"), - i); - ret = -1; - goto cleanup; - } - } } - virCgroupFree(&cgroup_vcpu);
if (qemuProcessSetSchedParams(i, cpupids[i],
Add qemuDomainSetIOThreads in order to adjust the live and config IOThread values for the domain similar to the qemuDomainSetVcpus implementation. The qemuDomainHotplugIOThread will more or less mirror qemuDomainHotplugVcpus including usage of the recently extracted API's to manage the cgroup and adding pinning if the domain has a cpuset. Unlike the Vcpu code, IOThreads are implemented by qmp objects. Forcing the HotplugIOThread code to add/delete one at a time to/from the end of the list rather than attempting to batch add/delete more than one and attempting to manage/determine which failed leaving potential holes unsequenced id numbers. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_audit.c | 9 ++ src/conf/domain_audit.h | 6 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 331 +++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 347 insertions(+) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 159ebf5..92fcdd3 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -790,6 +790,15 @@ virDomainAuditVcpu(virDomainObjPtr vm, return virDomainAuditResource(vm, "vcpu", oldvcpu, newvcpu, reason, success); } +void +virDomainAuditIOThread(virDomainObjPtr vm, + unsigned int oldiothread, unsigned int newiothread, + const char *reason, bool success) +{ + return virDomainAuditResource(vm, "iothread", oldiothread, newiothread, + reason, success); +} + static void virDomainAuditLifecycle(virDomainObjPtr vm, const char *op, const char *reason, bool success) diff --git a/src/conf/domain_audit.h b/src/conf/domain_audit.h index 4c1ef90..97dadca 100644 --- a/src/conf/domain_audit.h +++ b/src/conf/domain_audit.h @@ -102,6 +102,12 @@ void virDomainAuditVcpu(virDomainObjPtr vm, const char *reason, bool success) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); +void virDomainAuditIOThread(virDomainObjPtr vm, + unsigned int oldiothread, + unsigned int newiothread, + const char *reason, + bool success) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); void virDomainAuditSecurityLabel(virDomainObjPtr vm, bool success) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ca3520d..0d09c23 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -119,6 +119,7 @@ virDomainAuditDisk; virDomainAuditFS; virDomainAuditHostdev; virDomainAuditInit; +virDomainAuditIOThread; virDomainAuditMemory; virDomainAuditNet; virDomainAuditNetDevice; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1fca43c..dbd7ff5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6182,6 +6182,336 @@ qemuDomainPinIOThread(virDomainPtr dom, return ret; } +static int +qemuDomainHotplugIOThread(virQEMUDriverPtr driver, + virDomainObjPtr vm, + unsigned int req_niothreads) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *alias = NULL; + size_t i; + int rc = -1; + int ret = -1; + unsigned int old_niothreads = vm->def->iothreads; + int new_niothreads = 0; + qemuMonitorIOThreadsInfoPtr *new_iothreads = NULL; + virCgroupPtr cgroup_iothread = NULL; + char *mem_mask = NULL; + + qemuDomainObjEnterMonitor(driver, vm); + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change IOThreads for an inactive domain")); + goto exit_monitor; + } + + /* Only allow to hot add or remove one IOThread at a time forcing + * the caller to "do the right thing" especially with respect to + * deletion since we're dealing with unsigned int math. Getting + * this error probably proves the caller used some bad math. + */ + if (req_niothreads > old_niothreads + 1 || + (old_niothreads && req_niothreads < old_niothreads - 1)) { + virReportError(VIR_ERR_INVALID_ARG, + _("only allowed to hot add or remove one IOThread " + "at a time for the domain, current count is: %d"), + old_niothreads); + goto exit_monitor; + } + + if (req_niothreads > old_niothreads) { + /* Adding to end - new alias is req_niothreads */ + if (virAsprintf(&alias, "iothread%u", req_niothreads) < 0) + goto exit_monitor; + + rc = qemuMonitorAddObject(priv->mon, "iothread", alias, NULL); + } else { + /* Removing from end - alias is current old_niothreads */ + if (virAsprintf(&alias, "iothread%u", old_niothreads) < 0) + goto exit_monitor; + + rc = qemuMonitorDelObject(priv->mon, alias); + } + + if (rc < 0) + goto exit_monitor; + + /* Although conceptually this could be done later - at this point, + * the running VM either has a new IOThread or has one removed. + * Since decisions depend upon knowing how many threads are in the + * running vm, adjusting this now that we've theoretically added or + * removed a thread. + */ + if (virDomainObjIsActive(vm)) + vm->def->iothreads = req_niothreads; + + /* After hotplugging the IOThreads we need to re-detect the + * IOThreads thread_id's, adjust the cgroups, thread affinity, + * and the priv->iothreadpids list. + */ + if ((new_niothreads = qemuMonitorGetIOThreads(priv->mon, + &new_iothreads)) < 0) { + virResetLastError(); + goto exit_monitor; + } + + /* ohhh something went wrong */ + if (new_niothreads != req_niothreads) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("got wrong number of IOThread ids from QEMU monitor. " + "got %d, wanted %d"), + new_niothreads, req_niothreads); + + /* Let's set this too - just to be correct */ + vm->def->iothreads = new_niothreads; + goto exit_monitor; + } + + if (req_niothreads > old_niothreads) { + /* Add our thread_id to the list of thread id's */ + if (VIR_REALLOC_N(priv->iothreadpids, priv->niothreadpids + 1) < 0) + goto exit_monitor; + priv->iothreadpids[priv->niothreadpids] = + new_iothreads[new_niothreads - 1]->thread_id; + priv->niothreadpids++; + } else { + /* Remove our thread_id from the list of thread id's */ + priv->iothreadpids[priv->niothreadpids - 1] = 0; + if (VIR_REALLOC_N(priv->iothreadpids, priv->niothreadpids - 1) < 0) + goto exit_monitor; + priv->niothreadpids--; + } + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if (virDomainNumatuneGetMode(vm->def->numa, -1) == + VIR_DOMAIN_NUMATUNE_MEM_STRICT && + virDomainNumatuneMaybeFormatNodeset(vm->def->numa, + priv->autoNodeset, + &mem_mask, -1) < 0) + goto cleanup; + + /* Add */ + if (req_niothreads > old_niothreads) { + unsigned int thread_id = new_iothreads[new_niothreads - 1]->thread_id; + + /* Add IOThread to cgroup if present */ + if (priv->cgroup) { + cgroup_iothread = qemuDomainHotplugAddCgroup(priv->cgroup, + virCgroupNewIOThread, + new_niothreads, + mem_mask, + thread_id); + if (!cgroup_iothread) + goto cleanup; + } + + /* Inherit def->cpuset */ + if (vm->def->cpumask) { + if (qemuDomainHotplugAddPin(vm->def->cpumask, + new_niothreads, + thread_id, + &vm->def->cputune.iothreadspin, + &vm->def->cputune.niothreadspin, + qemuSetupCgroupIOThreadsPin, + cgroup_iothread) < 0) + goto cleanup; + } + } else { /* Delete */ + /* Remove the cgroup and pin related links */ + if (qemuDomainHotplugDelCgroupPin(priv->cgroup, + virCgroupNewIOThread, + old_niothreads, + &vm->def->cputune.iothreadspin, + &vm->def->cputune.niothreadspin) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + if (new_iothreads) { + for (i = 0; i < new_niothreads; i++) + qemuMonitorIOThreadsInfoFree(new_iothreads[i]); + VIR_FREE(new_iothreads); + } + VIR_FREE(mem_mask); + virDomainAuditIOThread(vm, old_niothreads, req_niothreads, + "update", rc == 0); + if (cgroup_iothread) + virCgroupFree(&cgroup_iothread); + VIR_FREE(alias); + return ret; + + exit_monitor: + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; +} + +static int +qemuDomainSetIOThreads(virDomainPtr dom, + unsigned int niothreads, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + virQEMUDriverConfigPtr cfg = NULL; + virCapsPtr caps = NULL; + qemuDomainObjPrivatePtr priv; + virCgroupPtr cgroup_temp = NULL; + virBitmapPtr all_nodes = NULL; + char *all_nodes_str = NULL; + char *mem_mask = NULL; + virDomainDefPtr persistentDef; + size_t i, j; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + if ((unsigned short) niothreads != niothreads) { + virReportError(VIR_ERR_INVALID_ARG, + _("argument out of range: %d"), niothreads); + return -1; + } + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + cfg = virQEMUDriverGetConfig(driver); + + if (virDomainSetIOThreadsEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + goto cleanup; + + /* Same value? Nothing to do */ + if (niothreads == vm->def->iothreads) { + ret = 0; + goto cleanup; + } + + priv = vm->privateData; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0) + goto endjob; + + if (!(all_nodes = virNumaGetHostNodeset())) + goto endjob; + + if (!(all_nodes_str = virBitmapFormat(all_nodes))) + goto endjob; + + if (virCgroupGetCpusetMems(cgroup_temp, &mem_mask) < 0 || + virCgroupSetCpusetMems(cgroup_temp, all_nodes_str) < 0) + goto endjob; + } + + if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) + goto endjob; + + /* For a live change - let's make sure the binary supports this */ + if (flags & VIR_DOMAIN_AFFECT_LIVE && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported with this binary")); + goto endjob; + } + + /* If removing and there is a disk using the IOThread(s) to be + * removed, then fail (both live and config). We need the dual + * loop since more than one IOThread can be removed at a time and + * we need to check all that are being removed. Although this is + * unsigned arithmetic 'i' cannot get less than zero since the code + * is comparing the 'id' being used which is defined as 1..n + */ + if (niothreads < vm->def->iothreads) { + for (i = vm->def->iothreads; i > niothreads; i--) { + for (j = 0; j < vm->def->ndisks; j++) { + if (vm->def->disks[j]->iothread == i) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot remove IOThread %zu since it " + "is being used by disk path '%s'"), + i, NULLSTR(vm->def->disks[j]->src->path)); + goto endjob; + } + } + } + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + /* Hot plug/unplug one at a time - incrementally when adding + * and decrementally when removing - this avoids "gaps" in + * the sequencing of the list if there's an error especially + * during the removal processing + */ + if (niothreads > vm->def->iothreads) { + for (i = vm->def->iothreads + 1; i <= niothreads; i++) + if (qemuDomainHotplugIOThread(driver, vm, i) < 0) + goto endjob; + } else { + /* NB: vm->def->iothreads is unsigned - so if we're going to + * zero IOThreads, then we cannot use "just" - 1 math for 'i' + */ + for (i = vm->def->iothreads - 1; + i >= niothreads && vm->def->iothreads; i--) + if (qemuDomainHotplugIOThread(driver, vm, i) < 0) + goto endjob; + } + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + /* Remove iothreadpin entries for iothreads that were unplugged. + * We take from the end just in case there's an error "in the middle" + * which could leave id's such as 1, 2, 4. If we added a new iothread + * it would also get id 4, which wouldn't be good + */ + if (niothreads < persistentDef->iothreads) { + for (i = persistentDef->iothreads; i > niothreads; i--) + virDomainPinDel(&persistentDef->cputune.iothreadspin, + &persistentDef->cputune.niothreadspin, + i); + } + + persistentDef->iothreads = niothreads; + if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0) + goto endjob; + } + + ret = 0; + + endjob: + if (mem_mask) { + virErrorPtr err = virSaveLastError(); + virCgroupSetCpusetMems(cgroup_temp, mem_mask); + virSetError(err); + virFreeError(err); + } + + qemuDomainObjEndJob(driver, vm); + + cleanup: + qemuDomObjEndAPI(&vm); + VIR_FREE(mem_mask); + VIR_FREE(all_nodes_str); + virBitmapFree(all_nodes); + virCgroupFree(&cgroup_temp); + virObjectUnref(caps); + virObjectUnref(cfg); + return ret; +} + static int qemuDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr seclabel) { virQEMUDriverPtr driver = dom->conn->privateData; @@ -19868,6 +20198,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetMaxVcpus = qemuDomainGetMaxVcpus, /* 0.4.4 */ .domainGetIOThreadsInfo = qemuDomainGetIOThreadsInfo, /* 1.2.14 */ .domainPinIOThread = qemuDomainPinIOThread, /* 1.2.14 */ + .domainSetIOThreads = qemuDomainSetIOThreads, /* 1.2.14 */ .domainGetSecurityLabel = qemuDomainGetSecurityLabel, /* 0.6.1 */ .domainGetSecurityLabelList = qemuDomainGetSecurityLabelList, /* 0.10.0 */ .nodeGetSecurityModel = qemuNodeGetSecurityModel, /* 0.6.1 */ -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1161617 Add command to allow adding and removing IOThreads from the domain including the configuration and live domain. $ virsh setiothreads --help NAME setiothreads - change number of IOThreads SYNOPSIS setiothreads <domain> <count> [--config] [--live] [--current] DESCRIPTION Change the number of IOThreads in the guest domain. OPTIONS [--domain] <string> domain name, id or uuid [--count] <number> number of IOThreads --config affect next boot --live affect running domain --current affect current domain Assuming a domain with multiple IOThreads assigned to various threads and trying to remove an IOThread assigned to a disk resource results in an error on removal attempt: $ virsh iothreadsinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 2 2 3 3 0-3 $ virsh setiothreads $dom 1 error: invalid argument: cannot remove IOThread 2 since it is being used by disk path '/home/vm-images/iothr-vol1' Adding an IOThread to a domain and then removing it: $ virsh iothreadsinfo $dom No IOThreads found for the domain $ virsh setiothreads $dom 1 $ virsh iothreadsinfo $dom IOThread ID CPU Affinity --------------------------------------------------- 1 0-3 $ virsh setiothreads $dom 0 $ virsh iothreadsinfo $dom No IOThreads found for the domain $ Invalid number of IOThreads provided: $ virsh setiothreads $dom -1 error: Invalid number of IOThreads $ Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 17 +++++++++++ 2 files changed, 99 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1d8225c..69d2a94 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6978,6 +6978,82 @@ cmdIOThreadPin(vshControl *ctl, const vshCmd *cmd) } /* + * "setiothreads" command + */ +static const vshCmdInfo info_setiothreads[] = { + {.name = "help", + .data = N_("change number of IOThreads") + }, + {.name = "desc", + .data = N_("Change the number of IOThreads in the guest domain.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_setiothreads[] = { + {.name = "domain", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("domain name, id or uuid") + }, + {.name = "count", + .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ, + .help = N_("number of IOThreads") + }, + {.name = "config", + .type = VSH_OT_BOOL, + .help = N_("affect next boot") + }, + {.name = "live", + .type = VSH_OT_BOOL, + .help = N_("affect running domain") + }, + {.name = "current", + .type = VSH_OT_BOOL, + .help = N_("affect current domain") + }, + {.name = NULL} +}; + +static bool +cmdSetIOThreads(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + int count = 0; + bool ret = false; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptInt(cmd, "count", &count) < 0 || count < 0) { + vshError(ctl, "%s", _("Invalid number of IOThreads")); + goto cleanup; + } + + if (virDomainSetIOThreads(dom, count, flags) < 0) + goto cleanup; + + ret = true; + + cleanup: + virDomainFree(dom); + return ret; +} + +/* * "cpu-compare" command */ static const vshCmdInfo info_cpu_compare[] = { @@ -12903,6 +12979,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_iothreadpin, .flags = 0 }, + {.name = "setiothreads", + .handler = cmdSetIOThreads, + .opts = opts_setiothreads, + .info = info_setiothreads, + .flags = 0 + }, {.name = "send-key", .handler = cmdSendKey, .opts = opts_send_key, diff --git a/tools/virsh.pod b/tools/virsh.pod index 8262a45..3376e49 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1416,6 +1416,23 @@ If no flag is specified, behavior is different depending on hypervisor. B<Note>: The expression is sequentially evaluated, so "0-15,^8" is identical to "9-14,0-7,15" but not identical to "^8,0-15". +=item B<setiothreads> I<domain> I<count> +[[I<--config>] [I<--live>] | [I<--current>]] + +Change the number of IOThreads in a guest domain. The I<count> value may be +limited by host or hypervisor. A value of zero removes all IOThreads from +the guest domain. If an IOThread is currently assigned to a disk resource +such as via the B<attach-disk> command, then an attempt to remove the +IOThread will fail. + +If I<--live> is specified, affect a running guest. If the guest is not +running an error is returned. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified or I<--live> and I<--config> are not specified, +affect the current guest state. +Both I<--live> and I<--config> flags may be given, but if not flag is +specified, behavior is dfferent depending on hypervisor. + =item B<managedsave> I<domain> [I<--bypass-cache>] [{I<--running> | I<--paused>}] [I<--verbose>] -- 2.1.0
On 03/19/2015 01:08 PM, John Ferlan wrote:
These patches will implement the Set IOThreads command to complete the IOThreads family of calls/settings. There are 3 patches in the middle (3, 4, & 5) which are just refactoring the SetVcpus code so that the SetIOThreads code can make use of the same functionality rather than cut-n-paste
John Ferlan (7): Implement public API for virDomainIOThreadsSet remote: Plumbing for virDomainSetIOThreads qemu: qemuDomainHotplugVcpus - separate out the add cgroup qemu: qemuDomainHotplugVcpus - separate out the del cgroup and pin qemu: qemuDomainHotplugVcpus - separate out pin adjustment code qemu: Add support to set IOThreads count virsh: Add setiothreads command
include/libvirt/libvirt-domain.h | 3 + src/conf/domain_audit.c | 9 + src/conf/domain_audit.h | 6 + src/driver-hypervisor.h | 6 + src/libvirt-domain.c | 60 +++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 1 + src/qemu/qemu_driver.c | 544 +++++++++++++++++++++++++++++++++------ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +- src/remote_protocol-structs | 6 + tools/virsh-domain.c | 82 ++++++ tools/virsh.pod | 17 ++ 13 files changed, 676 insertions(+), 76 deletions(-)
While we work through the other parts of this - can patches 3-5 be reviewed separately as regardless of the implementation they'll be useful for IOThreads too. Tks - John
participants (3)
-
John Ferlan -
Ján Tomko -
Peter Krempa