[libvirt] [PATCH 0/4] Fix a couple of iothread bugs
Details in the patches. John Ferlan (4): qemu: During startup need more QEMU_CAPS_OBJECT_IOTHREAD checks qemu: Check QEMU_CAPS_OBJECT_IOTHREAD for qemuDomainPinIOThread conf: Refactor the iothreadid initialization conf: Optimize the iothreadid initialization src/conf/domain_conf.c | 83 +++++++++++++++++++++++++++++++++++++++---------- src/qemu/qemu_cgroup.c | 3 ++ src/qemu/qemu_driver.c | 6 ++++ src/qemu/qemu_process.c | 18 +++++++---- 4 files changed, 88 insertions(+), 22 deletions(-) -- 2.1.0
During qemu process startup (qemuProcessStart), the call to qemuProcessDetectIOThreadPIDs will not attempt to fill in the ->thread_id values if the binary doesn't support IOThreads. However, subsequent calls to setup the IOThread cgroups, affinity, and scheduler parameters had no such check, thus could attempt to set thread_id = 0, which would not be a good idea. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 3 +++ src/qemu/qemu_process.c | 18 ++++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 570dab5..66318ea 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1163,6 +1163,9 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) char *mem_mask = NULL; virDomainNumatuneMemMode mem_mode; + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0; + if ((period || quota) && !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8aa9efc..67e7cbc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2514,10 +2514,14 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) static int qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) { + qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; size_t i; int ret = -1; + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0; + for (i = 0; i < def->niothreadids; i++) { /* set affinity only for existing iothreads */ if (!def->iothreadids[i]->cpumask) @@ -2574,12 +2578,14 @@ qemuProcessSetSchedulers(virDomainObjPtr vm) return -1; } - for (i = 0; i < vm->def->niothreadids; i++) { - if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, - vm->def->iothreadids[i]->thread_id, - vm->def->cputune.niothreadsched, - vm->def->cputune.iothreadsched) < 0) - return -1; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + for (i = 0; i < vm->def->niothreadids; i++) { + if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, + vm->def->iothreadids[i]->thread_id, + vm->def->cputune.niothreadsched, + vm->def->cputune.iothreadsched) < 0) + return -1; + } } return 0; -- 2.1.0
On Tue, Oct 13, 2015 at 11:47:07 -0400, John Ferlan wrote:
During qemu process startup (qemuProcessStart), the call to qemuProcessDetectIOThreadPIDs will not attempt to fill in the ->thread_id values if the binary doesn't support IOThreads. However, subsequent calls to setup the IOThread cgroups, affinity, and scheduler parameters had no such check, thus could attempt to set thread_id = 0, which would not be a good idea.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 3 +++ src/qemu/qemu_process.c | 18 ++++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-)
Do we even allow to start a VM that has IOthreads enabled but qemu does not support them? In that case that's the point to fix so that we have a central point where it's checked.
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 570dab5..66318ea 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1163,6 +1163,9 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) char *mem_mask = NULL; virDomainNumatuneMemMode mem_mode;
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0;
Spreading checks like this through the code usually ends up to be fragile. I think this should be if (def->niothreads == 0) return 0;
+ if ((period || quota) && !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8aa9efc..67e7cbc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2514,10 +2514,14 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) static int qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) { + qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; size_t i; int ret = -1;
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0; + for (i = 0; i < def->niothreadids; i++) {
Here this function is doing the right thing.
/* set affinity only for existing iothreads */ if (!def->iothreadids[i]->cpumask) @@ -2574,12 +2578,14 @@ qemuProcessSetSchedulers(virDomainObjPtr vm) return -1; }
- for (i = 0; i < vm->def->niothreadids; i++) { - if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, - vm->def->iothreadids[i]->thread_id, - vm->def->cputune.niothreadsched, - vm->def->cputune.iothreadsched) < 0) - return -1; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + for (i = 0; i < vm->def->niothreadids; i++) {
As well as here.
+ if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, + vm->def->iothreadids[i]->thread_id, + vm->def->cputune.niothreadsched, + vm->def->cputune.iothreadsched) < 0) + return -1; + }
Peter
On 10/13/2015 12:08 PM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 11:47:07 -0400, John Ferlan wrote:
During qemu process startup (qemuProcessStart), the call to qemuProcessDetectIOThreadPIDs will not attempt to fill in the ->thread_id values if the binary doesn't support IOThreads. However, subsequent calls to setup the IOThread cgroups, affinity, and scheduler parameters had no such check, thus could attempt to set thread_id = 0, which would not be a good idea.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 3 +++ src/qemu/qemu_process.c | 18 ++++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-)
Do we even allow to start a VM that has IOthreads enabled but qemu does not support them? In that case that's the point to fix so that we have a central point where it's checked.
hmm... I guess these changes were more CYA than anything else... Look for any callers to the API's that were doing actions based on niothreadids and make sure we add the caps check. Blind ambition. I'll adjust this patch to be just the if (def->niothreads == 0) return 0; in qemuSetupCgroupForIOThreads John
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 570dab5..66318ea 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1163,6 +1163,9 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) char *mem_mask = NULL; virDomainNumatuneMemMode mem_mode;
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0;
Spreading checks like this through the code usually ends up to be fragile.
I think this should be if (def->niothreads == 0) return 0;
+ if ((period || quota) && !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8aa9efc..67e7cbc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2514,10 +2514,14 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) static int qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) { + qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; size_t i; int ret = -1;
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0; + for (i = 0; i < def->niothreadids; i++) {
Here this function is doing the right thing.
/* set affinity only for existing iothreads */ if (!def->iothreadids[i]->cpumask) @@ -2574,12 +2578,14 @@ qemuProcessSetSchedulers(virDomainObjPtr vm) return -1; }
- for (i = 0; i < vm->def->niothreadids; i++) { - if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, - vm->def->iothreadids[i]->thread_id, - vm->def->cputune.niothreadsched, - vm->def->cputune.iothreadsched) < 0) - return -1; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + for (i = 0; i < vm->def->niothreadids; i++) {
As well as here.
+ if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, + vm->def->iothreadids[i]->thread_id, + vm->def->cputune.niothreadsched, + vm->def->cputune.iothreadsched) < 0) + return -1; + }
Peter
On 10/13/2015 12:08 PM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 11:47:07 -0400, John Ferlan wrote:
During qemu process startup (qemuProcessStart), the call to qemuProcessDetectIOThreadPIDs will not attempt to fill in the ->thread_id values if the binary doesn't support IOThreads. However, subsequent calls to setup the IOThread cgroups, affinity, and scheduler parameters had no such check, thus could attempt to set thread_id = 0, which would not be a good idea.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 3 +++ src/qemu/qemu_process.c | 18 ++++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-)
Do we even allow to start a VM that has IOthreads enabled but qemu does not support them? In that case that's the point to fix so that we have a central point where it's checked.
Upon further reflection... During qemuBuildCommandLine there is a check for 'iothreads > 0 and have the capability', then create 'niothreadids' objects. Then during qemuCheckIOThreads called from qemuBuildDriveDevStr, we will fail if there's a disk with an "iothread='#'" property. Thus, if just defined (eg, <iothreads> and <iothreadids>), we ignore them if the emulator doesn't support it. However, if used, then yes we fail. Naturally this is one of those only one downstream platform really cares - just happens to be one that does a lot of QA for Red Hat. Although a fairly useless configuration, I think because it seems it's possible to have 'niothreadids' with empty fields other than 'iothread_id' that checking for the capability is more correct in these instances than checking for niothreadids. Unless of course you feel we should fail in qemuBuildCommandLine if iothreads are defined. John
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 570dab5..66318ea 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -1163,6 +1163,9 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm) char *mem_mask = NULL; virDomainNumatuneMemMode mem_mode;
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0;
Spreading checks like this through the code usually ends up to be fragile.
I think this should be if (def->niothreads == 0) return 0;
+ if ((period || quota) && !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8aa9efc..67e7cbc 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2514,10 +2514,14 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm) static int qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm) { + qemuDomainObjPrivatePtr priv = vm->privateData; virDomainDefPtr def = vm->def; size_t i; int ret = -1;
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) + return 0; + for (i = 0; i < def->niothreadids; i++) {
Here this function is doing the right thing.
/* set affinity only for existing iothreads */ if (!def->iothreadids[i]->cpumask) @@ -2574,12 +2578,14 @@ qemuProcessSetSchedulers(virDomainObjPtr vm) return -1; }
- for (i = 0; i < vm->def->niothreadids; i++) { - if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, - vm->def->iothreadids[i]->thread_id, - vm->def->cputune.niothreadsched, - vm->def->cputune.iothreadsched) < 0) - return -1; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + for (i = 0; i < vm->def->niothreadids; i++) {
As well as here.
+ if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id, + vm->def->iothreadids[i]->thread_id, + vm->def->cputune.niothreadsched, + vm->def->cputune.iothreadsched) < 0) + return -1; + }
Peter
On Tue, Oct 13, 2015 at 18:31:11 -0400, John Ferlan wrote:
On 10/13/2015 12:08 PM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 11:47:07 -0400, John Ferlan wrote:
During qemu process startup (qemuProcessStart), the call to qemuProcessDetectIOThreadPIDs will not attempt to fill in the ->thread_id values if the binary doesn't support IOThreads. However, subsequent calls to setup the IOThread cgroups, affinity, and scheduler parameters had no such check, thus could attempt to set thread_id = 0, which would not be a good idea.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_cgroup.c | 3 +++ src/qemu/qemu_process.c | 18 ++++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-)
Do we even allow to start a VM that has IOthreads enabled but qemu does not support them? In that case that's the point to fix so that we have a central point where it's checked.
Upon further reflection...
During qemuBuildCommandLine there is a check for 'iothreads > 0 and have the capability', then create 'niothreadids' objects.
Then during qemuCheckIOThreads called from qemuBuildDriveDevStr, we will fail if there's a disk with an "iothread='#'" property.
Thus, if just defined (eg, <iothreads> and <iothreadids>), we ignore them if the emulator doesn't support it. However, if used, then yes we
That seems rather strange. With other things we usually point out that the configuration is broken so that the user knows that the features are not available. Additionally, since you ignore creation of the threads if qemu doesn't support it you might get into situations where if you migrate to a newer version you actually will create a different configuration with the same config. It might not fail for iothreads, but it's calling for problems.
fail. Naturally this is one of those only one downstream platform really cares - just happens to be one that does a lot of QA for Red Hat.
Although a fairly useless configuration, I think because it seems it's possible to have 'niothreadids' with empty fields other than 'iothread_id' that checking for the capability is more correct in these instances than checking for niothreadids.
Unless of course you feel we should fail in qemuBuildCommandLine if iothreads are defined.
I do feel rather strongly that we should point out that the config is invalid rather than just silently ignoring stuff the user requested. Peter
John
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
[please trim the rest of the message you are not responding to]
https://bugzilla.redhat.com/show_bug.cgi?id=1249981 When trying to pin the thread to the live process in qemuDomainPinIOThread, there was no check whether the binary supported IOThreads, thus thread_id = 0 (eg current) would be erroneously used. Follow qemuDomainChgIOThread and only check for the bit if changing the live definition vs. changing the persistent definition. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8cd5ee3..7308c3b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5813,6 +5813,12 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainIOThreadIDDefPtr iothrid; virBitmapPtr cpumask; + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported with this binary")); + goto endjob; + } + if (!(iothrid = virDomainIOThreadIDFind(def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, _("iothread %d not found"), iothread_id); -- 2.1.0
On Tue, Oct 13, 2015 at 11:47:08 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1249981
When trying to pin the thread to the live process in qemuDomainPinIOThread, there was no check whether the binary supported IOThreads, thus thread_id = 0 (eg current) would be erroneously used.
Follow qemuDomainChgIOThread and only check for the bit if changing the live definition vs. changing the persistent definition.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8cd5ee3..7308c3b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5813,6 +5813,12 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainIOThreadIDDefPtr iothrid; virBitmapPtr cpumask;
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported with this binary")); + goto endjob; + } + if (!(iothrid = virDomainIOThreadIDFind(def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, _("iothread %d not found"), iothread_id);
Here this function should return that the iothread could not be found as such VM shouldn't allow any iothreads if we don't support them. Peter
On 10/13/2015 12:10 PM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 11:47:08 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1249981
When trying to pin the thread to the live process in qemuDomainPinIOThread, there was no check whether the binary supported IOThreads, thus thread_id = 0 (eg current) would be erroneously used.
Follow qemuDomainChgIOThread and only check for the bit if changing the live definition vs. changing the persistent definition.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8cd5ee3..7308c3b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5813,6 +5813,12 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainIOThreadIDDefPtr iothrid; virBitmapPtr cpumask;
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("IOThreads not supported with this binary")); + goto endjob; + } + if (!(iothrid = virDomainIOThreadIDFind(def, iothread_id))) { virReportError(VIR_ERR_INVALID_ARG, _("iothread %d not found"), iothread_id);
Here this function should return that the iothread could not be found as such VM shouldn't allow any iothreads if we don't support them.
I think it's preferable to have consistent error messages. See qemuDomainGetIOThreadsLive and qemuDomainChgIOThread. Giving a message such as iothread could not be found is I believe misleading. John
On Tue, Oct 13, 2015 at 18:33:26 -0400, John Ferlan wrote:
On 10/13/2015 12:10 PM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 11:47:08 -0400, John Ferlan wrote:
...
Here this function should return that the iothread could not be found as such VM shouldn't allow any iothreads if we don't support them.
I think it's preferable to have consistent error messages. See qemuDomainGetIOThreadsLive and qemuDomainChgIOThread.
Giving a message such as iothread could not be found is I believe misleading.
It is not misleading. It's true. In the configuration described you can't have any iothreads so a message that you don't have any is spot on. Peter
On 10/14/2015 04:38 AM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 18:33:26 -0400, John Ferlan wrote:
On 10/13/2015 12:10 PM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 11:47:08 -0400, John Ferlan wrote:
...
Here this function should return that the iothread could not be found as such VM shouldn't allow any iothreads if we don't support them.
I think it's preferable to have consistent error messages. See qemuDomainGetIOThreadsLive and qemuDomainChgIOThread.
Giving a message such as iothread could not be found is I believe misleading.
It is not misleading. It's true. In the configuration described you can't have any iothreads so a message that you don't have any is spot on.
And there are no iothreads because they are not supported in the environment - it's the root cause. If someone looked at their XML and saw "<iothreads>1</iothreads>" and/or "<iothreadids> <iothread id="1"/> </iothreadids>, they could claim they should have iothreads. Thus the problem isn't because there are no iothreads defined, it's because they're not supported. Fortunately if we fix what was pointed out in patch 1 to not allow startup if iothreads are requested, but the environment doesn't support them, then this one won't matter anyway. Oh and IMO the right message would then be displayed... Looking back through history - the qemuBuildCommandLine check was put in when there were *just* iothreads and we iothreadids weren't considered. But still, thinking about it now - the check shouldn't have been conjunctive. Although that may have been a conscious decision at the time since the capability check was made when a disk requested using an IOThread (and we couldn't dynamically add the iothread object when first supported). I'll make adjustments and post a v2 of the series... John
Create a separate local API that will fill in the iothreadid array entries that were not defined by <iothread id='#'> entries in the XML. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 65e0d8e..217179d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2329,6 +2329,35 @@ virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, } +static int +virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) +{ + unsigned int iothread_id = 1; + int retval = -1; + + /* Same value (either 0 or some number), then we have none to fill in or + * the iothreadid array was filled from the XML + */ + if (def->iothreads == def->niothreadids) + return 0; + + while (def->niothreadids != def->iothreads) { + if (!virDomainIOThreadIDFind(def, iothread_id)) { + virDomainIOThreadIDDefPtr iothrid; + + if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) + goto error; + iothrid->autofill = true; + } + iothread_id++; + } + retval = 0; + + error: + return retval; +} + + void virDomainPinDefFree(virDomainPinDefPtr def) { @@ -14979,22 +15008,8 @@ virDomainDefParseXML(xmlDocPtr xml, } VIR_FREE(nodes); - /* If no iothreadid's or not fully populated, let's finish the job - * here rather than in PostParseCallback - */ - if (def->iothreads && def->iothreads != def->niothreadids) { - unsigned int iothread_id = 1; - while (def->niothreadids != def->iothreads) { - if (!virDomainIOThreadIDFind(def, iothread_id)) { - virDomainIOThreadIDDefPtr iothrid; - - if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) - goto error; - iothrid->autofill = true; - } - iothread_id++; - } - } + if (virDomainIOThreadIDDefArrayInit(def) < 0) + goto error; /* Extract cpu tunables. */ if ((n = virXPathULong("string(./cputune/shares[1])", ctxt, -- 2.1.0
On Tue, Oct 13, 2015 at 11:47:09 -0400, John Ferlan wrote:
Create a separate local API that will fill in the iothreadid array entries that were not defined by <iothread id='#'> entries in the XML.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 65e0d8e..217179d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2329,6 +2329,35 @@ virDomainIOThreadIDDefArrayFree(virDomainIOThreadIDDefPtr *def, }
+static int +virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) +{ + unsigned int iothread_id = 1; + int retval = -1; + + /* Same value (either 0 or some number), then we have none to fill in or + * the iothreadid array was filled from the XML
The previous comment removed later was a bit easier to read.
+ */ + if (def->iothreads == def->niothreadids) + return 0;
ACK, Peter
https://bugzilla.redhat.com/show_bug.cgi?id=1264008 The existing algorithm assumed that someone was making small, incremental changes; however, it is possible to change iothreads from 0 (or relatively small number) to some really large number and the algorithm would possibly spin its wheels doing unnecessary searches. So, optimize the algorithm in order to first detect whether there are any iothreadid's defined in the XML. If not, then rather than add one at a time searching for the next valid id, just allocate the whole array and populate it as designed starting at iothread_id = 1 up to the number of iothreads defined in the XML Otherwise, we have a situation where "some number" of iothreadid's were defined in the XML and we're filling in the holes of iothread_id's. Thus, instead of determining if the iothread_id was used (ThreadIDFind) for every ID entry that needs to be filled in, let's only call the find while we still have holes and rather than additionally calling ThreadIDAdd (which also calls the ThreadIDFind), let's just directly add the entry. These algorithm changes only "penalize" those with a large iothreads value that also have a large number of iothread id's which aren't completely defined. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 46 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 217179d..6c90653 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2334,6 +2334,8 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) { unsigned int iothread_id = 1; int retval = -1; + size_t i; + virDomainIOThreadIDDefPtr iothrid = NULL; /* Same value (either 0 or some number), then we have none to fill in or * the iothreadid array was filled from the XML @@ -2341,15 +2343,49 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) if (def->iothreads == def->niothreadids) return 0; - while (def->niothreadids != def->iothreads) { - if (!virDomainIOThreadIDFind(def, iothread_id)) { - virDomainIOThreadIDDefPtr iothrid; + /* Optimize - there are no <iothread id='#'> in the XML */ + if (def->niothreadids == 0) { + if (VIR_ALLOC_N(def->iothreadids, def->iothreads) < 0) + goto error; + def->niothreadids = def->iothreads; + for (i = 1; i <= def->iothreads; i++) { + if (VIR_ALLOC(iothrid) < 0) + goto error; + def->iothreadids[i - 1] = iothrid; + iothrid->iothread_id = i; + iothrid->autofill = true; + } + } else { + int found = 0; + int orig_nids = def->niothreadids; + + /* <iothread id='#'> entries were found, then let's fill in the + * holes one at a time, e.g. the relatively hard way. Rather than + * using ThreadIDFind and call ThreadIDAdd which also calls + * ThreadIDFind again which could cause lots of needless spinning + * let's just add the entries directly + */ + for (i = 0; + i < def->iothreads && def->niothreadids != def->iothreads; i++) { + /* While we still have defined <thread id='#'>'s compare our + * current thread_id value against the array. + */ + if (found < orig_nids && + virDomainIOThreadIDFind(def, iothread_id)) { + iothread_id++; + found++; + continue; + } - if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) + /* Add a new entry using the current iothread_id */ + if (VIR_ALLOC(iothrid) < 0) goto error; + iothrid->iothread_id = iothread_id++; iothrid->autofill = true; + if (VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids, + iothrid) < 0) + goto error; } - iothread_id++; } retval = 0; -- 2.1.0
On Tue, Oct 13, 2015 at 11:47:10 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1264008
The existing algorithm assumed that someone was making small, incremental changes; however, it is possible to change iothreads from 0 (or relatively small number) to some really large number and the algorithm would possibly spin its wheels doing unnecessary searches.
While the existing algorithm was "suboptimal" the use case is strange too. Starting a million iothreads certainly won't help in the total performance.
So, optimize the algorithm in order to first detect whether there are any iothreadid's defined in the XML. If not, then rather than add one at a time searching for the next valid id, just allocate the whole array and populate it as designed starting at iothread_id = 1 up to the number of iothreads defined in the XML
Otherwise, we have a situation where "some number" of iothreadid's were defined in the XML and we're filling in the holes of iothread_id's. Thus, instead of determining if the iothread_id was used (ThreadIDFind) for every ID entry that needs to be filled in, let's only call the find while we still have holes and rather than additionally calling ThreadIDAdd (which also calls the ThreadIDFind), let's just directly add the entry.
These algorithm changes only "penalize" those with a large iothreads value that also have a large number of iothread id's which aren't completely defined.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 46 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 217179d..6c90653 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2334,6 +2334,8 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) { unsigned int iothread_id = 1; int retval = -1; + size_t i; + virDomainIOThreadIDDefPtr iothrid = NULL;
/* Same value (either 0 or some number), then we have none to fill in or * the iothreadid array was filled from the XML @@ -2341,15 +2343,49 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) if (def->iothreads == def->niothreadids) return 0;
The code below seems too complex. I'd suggest something along the following pseudo-code: assert(def->iothreads >= def->niothreads); virBitmapPtr *threads = virBitmapNew(def->iothreads); ssize_t nxt = 0; virBitmapSetAll(threads); /* mark which are already provided by the user */ for (i = 0; i < def->niothreads; i++) virBitmapClearBit(threads, def->iothreadids[i]->id); /* resize array */ VIR_REALLOC_N(def->iothreadids....); while ((nxt = virBitmapNextSetBit(bitmap, nxt)) >= 0) { /* add the stuff */ } (may contain off-by-ones)
- while (def->niothreadids != def->iothreads) { - if (!virDomainIOThreadIDFind(def, iothread_id)) { - virDomainIOThreadIDDefPtr iothrid; + /* Optimize - there are no <iothread id='#'> in the XML */ + if (def->niothreadids == 0) { + if (VIR_ALLOC_N(def->iothreadids, def->iothreads) < 0) + goto error; + def->niothreadids = def->iothreads; + for (i = 1; i <= def->iothreads; i++) { + if (VIR_ALLOC(iothrid) < 0) + goto error; + def->iothreadids[i - 1] = iothrid; + iothrid->iothread_id = i; + iothrid->autofill = true; + } + } else { + int found = 0; + int orig_nids = def->niothreadids; + + /* <iothread id='#'> entries were found, then let's fill in the + * holes one at a time, e.g. the relatively hard way. Rather than + * using ThreadIDFind and call ThreadIDAdd which also calls + * ThreadIDFind again which could cause lots of needless spinning + * let's just add the entries directly + */ + for (i = 0; + i < def->iothreads && def->niothreadids != def->iothreads; i++) {
I'll happily live with a longer line rather than a broken if statement.
+ /* While we still have defined <thread id='#'>'s compare our + * current thread_id value against the array. + */ + if (found < orig_nids && + virDomainIOThreadIDFind(def, iothread_id)) { + iothread_id++; + found++; + continue; + }
- if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) + /* Add a new entry using the current iothread_id */ + if (VIR_ALLOC(iothrid) < 0) goto error; + iothrid->iothread_id = iothread_id++; iothrid->autofill = true; + if (VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids, + iothrid) < 0)
Rather than extending the array all the time you can extend it once and then fill it.
+ goto error; } - iothread_id++; } retval = 0;
-- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/13/2015 12:29 PM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 11:47:10 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1264008
The existing algorithm assumed that someone was making small, incremental changes; however, it is possible to change iothreads from 0 (or relatively small number) to some really large number and the algorithm would possibly spin its wheels doing unnecessary searches.
While the existing algorithm was "suboptimal" the use case is strange too. Starting a million iothreads certainly won't help in the total performance.
Yeah - I agree, but since we didn't set a limit... I think to a degree we are stuck. Sometimes I wonder about the reality of tests. Then again, I seem to recall a recent dialog over the number of nics (and thus hwaddrs) that were deemed reasonable when the algorithm was first written...
So, optimize the algorithm in order to first detect whether there are any iothreadid's defined in the XML. If not, then rather than add one at a time searching for the next valid id, just allocate the whole array and populate it as designed starting at iothread_id = 1 up to the number of iothreads defined in the XML
Otherwise, we have a situation where "some number" of iothreadid's were defined in the XML and we're filling in the holes of iothread_id's. Thus, instead of determining if the iothread_id was used (ThreadIDFind) for every ID entry that needs to be filled in, let's only call the find while we still have holes and rather than additionally calling ThreadIDAdd (which also calls the ThreadIDFind), let's just directly add the entry.
These algorithm changes only "penalize" those with a large iothreads value that also have a large number of iothread id's which aren't completely defined.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 46 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 217179d..6c90653 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2334,6 +2334,8 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) { unsigned int iothread_id = 1; int retval = -1; + size_t i; + virDomainIOThreadIDDefPtr iothrid = NULL;
/* Same value (either 0 or some number), then we have none to fill in or * the iothreadid array was filled from the XML @@ -2341,15 +2343,49 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) if (def->iothreads == def->niothreadids) return 0;
The code below seems too complex. I'd suggest something along the following pseudo-code:
assert(def->iothreads >= def->niothreads);
virBitmapPtr *threads = virBitmapNew(def->iothreads); ssize_t nxt = 0;
virBitmapSetAll(threads);
/* mark which are already provided by the user */ for (i = 0; i < def->niothreads; i++) virBitmapClearBit(threads, def->iothreadids[i]->id);
/* resize array */ VIR_REALLOC_N(def->iothreadids....);
while ((nxt = virBitmapNextSetBit(bitmap, nxt)) >= 0) { /* add the stuff */ }
This would work if we required thread_id values to be <= def->iothreads, but we don't, yet... I thought of using a bitmap, but it would no cover the following case : <iothreads>4</iothreads> <iothreadids> <iothread id='100'/> <iothreadids> This would/should create thread ids 100, 1, 2, 3 We never placed a limit on the thread_id value other than it cannot be zero. Although we do indicate the default algorithm is 1 to the number of iothreads. Would a 'real world' user do something like this - perhaps not. Much less make "<iothreads>1000000</iothreads>" (that'd be one long command line - say nothing of the resources required in order to really start it. I'd be all for restricting iothread id values to be <= def->iothreads - that'd solve the unnecessarily complex algorithm. My other thought was to restrict the number of iothreads to be the number of disk devices or even 2 * ndisks. It's possible to assign multiple disks to 1 iothread, but impossible to assign 1 disk to multiple threads.
(may contain off-by-ones)
- while (def->niothreadids != def->iothreads) { - if (!virDomainIOThreadIDFind(def, iothread_id)) { - virDomainIOThreadIDDefPtr iothrid; + /* Optimize - there are no <iothread id='#'> in the XML */ + if (def->niothreadids == 0) { + if (VIR_ALLOC_N(def->iothreadids, def->iothreads) < 0) + goto error; + def->niothreadids = def->iothreads; + for (i = 1; i <= def->iothreads; i++) { + if (VIR_ALLOC(iothrid) < 0) + goto error; + def->iothreadids[i - 1] = iothrid; + iothrid->iothread_id = i; + iothrid->autofill = true; + } + } else { + int found = 0; + int orig_nids = def->niothreadids; + + /* <iothread id='#'> entries were found, then let's fill in the + * holes one at a time, e.g. the relatively hard way. Rather than + * using ThreadIDFind and call ThreadIDAdd which also calls + * ThreadIDFind again which could cause lots of needless spinning + * let's just add the entries directly + */ + for (i = 0; + i < def->iothreads && def->niothreadids != def->iothreads; i++) {
I'll happily live with a longer line rather than a broken if statement.
Well 80 cols max is the desired number. Although in thinking about it, I don't think we could ever hit the i == def->iothreads - if we did then there was another error. Removing it moves us back down to < 80 all on one for line.
+ /* While we still have defined <thread id='#'>'s compare our + * current thread_id value against the array. + */ + if (found < orig_nids && + virDomainIOThreadIDFind(def, iothread_id)) { + iothread_id++; + found++; + continue; + }
- if (!(iothrid = virDomainIOThreadIDAdd(def, iothread_id))) + /* Add a new entry using the current iothread_id */ + if (VIR_ALLOC(iothrid) < 0) goto error; + iothrid->iothread_id = iothread_id++; iothrid->autofill = true; + if (VIR_APPEND_ELEMENT_COPY(def->iothreadids, def->niothreadids, + iothrid) < 0)
Rather than extending the array all the time you can extend it once and then fill it.
Suffice to say those VIR_APPEND_* are not necessarily self documenting. I assume though you are indicating using VIR_RESIZE_N. However, that may not work in 'all cases'. This algorithm handles the case where there's 5 iothreads and let's say id "2" and "5" are defined. There's no way to "know" what the id's are without looking and I gave up sorting them. Again, assuming iothread_id can be > def->iothreads. John
+ goto error; } - iothread_id++; } retval = 0;
-- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Oct 13, 2015 at 13:54:54 -0400, John Ferlan wrote:
On 10/13/2015 12:29 PM, Peter Krempa wrote:
On Tue, Oct 13, 2015 at 11:47:10 -0400, John Ferlan wrote:
...
@@ -2341,15 +2343,49 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def) if (def->iothreads == def->niothreadids) return 0;
The code below seems too complex. I'd suggest something along the following pseudo-code:
assert(def->iothreads >= def->niothreads);
virBitmapPtr *threads = virBitmapNew(def->iothreads); ssize_t nxt = 0;
virBitmapSetAll(threads);
/* mark which are already provided by the user */ for (i = 0; i < def->niothreads; i++) virBitmapClearBit(threads, def->iothreadids[i]->id);
/* resize array */ VIR_REALLOC_N(def->iothreadids....);
while ((nxt = virBitmapNextSetBit(bitmap, nxt)) >= 0) {
[1]
/* add the stuff */ }
This would work if we required thread_id values to be <= def->iothreads, but we don't, yet... I thought of using a bitmap, but it would no cover the following case :
Actually not necessarily, you only need to map the IDs in the range of <0,def->iothreads> since the worst case scenario is that you'd be adding all the iothreads in that range (if def->niothreadids is 0). Any iothread with id greater than def->iothreads will be ignored in the mapping process, but left in the array so the suggested loop [1] needs to be slightly modified: while ((nxt = virBitmapNextSetBit(bitmap, nxt)) >= 0 && def->niothreadis < def->iothreads) {
<iothreads>4</iothreads> <iothreadids> <iothread id='100'/> <iothreadids>
This would/should create thread ids 100, 1, 2, 3
The code above with the slight tweak will produce exactly the results above.
We never placed a limit on the thread_id value other than it cannot be zero. Although we do indicate the default algorithm is 1 to the number of iothreads.
Would a 'real world' user do something like this - perhaps not. Much less make "<iothreads>1000000</iothreads>" (that'd be one long command line - say nothing of the resources required in order to really start it.
I'd be all for restricting iothread id values to be <= def->iothreads - that'd solve the unnecessarily complex algorithm.
My other thought was to restrict the number of iothreads to be the number of disk devices or even 2 * ndisks. It's possible to assign multiple disks to 1 iothread, but impossible to assign 1 disk to multiple threads.
IMO, none of the above is necessary. Peter
participants (2)
-
John Ferlan -
Peter Krempa