On Fri, Apr 24, 2015 at 12:05:56 -0400, John Ferlan wrote:
> Remove the iothreadspin array from cputune and replace with a cpumask
> to be stored in the iothreadids list.
>
> Adjust the test output because our printing goes in order of the iothreadids
> list now.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 10 +-
> src/conf/domain_conf.c | 112 ++++++++++-----------
> src/conf/domain_conf.h | 3 +-
> src/qemu/qemu_cgroup.c | 16 +--
> src/qemu/qemu_driver.c | 75 ++++----------
> src/qemu/qemu_process.c | 10 +-
> .../qemuxml2xmlout-cputune-iothreads.xml | 2 +-
> 7 files changed, 86 insertions(+), 142 deletions(-)
>
...
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ddb0d83..129908d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
...
> @@ -20862,16 +20859,19 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> VIR_FREE(cpumask);
> }
>
> - for (i = 0; i < def->cputune.niothreadspin; i++) {
> + for (i = 0; i < def->niothreadids; i++) {
> char *cpumask;
> - /* Ignore the iothreadpin which inherit from "cpuset of "<vcpu>." */
> - if (virBitmapEqual(def->cpumask, def->cputune.iothreadspin[i]->cpumask))
> +
> + /* Ignore iothreadids with no cpumask or a cpumask that
> + * inherits from "cpuset of "<vcpu>." */
> + if (!def->iothreadids[i]->cpumask ||
> + virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask))
> continue;
I still think that comparing the cpu map with the default cpumap is
wrong. It should not be copied there in the first place. Additionally if
the user specifies the same CPU map as the default one, we should not
remove it.
>
> virBufferAsprintf(buf, "<iothreadpin iothread='%u' ",
> - def->cputune.iothreadspin[i]->id);
> + def->iothreadids[i]->iothread_id);
>
> - if (!(cpumask = virBitmapFormat(def->cputune.iothreadspin[i]->cpumask)))
> + if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask)))
> goto error;
>
> virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask);
...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 330ffdf..1fac0b8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
...
> @@ -6145,31 +6114,23 @@ qemuDomainPinIOThread(virDomainPtr dom,
> }
>
> if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> + virDomainIOThreadIDDefPtr iothrid;
> + virBitmapPtr cpumask;
> +
> /* Coverity didn't realize that targetDef must be set if we got here. */
> sa_assert(persistentDef);
>
> - if (iothread_id > persistentDef->iothreads) {
> + if (!(iothrid = virDomainIOThreadIDFind(persistentDef, iothread_id))) {
> virReportError(VIR_ERR_INVALID_ARG,
> - _("iothread value out of range %d > %d"),
> - iothread_id, persistentDef->iothreads);
> + _("iothreadid %d not found"), iothread_id);
> goto endjob;
> }
>
> - if (!persistentDef->cputune.iothreadspin) {
> - if (VIR_ALLOC(persistentDef->cputune.iothreadspin) < 0)
> - goto endjob;
> - persistentDef->cputune.niothreadspin = 0;
> - }
> - if (virDomainPinAdd(&persistentDef->cputune.iothreadspin,
> - &persistentDef->cputune.niothreadspin,
> - cpumap,
> - maplen,
> - iothread_id) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("failed to update or add iothreadspin xml "
> - "of a persistent domain"));
> + if (!(cpumask = virBitmapNewData(cpumap, maplen)))
> goto endjob;
> - }
> +
> + virBitmapFree(iothrid->cpumask);
> + iothrid->cpumask = cpumask;
Much nicer!
>
> ret = virDomainSaveConfig(cfg->configDir, persistentDef);
> goto endjob;
The code is much cleaner now. The cpu threads/pinning implementation is
horrible in this aspect.
ACK with the bitmap comparison removed.
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list