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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list