8:46 a.m.
On Thu, Apr 23, 2015 at 08:58:26 -0400, John Ferlan wrote:
On 04/23/2015 08:02 AM, Peter Krempa wrote:
> On Tue, Apr 21, 2015 at 19:31:25 -0400, John Ferlan wrote:
>> Remove the iothreadspin array from cputune and replace with a cpumask
>> to be stored in the iothreadids list
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> docs/formatdomain.html.in | 10 ++--
>> src/conf/domain_conf.c | 118 +++++++++++++++++++++-------------------------
>> src/conf/domain_conf.h | 2 +-
>> src/qemu/qemu_cgroup.c | 13 ++---
>> src/qemu/qemu_driver.c | 79 +++++++++----------------------
>> src/qemu/qemu_process.c | 7 +--
>> 6 files changed, 86 insertions(+), 143 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 518f7c5..7af6bd7 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -624,11 +624,11 @@
>> and attribute <code>cpuset</code> of element
<code>vcpu</code> is
>> not specified, the IOThreads are pinned to all the physical CPUs
>> by default. There are two required attributes, the attribute
>> - <code>iothread</code> specifies the IOThread id and the
attribute
>> - <code>cpuset</code> specifying which physical CPUs to pin
to. The
>> - <code>iothread</code> value begins at "1"
through the number of
>> - <a
href="#elementsIOThreadsAllocation"><code>iothreads</code></a>
>> - allocated to the domain. A value of "0" is not permitted.
>> + <code>iothread</code> specifies the IOThread ID and the
attribute
>> + <code>cpuset</code> specifying which physical CPUs to pin
to. See
>> + the <code>iothreadids</code>
>> + <a
href="#elementsIOThreadsAllocation"><code>description</code></a>
>> + for valid <code>iothread</code> values.
>
> This description is fair enough. It hints that the implicit ones are
> numbered too.
>
>> <span class="since">Since 1.2.9</span>
>> </dd>
>> <dt><code>shares</code></dt>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index bd25d52..969e56f 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2118,8 +2118,10 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int npin)
>> void
>> virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def)
>> {
>> - if (def)
>> + if (def) {
>> + virBitmapFree(def->cpumask);
>> VIR_FREE(def);
>
> This fixes the syntax check failure from 1/9.
>
Byproduct of rewrites...
I've change 1/9 to be (so that each patch passes check/syntax-check)
+ if (!def)
+ return;
+ VIR_FREE(def);
Which will change this to be:
if (!def)
return;
+ virBitmapFree(def->cpumask);
VIR_FREE(def);
>> + }
>
Okay, I didn't see that because I've remembered a different version of
patch 1 where you allocated the full list upfront.
By the way, the approach in this series now has the following loophole:
1) Specify <iothreads> > nelemes(<iothreadids>)
2) Specify iothreadpin for thread 9999.
Now one of the threads is added with id 9999 due to the code above.
Thus I think that the PostParse callback code should probably be removed
and the missing threads should be numbered right after <iohtreadids> is
parsed so that this patch doesn't allow that.
}
>>
>>
>> @@ -2341,9 +2343,6 @@ void virDomainDefFree(virDomainDefPtr def)
>>
>> virDomainPinDefFree(def->cputune.emulatorpin);
>>
>> - virDomainPinDefArrayFree(def->cputune.iothreadspin,
>> - def->cputune.niothreadspin);
>> -
>> for (i = 0; i < def->cputune.nvcpusched; i++)
>> virBitmapFree(def->cputune.vcpusched[i].ids);
>> VIR_FREE(def->cputune.vcpusched);
>> @@ -13316,74 +13315,77 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node,
>> * and an iothreadspin has the form
>> * <iothreadpin iothread='1' cpuset='2'/>
>> */
>> -static virDomainPinDefPtr
>> +static int
>> virDomainIOThreadPinDefParseXML(xmlNodePtr node,
>> xmlXPathContextPtr ctxt,
>> - int iothreads)
>> + virDomainDefPtr def)
>> {
>> - virDomainPinDefPtr def;
>> + int ret = -1;
>> + virDomainIOThreadIDDefPtr iothrid;
>> + virBitmapPtr cpumask;
>> xmlNodePtr oldnode = ctxt->node;
>> unsigned int iothreadid;
>> char *tmp = NULL;
>>
>> - if (VIR_ALLOC(def) < 0)
>> - return NULL;
>> -
>> ctxt->node = node;
>>
>> if (!(tmp = virXPathString("string(./@iothread)", ctxt))) {
>> virReportError(VIR_ERR_XML_ERROR, "%s",
>> _("missing iothread id in iothreadpin"));
>> - goto error;
>> + goto cleanup;
>> }
>>
>> if (virStrToLong_uip(tmp, NULL, 10, &iothreadid) < 0) {
>> virReportError(VIR_ERR_XML_ERROR,
>> _("invalid setting for iothread
'%s'"), tmp);
>> - goto error;
>> + goto cleanup;
>> }
>> VIR_FREE(tmp);
>>
>> if (iothreadid == 0) {
>> virReportError(VIR_ERR_XML_ERROR, "%s",
>> _("zero is an invalid iothread id value"));
>> - goto error;
>> - }
>> -
>> - /* IOThreads are numbered "iothread1...iothread<n>", where
>> - * "n" is the iothreads value */
>> - if (iothreadid > iothreads) {
>> - virReportError(VIR_ERR_XML_ERROR, "%s",
>> - _("iothread id must not exceed iothreads"));
>
> [1]
>
>> - goto error;
>> + goto cleanup;
>> }
>>
>> - def->id = iothreadid;
>> -
>> if (!(tmp = virXMLPropString(node, "cpuset"))) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("missing cpuset for iothreadpin"));
>> - goto error;
>> + goto cleanup;
>> }
>>
>> - if (virBitmapParse(tmp, 0, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN)
< 0)
>> - goto error;
>> + if (virBitmapParse(tmp, 0, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
>> + goto cleanup;
>>
>> - if (virBitmapIsAllClear(def->cpumask)) {
>> + if (virBitmapIsAllClear(cpumask)) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("Invalid value of 'cpuset': %s"),
>> tmp);
>> - goto error;
>> + goto cleanup;
>> + }
>> +
>> + if (!(iothrid = virDomainIOThreadIDFind(def, iothreadid))) {
>> + if (!(iothrid = virDomainIOThreadIDAdd(def, iothreadid)))
>
> Why are you adding a iothread definition here? This code is executed
> after the iothread info should already be parsed so adding a thread here
> doesn't make sense. The section here should ressemble the check [1] that
> you've removed.
>
Because you required that the iothreadpin information to be included in
the iothreadid; however, if someone hasn't defined <iothreadids>'s, then
there won't be any until the PostParse is called - thus we have to add
one here so that we can store the pin information for an iothread
This means someone could have:
<iothreads>4</iothreads>
<cputune>
...
<iothreadpin iothread="1" cpuset="5,6"/>
...
</cputune>
NOTE: No <iothreadids>
So I believe it stays as is
>> + goto cleanup;
>> + iothrid->autofill = true;
>> + }
>> +
>> + if (iothrid->cpumask) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("duplicate iothreadpin for same iothread
'%u'"),
>> + iothreadid);
>> + goto cleanup;
>> }
>>
>> + iothrid->cpumask = cpumask;
>> + cpumask = NULL;
>> + ret = 0;
>> +
>> cleanup:
>> VIR_FREE(tmp);
>> + virBitmapFree(cpumask);
>> ctxt->node = oldnode;
>> - return def;
>> -
>> - error:
>> - VIR_FREE(def);
>> - goto cleanup;
>> + return ret;
>
It doesn't make much sense though. The default pinning is used from
def->cpumap if the specific is not present. If the specific pinning is
removed it should be freed.
}
>>
>>
>> @@ -14250,28 +14252,11 @@ virDomainDefParseXML(xmlDocPtr xml,
>> goto error;
>> }
>>
>> - if (n && VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0)
>> - goto error;
>> -
>> for (i = 0; i < n; i++) {
>> - virDomainPinDefPtr iothreadpin = NULL;
>> - iothreadpin = virDomainIOThreadPinDefParseXML(nodes[i], ctxt,
>> - def->iothreads);
>> - if (!iothreadpin)
>> + if (virDomainIOThreadPinDefParseXML(nodes[i], ctxt, def) < 0)
>> goto error;
>> -
>> - if (virDomainPinIsDuplicate(def->cputune.iothreadspin,
>> - def->cputune.niothreadspin,
>> - iothreadpin->id)) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("duplicate iothreadpin for same
iothread"));
>> - virDomainPinDefFree(iothreadpin);
>> - goto error;
>> - }
>> -
>> - def->cputune.iothreadspin[def->cputune.niothreadspin++] =
>> - iothreadpin;
>> }
>> + def->cputune.niothreadspin = n;
>
> This variable should be removed along with the iothread pinning
> structure, shouldn't it?
>
OK
>> VIR_FREE(nodes);
>>
>> if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt,
&nodes)) < 0) {
>> @@ -14384,7 +14369,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>>
>> if (virDomainNumatuneHasPlacementAuto(def->numa) &&
>> !def->cpumask && !def->cputune.vcpupin &&
>> - !def->cputune.emulatorpin && !def->cputune.iothreadspin)
>> + !def->cputune.emulatorpin &&
!def->cputune.niothreadspin)
>
> This could be replaced by a function that checks whether any of the
> threads has pinning info present rather than counting the pinning info.
>
OK
>> def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;
>>
>> if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0)
{
>> @@ -20837,20 +20822,25 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>> VIR_FREE(cpumask);
>> }
>>
>> - for (i = 0; i < def->cputune.niothreadspin; i++) {
>> - char *cpumask;
>> - /* Ignore the iothreadpin which inherit from "cpuset of
"<vcpu>." */
>> - if (virBitmapEqual(def->cpumask,
def->cputune.iothreadspin[i]->cpumask))
>> - continue;
>> + if (def->cputune.niothreadspin) {
>
> The check above is useless. If neither of the iothreads has pin info, it
> would be skipped ...
>
yep
>> + for (i = 0; i < def->niothreadids; i++) {
>> + char *cpumask;
>>
>> - virBufferAsprintf(buf, "<iothreadpin iothread='%u'
",
>> - def->cputune.iothreadspin[i]->id);
>> + /* 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;
>
> ... here. Also why are you explicitly rejecting cpumask that is equal to
> the default one? If a user explicitly specifies it that way then the
> info would be lost. Additionally, def->cpumask here is used on the
> iothread automatically without filling it to the structure if it's used
> so there's no need to do that.
>
Prior review - it's already there - see commit id '938fb12f'.... I'm
fairly certain I was specifically asked to do that...
If it needs to change that should be a separate patch IMO... I think if
you look a few lines above you'll see the same/similar check for vcpupin
>>
>> - if (!(cpumask =
virBitmapFormat(def->cputune.iothreadspin[i]->cpumask)))
>> - goto error;
>> + virBufferAsprintf(buf, "<iothreadpin iothread='%u'
",
>> + def->iothreadids[i]->iothread_id);
>>
>> - virBufferAsprintf(buf, "cpuset='%s'/>\n",
cpumask);
>> - VIR_FREE(cpumask);
>> + if (!(cpumask =
virBitmapFormat(def->iothreadids[i]->cpumask)))
>> + goto error;
>> +
>> + virBufferAsprintf(buf, "cpuset='%s'/>\n",
cpumask);
>> + VIR_FREE(cpumask);
>> + }
>> }
>>
>> for (i = 0; i < def->cputune.nvcpusched; i++) {
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index cc98f3d..c71e1b8 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2057,6 +2057,7 @@ struct _virDomainIOThreadIDDef {
>> bool autofill;
>> unsigned int iothread_id;
>> int thread_id;
>> + virBitmapPtr cpumask;
>> };
>>
>> void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def);
>> @@ -2075,7 +2076,6 @@ struct _virDomainCputune {
>> virDomainPinDefPtr *vcpupin;
>> virDomainPinDefPtr emulatorpin;
>> size_t niothreadspin;
>
> This one should be removed too.
>
yep
>> - virDomainPinDefPtr *iothreadspin;
>>
>> size_t nvcpusched;
>> virDomainThreadSchedParamPtr vcpusched;
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index cdf0aaf..5282449 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -1149,7 +1149,7 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
>> virCgroupPtr cgroup_iothread = NULL;
>> qemuDomainObjPrivatePtr priv = vm->privateData;
>> virDomainDefPtr def = vm->def;
>> - size_t i, j;
>> + size_t i;
>> unsigned long long period = vm->def->cputune.period;
>> long long quota = vm->def->cputune.quota;
>> char *mem_mask = NULL;
>> @@ -1214,18 +1214,11 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
>> /* default cpu masks */
>> if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
>> cpumask = priv->autoCpuset;
>> + else if (def->iothreadids[i]->cpumask)
>> + cpumask = def->iothreadids[i]->cpumask;
>
> This has to be the first case. Even if
> VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO is specified, it cannot override what
> the user configured pinning.
>
OK, but see qemuSetupCgroupForVcpu and qemuSetupCgroupForEmulator...
Vcpu has one way of doing it and Emulator slightly different, but both
check AUTO first.
VCPU checks AUTO first, then sets to default. Then it checks the vcpupin
and resets to whatever is there.
Emulator checks AUTO first, then it checks if emulatorpin is set and
uses that, and finally falls back to default if defined.
Um, I made a mistake when I refactored qemuSetupCgroupForEmulator.
qemuSetupCgroupForVcpu is the right approach.
I followed emulatorpin
If I'm wrong, then emulator and vcpu are wrong, but I think fixing that
is a separate patch.
yes.
>> else
>> cpumask = def->cpumask;
>>
>> - /* specific cpu mask */
>> - for (j = 0; j < def->cputune.niothreadspin; j++) {
>> - if (def->cputune.iothreadspin[j]->id ==
>> - def->iothreadids[i]->iothread_id) {
>> - cpumask = def->cputune.iothreadspin[j]->cpumask;
>> - break;
>> - }
>
> Finally, this can be removed!
>
>> - }
>> -
>> if (cpumask &&
>> qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0)
>> goto cleanup;
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 0f95cc7..ee13d08 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -5894,19 +5894,14 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef,
>> goto cleanup;
>>
>> for (i = 0; i < targetDef->niothreadids; i++) {
>> - virDomainPinDefPtr pininfo;
>> -
>> if (VIR_ALLOC(info_ret[i]) < 0)
>> goto cleanup;
>>
>> /* IOThread ID's are taken from the iothreadids list */
>> info_ret[i]->iothread_id =
targetDef->iothreadids[i]->iothread_id;
>>
>> - /* Initialize the cpumap */
>> - pininfo = virDomainPinFind(targetDef->cputune.iothreadspin,
>> - targetDef->cputune.niothreadspin,
>> -
targetDef->iothreadids[i]->iothread_id);
>> - if (!pininfo) {
>> + cpumask = targetDef->iothreadids[i]->cpumask;
>> + if (!cpumask) {
>> if (targetDef->cpumask) {
>> cpumask = targetDef->cpumask;
>> } else {
>> @@ -5915,8 +5910,6 @@ qemuDomainGetIOThreadsConfig(virDomainDefPtr targetDef,
>> virBitmapSetAll(bitmap);
>> cpumask = bitmap;
>> }
>> - } else {
>> - cpumask = pininfo->cpumask;
>> }
>> if (virBitmapToData(cpumask, &info_ret[i]->cpumap,
>> &info_ret[i]->cpumaplen) < 0)
>> @@ -5999,8 +5992,6 @@ qemuDomainPinIOThread(virDomainPtr dom,
>> virDomainDefPtr persistentDef = NULL;
>> virBitmapPtr pcpumap = NULL;
>> qemuDomainObjPrivatePtr priv;
>> - virDomainPinDefPtr *newIOThreadsPin = NULL;
>> - size_t newIOThreadsPinNum = 0;
>> virCgroupPtr cgroup_iothread = NULL;
>> virObjectEventPtr event = NULL;
>> char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = "";
>> @@ -6050,33 +6041,21 @@ qemuDomainPinIOThread(virDomainPtr dom,
>> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>>
>> virDomainIOThreadIDDefPtr iothrid;
>> + virBitmapPtr cpumask;
>>
>> if (!(iothrid = virDomainIOThreadIDFind(vm->def, iothread_id))) {
>> virReportError(VIR_ERR_INVALID_ARG,
>> - _("iothread value %d not found"),
iothread_id);
>> + _("iothreadid %d not found"),
iothread_id);
>
> I'd compromise between those two.
>
> "iothread %d not foud"
>
OK
>> goto endjob;
>> }
>>
>> - if (vm->def->cputune.iothreadspin) {
>> - newIOThreadsPin =
>> - virDomainPinDefCopy(vm->def->cputune.iothreadspin,
>> - vm->def->cputune.niothreadspin);
>> - if (!newIOThreadsPin)
>> - goto endjob;
>> -
>> - newIOThreadsPinNum = vm->def->cputune.niothreadspin;
>> - } else {
>> - if (VIR_ALLOC(newIOThreadsPin) < 0)
>> - goto endjob;
>> - newIOThreadsPinNum = 0;
>> - }
>> -
>> - if (virDomainPinAdd(&newIOThreadsPin, &newIOThreadsPinNum,
>> - cpumap, maplen, iothread_id) < 0) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("failed to update iothreadspin"));
>> + if (!(cpumask = virBitmapNewData(cpumap, maplen)))
>> goto endjob;
>> - }
>> +
>> + if (!iothrid->cpumask)
>> + vm->def->cputune.niothreadspin++;
>
> This should go. If you add a function that checks whether pinning s
> specified it will not be needed.
>
Yep
>> + virBitmapFree(iothrid->cpumask);
>> + iothrid->cpumask = cpumask;
>>
>> /* Configure the corresponding cpuset cgroup before set affinity. */
>> if (virCgroupHasController(priv->cgroup,
>> @@ -6099,14 +6078,6 @@ qemuDomainPinIOThread(virDomainPtr dom,
>> }
>> }
>>
>> - if (vm->def->cputune.iothreadspin)
>> - virDomainPinDefArrayFree(vm->def->cputune.iothreadspin,
>> - vm->def->cputune.niothreadspin);
>> -
>> - vm->def->cputune.iothreadspin = newIOThreadsPin;
>> - vm->def->cputune.niothreadspin = newIOThreadsPinNum;
>> - newIOThreadsPin = NULL;
>> -
>> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) <
0)
>> goto endjob;
>>
>> @@ -6124,31 +6095,25 @@ 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;
>> - }
>> +
>> + if (!iothrid->cpumask)
>> + persistentDef->cputune.niothreadspin++;
>
> Again, this should not be necessary.
>
yep
>> + virBitmapFree(iothrid->cpumask);
>> + iothrid->cpumask = cpumask;
>>
>> ret = virDomainSaveConfig(cfg->configDir, persistentDef);
>> goto endjob;
>
> The code looks much better now, but this patch has still some nits.
>
> Peter
>
All the following changes to be squashed in:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a191b02..9876557 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2115,6 +2115,19 @@ virDomainPinDefCopy(virDomainPinDefPtr *src, int
npin)
Adding patches in plaintex in thunderbird or other client that breaks
lines makes them unusable so I don't usually bother checking it.
}
Peter