[libvirt] [PATCH 30/34] conf: Fix how iothread scheduler info is stored
John Ferlan
jferlan at redhat.com
Mon Jan 18 23:06:33 UTC 2016
On 01/14/2016 11:27 AM, Peter Krempa wrote:
> Similarly to previous commit change the way how iothread scheduler info
> is stored and clean up a lot of unnecessary code.
(and hours of careful cut-n-paste ;-))
> ---
> src/conf/domain_conf.c | 141 +++++++--------------
> src/conf/domain_conf.h | 8 +-
> src/libvirt_private.syms | 1 -
> src/qemu/qemu_driver.c | 3 -
> src/qemu/qemu_process.c | 39 +-----
> .../qemuxml2xmlout-cputune-iothreadsched.xml | 3 +-
> 6 files changed, 53 insertions(+), 142 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e2dda9a..4ca03d9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2559,10 +2559,6 @@ void virDomainDefFree(virDomainDefPtr def)
>
> virBitmapFree(def->cputune.emulatorpin);
>
> - for (i = 0; i < def->cputune.niothreadsched; i++)
> - virBitmapFree(def->cputune.iothreadsched[i].ids);
> - VIR_FREE(def->cputune.iothreadsched);
> -
> virDomainNumaFree(def->numa);
>
> virSysinfoDefFree(def->sysinfo);
> @@ -14649,25 +14645,26 @@ virDomainVcpuThreadSchedParse(xmlNodePtr node,
> }
>
>
> -static int
> -virDomainThreadSchedParse(xmlNodePtr node,
> - unsigned int minid,
> - unsigned int maxid,
> - const char *name,
> - virDomainThreadSchedParamPtr sp)
> -{
> - if (!(sp->ids = virDomainSchedulerParse(node, name, &sp->policy,
> - &sp->priority)))
> - return -1;
> +static virDomainThreadSchedParamPtr
> +virDomainDefGetIOThreadSched(virDomainDefPtr def,
> + unsigned int iothread)
> +{
> + virDomainIOThreadIDDefPtr iothrinfo;
>
> - if (virBitmapNextSetBit(sp->ids, -1) < minid ||
> - virBitmapLastSetBit(sp->ids) > maxid) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("%ssched bitmap is out of range"), name);
> - return -1;
> - }
> + if (!(iothrinfo = virDomainIOThreadIDFind(def, iothread)))
> + return NULL;
>
> - return 0;
> + return &iothrinfo->sched;
> +}
> +
> +
> +static int
> +virDomainIOThreadSchedParse(xmlNodePtr node,
> + virDomainDefPtr def)
> +{
> + return virDomainThreadSchedParseHelper(node, "iothreads",
Here's somewhere to think about regarding Parse using "iothreads" while
Format uses "iothread"
> + virDomainDefGetIOThreadSched,
> + def);
> }
>
>
> @@ -15215,46 +15212,10 @@ virDomainDefParseXML(xmlDocPtr xml,
> _("cannot extract iothreadsched nodes"));
> goto error;
> }
> - if (n) {
> - if (n > def->niothreadids) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("too many iothreadsched nodes in cputune"));
> - goto error;
> - }
>
> - if (VIR_ALLOC_N(def->cputune.iothreadsched, n) < 0)
> + for (i = 0; i < n; i++) {
> + if (virDomainIOThreadSchedParse(nodes[i], def) < 0)
> goto error;
> - def->cputune.niothreadsched = n;
> -
> - for (i = 0; i < def->cputune.niothreadsched; i++) {
> - ssize_t pos = -1;
> -
> - if (virDomainThreadSchedParse(nodes[i],
> - 1, UINT_MAX,
> - "iothreads",
> - &def->cputune.iothreadsched[i]) < 0)
> - goto error;
> -
> - while ((pos = virBitmapNextSetBit(def->cputune.iothreadsched[i].ids,
> - pos)) > -1) {
> - if (!virDomainIOThreadIDFind(def, pos)) {
> - virReportError(VIR_ERR_XML_DETAIL, "%s",
> - _("iothreadsched attribute 'iothreads' "
> - "uses undefined iothread ids"));
> - goto error;
> - }
> - }
> -
> - for (j = 0; j < i; j++) {
> - if (virBitmapOverlaps(def->cputune.iothreadsched[i].ids,
> - def->cputune.iothreadsched[j].ids)) {
> - virReportError(VIR_ERR_XML_DETAIL, "%s",
> - _("iothreadsched attributes 'iothreads' "
> - "must not overlap"));
> - goto error;
> - }
> - }
> - }
> }
> VIR_FREE(nodes);
>
> @@ -18448,29 +18409,6 @@ virDomainIOThreadIDDel(virDomainDefPtr def,
> }
> }
>
> -void
> -virDomainIOThreadSchedDelId(virDomainDefPtr def,
> - unsigned int iothreadid)
> -{
> - size_t i;
> -
> - if (!def->cputune.iothreadsched || !def->cputune.niothreadsched)
> - return;
> -
> - for (i = 0; i < def->cputune.niothreadsched; i++) {
> - if (virBitmapIsBitSet(def->cputune.iothreadsched[i].ids, iothreadid)) {
> - ignore_value(virBitmapClearBit(def->cputune.iothreadsched[i].ids,
> - iothreadid));
> - if (virBitmapIsAllClear(def->cputune.iothreadsched[i].ids)) {
> - virBitmapFree(def->cputune.iothreadsched[i].ids);
> - VIR_DELETE_ELEMENT(def->cputune.iothreadsched, i,
> - def->cputune.niothreadsched);
> - }
> - return;
> - }
> - }
> -}
> -
>
> static int
> virDomainEventActionDefFormat(virBufferPtr buf,
> @@ -21665,6 +21603,27 @@ virDomainFormatVcpuSchedDef(virDomainDefPtr def,
>
>
> static int
> +virDomainFormatIOThreadSchedDef(virDomainDefPtr def,
> + virBufferPtr buf)
> +{
> + virBitmapPtr allthreadmap;
> + int ret;
> +
> + if (def->niothreadids == 0)
> + return 0;
> +
> + if (!(allthreadmap = virDomainIOThreadIDMap(def)))
> + return -1;
> +
> + ret = virDomainFormatSchedDef(def, buf, "iothread",
> + virDomainDefGetIOThreadSched, allthreadmap);
Just another place to consider if it's decided the 'name' should
commonly used (eg. notes from patch 29 and 28).
> +
> + virBitmapFree(allthreadmap);
> + return ret;
> +}
> +
> +
> +static int
> virDomainCputuneDefFormat(virBufferPtr buf,
> virDomainDefPtr def)
> {
> @@ -21741,22 +21700,8 @@ virDomainCputuneDefFormat(virBufferPtr buf,
> if (virDomainFormatVcpuSchedDef(def, &childrenBuf) < 0)
> goto cleanup;
>
> - for (i = 0; i < def->cputune.niothreadsched; i++) {
> - virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i];
> - char *ids = NULL;
> -
> - if (!(ids = virBitmapFormat(sp->ids)))
> - goto cleanup;
> -
> - virBufferAsprintf(&childrenBuf, "<iothreadsched iothreads='%s' scheduler='%s'",
> - ids, virProcessSchedPolicyTypeToString(sp->policy));
> - VIR_FREE(ids);
> -
> - if (sp->policy == VIR_PROC_POLICY_FIFO ||
> - sp->policy == VIR_PROC_POLICY_RR)
> - virBufferAsprintf(&childrenBuf, " priority='%d'", sp->priority);
> - virBufferAddLit(&childrenBuf, "/>\n");
> - }
> + if (virDomainFormatIOThreadSchedDef(def, &childrenBuf) < 0)
> + goto cleanup;
>
> if (virBufferUse(&childrenBuf)) {
> virBufferAddLit(buf, "<cputune>\n");
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 85740bc..b93835a 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1898,7 +1898,6 @@ typedef enum {
> typedef struct _virDomainThreadSchedParam virDomainThreadSchedParam;
> typedef virDomainThreadSchedParam *virDomainThreadSchedParamPtr;
> struct _virDomainThreadSchedParam {
> - virBitmapPtr ids;
> virProcessSchedPolicy policy;
> int priority;
> };
> @@ -2095,6 +2094,8 @@ struct _virDomainIOThreadIDDef {
> unsigned int iothread_id;
> int thread_id;
> virBitmapPtr cpumask;
> +
> + virDomainThreadSchedParam sched;
> };
>
> void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def);
> @@ -2111,9 +2112,6 @@ struct _virDomainCputune {
> unsigned long long emulator_period;
> long long emulator_quota;
> virBitmapPtr emulatorpin;
> -
> - size_t niothreadsched;
> - virDomainThreadSchedParamPtr iothreadsched;
> };
>
>
> @@ -2124,7 +2122,6 @@ struct _virDomainVcpuInfo {
> bool online;
> virBitmapPtr cpumask;
>
> - /* note: the sched.ids bitmap is unused so it doens't have to be cleared */
> virDomainThreadSchedParam sched;
> };
>
> @@ -2708,7 +2705,6 @@ virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def,
> virBitmapPtr virDomainIOThreadIDMap(virDomainDefPtr def)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id);
> -void virDomainIOThreadSchedDelId(virDomainDefPtr def, unsigned int iothread_id);
>
> unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags);
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 0c84672..cd595b0 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -343,7 +343,6 @@ virDomainIOThreadIDDefFree;
> virDomainIOThreadIDDel;
> virDomainIOThreadIDFind;
> virDomainIOThreadIDMap;
> -virDomainIOThreadSchedDelId;
> virDomainKeyWrapCipherNameTypeFromString;
> virDomainKeyWrapCipherNameTypeToString;
> virDomainLeaseDefFree;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index dfed936..34e82c2 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6043,8 +6043,6 @@ qemuDomainHotplugDelIOThread(virQEMUDriverPtr driver,
>
> virDomainIOThreadIDDel(vm->def, iothread_id);
>
> - virDomainIOThreadSchedDelId(vm->def, iothread_id);
> -
> if (qemuDomainDelCgroupForThread(priv->cgroup,
> VIR_CGROUP_THREAD_IOTHREAD,
> iothread_id) < 0)
> @@ -6134,7 +6132,6 @@ qemuDomainChgIOThread(virQEMUDriverPtr driver,
> }
>
> virDomainIOThreadIDDel(persistentDef, iothread_id);
> - virDomainIOThreadSchedDelId(persistentDef, iothread_id);
> persistentDef->iothreads--;
> }
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 3c1c6d8..abafbb1 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2251,34 +2251,6 @@ qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm)
> return ret;
> }
>
> -/* Set Scheduler parameters for vCPU or I/O threads. */
> -int
> -qemuProcessSetSchedParams(int id,
> - pid_t pid,
> - size_t nsp,
> - virDomainThreadSchedParamPtr sp)
> -{
> - bool val = false;
> - size_t i = 0;
> - virDomainThreadSchedParamPtr s = NULL;
> -
> - for (i = 0; i < nsp; i++) {
> - if (virBitmapGetBit(sp[i].ids, id, &val) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Cannot get bit from bitmap"));
> - }
> - if (val) {
> - s = &sp[i];
> - break;
> - }
> - }
> -
> - if (!s)
> - return 0;
> -
> - return virProcessSetScheduler(pid, s->policy, s->priority);
> -}
> -
> static int
> qemuProcessSetSchedulers(virDomainObjPtr vm)
> {
> @@ -2297,10 +2269,13 @@ qemuProcessSetSchedulers(virDomainObjPtr vm)
> }
>
> for (i = 0; i < vm->def->niothreadids; i++) {
> - if (qemuProcessSetSchedParams(vm->def->iothreadids[i]->iothread_id,
> - vm->def->iothreadids[i]->thread_id,
> - vm->def->cputune.niothreadsched,
> - vm->def->cputune.iothreadsched) < 0)
> + virDomainIOThreadIDDefPtr info = vm->def->iothreadids[i];
> +
> + if (info->sched.policy == VIR_PROC_POLICY_NONE)
> + continue;
> +
> + if (virProcessSetScheduler(info->thread_id, info->sched.policy,
> + info->sched.priority) < 0)
> return -1;
> }
>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml
> index 9f61336..01f1af9 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-cputune-iothreadsched.xml
> @@ -13,8 +13,7 @@
> <vcpupin vcpu='1' cpuset='1'/>
> <emulatorpin cpuset='1'/>
> <vcpusched vcpus='0-1' scheduler='fifo' priority='1'/>
> - <iothreadsched iothreads='1,3' scheduler='batch'/>
> - <iothreadsched iothreads='2' scheduler='batch'/>
> + <iothreadsched iothreads='1-3' scheduler='batch'/>
This undoes commit id '8680ea974' Not sure why there were two 'batch'
entries using different iothreads values...
Not a problem, but I'm wondering why a DO_TEST_DIFFERENT was used now.
John
> </cputune>
> <os>
> <type arch='i686' machine='pc'>hvm</type>
>
More information about the libvir-list
mailing list