On 04/27/2015 10:20 AM, Peter Krempa wrote:
> 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.
>
Removed the "virBitmapEqual(def->cpumask, def->iothreadids[i]->cpumask)"
check.
John
>>
>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list