On Mon, Mar 09, 2015 at 08:27:10AM -0400, John Ferlan wrote:
On 03/08/2015 07:03 AM, Ján Tomko wrote:
ACK series, thanks for touching up VcpuInfo and EmulatorInfo as well!
There's one bug that I noticed: If the CPUs are pinned domain-wide, that is: <vcpu placement='static' cpuset='0-1'>4</vcpu> <iothreads>2</iothreads>
Both vcpu threads and iothreads will inherit this pinning.
For a shutoff domain, vcpupininfo will display 0-1 for all vcpus, but iothreadsinfo shows 0-4, even though they will get pinned to 0-1 after domain startup.
Turns out the vpcupin info is filled for all the vcpus when the XML is parsed since commit 10f8a45deb0f057a70a0d49794d3a3d19d17dceb
Falling back to targetDef->cpumask in qemuDomainGetIOThreadsConfig (as qemuDomainGetEmulatorPinInfo does) would solve that too.
Squashed the following into patch 4 to address this...
I don't think this functional change belongs to the patch refactoring the function to use the virBitmap* APIs. But since the functionality has not yet been released, nobody should feel the need to backport any of these commits, so mixing them up is not that big deal.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2a9db1b..ceceafa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5653,6 +5653,8 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, virDomainIOThreadInfoPtr **info) { virDomainIOThreadInfoPtr *info_ret = NULL; + virBitmapPtr bitmap = NULL; + virBitmapPtr cpumask = NULL; int hostcpus; size_t i; int ret = -1; @@ -5668,7 +5670,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef,
for (i = 0; i < targetDef->iothreads; i++) { virDomainVcpuPinDefPtr pininfo; - virBitmapPtr bitmap = NULL;
if (VIR_ALLOC(info_ret[i]) < 0) goto cleanup; @@ -5681,20 +5682,22 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, targetDef->cputune.niothreadspin, i + 1); if (!pininfo) { - if (!(bitmap = virBitmapNew(hostcpus))) - goto cleanup; - virBitmapSetAll(bitmap); + if (targetDef->cpumask) { + cpumask = targetDef->cpumask; + } else { + if (!(bitmap = virBitmapNew(hostcpus))) + goto cleanup; + virBitmapSetAll(bitmap); + cpumask = bitmap; + } } else { - bitmap = pininfo->cpumask; + cpumask = pininfo->cpumask; } - if (virBitmapToData(bitmap, &info_ret[i]->cpumap, - &info_ret[i]->cpumaplen) < 0) { - if (!pininfo) - virBitmapFree(bitmap); + if (virBitmapToData(cpumask, &info_ret[i]->cpumap, + &info_ret[i]->cpumaplen) < 0) goto cleanup; - } - if (!pininfo) - virBitmapFree(bitmap); + virBitmapFree(bitmap); + bitmap = NULL; }
*info = info_ret; @@ -5707,6 +5710,7 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef, virDomainIOThreadsInfoFree(info_ret[i]); VIR_FREE(info_ret); } + virBitmapFree(bitmap);
return ret; }
Replaced the overly aggressively removed lines from patch 5...
I will send a separate patch for the domain_conf.c change that addresses any needs for iothreadspin setup to use the def->cpumask.
However the squashed-in patch could've benefited from going through a standard review process. If you plan to copy the vcpupin code in domain_conf.c to do the same with iothreadspin, the squashed-in change is redundant. Jan