8:57 a.m.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1178986
Martin Kletzander (3):
util: Add virProcessSetScheduler() function for scheduler settings
docs, schema, conf: Add support for setting scheduler parameters of
guest threads
qemu: Add support for setting vCPU and I/O thread scheduler setting
docs/formatdomain.html.in | 13 +++
docs/schemas/domaincommon.rng | 33 ++++++
src/conf/domain_conf.c | 137 +++++++++++++++++++++++-
src/conf/domain_conf.h | 24 +++++
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 7 +-
src/qemu/qemu_process.c | 76 ++++++++++++-
src/qemu/qemu_process.h | 5 +-
src/util/virprocess.c | 46 +++++++-
src/util/virprocess.h | 6 +-
tests/qemuxml2argvdata/qemuxml2argv-cputune.xml | 4 +
11 files changed, 345 insertions(+), 7 deletions(-)
--
2.2.1
8:57 a.m.
New subject: [libvirt] [PATCH 1/3] util: Add virProcessSetScheduler() function for scheduler settings
This function uses sched_setscheduler() function so it works with
processes and threads as well (even threads not created by us, which is
what we'll need in the future).
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virprocess.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
src/util/virprocess.h | 6 +++++-
3 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fb5d003..de3476e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1882,6 +1882,7 @@ virProcessSetMaxFiles;
virProcessSetMaxMemLock;
virProcessSetMaxProcesses;
virProcessSetNamespaces;
+virProcessSetScheduler;
virProcessTranslateStatus;
virProcessWait;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index d0a1500..85da5e8 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1,7 +1,7 @@
/*
* virprocess.c: interaction with processes
*
- * Copyright (C) 2010-2014 Red Hat, Inc.
+ * Copyright (C) 2010-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -32,7 +32,6 @@
# include <sys/time.h>
# include <sys/resource.h>
#endif
-#include <sched.h>
#if defined(__FreeBSD__) || HAVE_BSD_CPU_AFFINITY
# include <sys/param.h>
@@ -1052,3 +1051,46 @@ virProcessExitWithStatus(int status)
}
exit(value);
}
+
+int
+virProcessSetScheduler(pid_t pid, int policy, int priority)
+{
+ struct sched_param param = {0};
+
+ VIR_DEBUG("pid=%d, policy=%d, priority=%u", pid, policy, priority);
+
+ if (policy == SCHED_FIFO || policy == SCHED_RR) {
+ int min = 0;
+ int max = 0;
+
+ if ((min = sched_get_priority_min(policy)) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Cannot get minimum scheduler priority
value"));
+ return -1;
+ }
+
+ if ((max = sched_get_priority_max(policy)) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Cannot get maximum scheduler priority
value"));
+ return -1;
+ }
+
+ if (priority < min || priority > max) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Scheduler priority %d out of range [%d, %d]"),
+ priority, min, max);
+ return -1;
+ }
+
+ param.sched_priority = priority;
+ }
+
+ if (sched_setscheduler(pid, policy, ¶m) < 0) {
+ virReportSystemError(errno,
+ _("Cannot set scheduler parameters for pid %d"),
+ pid);
+ return -1;
+ }
+
+ return 0;
+}
diff --git a/src/util/virprocess.h b/src/util/virprocess.h
index bcaede5..34b15a4 100644
--- a/src/util/virprocess.h
+++ b/src/util/virprocess.h
@@ -1,7 +1,7 @@
/*
* virprocess.h: interaction with processes
*
- * Copyright (C) 2010-2014 Red Hat, Inc.
+ * Copyright (C) 2010-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -23,6 +23,7 @@
# define __VIR_PROCESS_H__
# include <sys/types.h>
+# include <sched.h>
# include "internal.h"
# include "virbitmap.h"
@@ -73,4 +74,7 @@ typedef int (*virProcessNamespaceCallback)(pid_t pid, void *opaque);
int virProcessRunInMountNamespace(pid_t pid,
virProcessNamespaceCallback cb,
void *opaque);
+
+int virProcessSetScheduler(pid_t pid, int policy, int priority);
+
#endif /* __VIR_PROCESS_H__ */
--
2.2.1
4:12 a.m.
New subject: [libvirt] [PATCH 1/3] util: Add virProcessSetScheduler() function for scheduler settings
On 01/13/2015 02:57 AM, Martin Kletzander wrote:
This function uses sched_setscheduler() function so it works with
processes and threads as well (even threads not created by us, which is
what we'll need in the future).
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virprocess.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
src/util/virprocess.h | 6 +++++-
3 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fb5d003..de3476e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1882,6 +1882,7 @@ virProcessSetMaxFiles;
virProcessSetMaxMemLock;
virProcessSetMaxProcesses;
virProcessSetNamespaces;
+virProcessSetScheduler;
virProcessTranslateStatus;
virProcessWait;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index d0a1500..85da5e8 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1,7 +1,7 @@
/*
* virprocess.c: interaction with processes
*
- * Copyright (C) 2010-2014 Red Hat, Inc.
+ * Copyright (C) 2010-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -32,7 +32,6 @@
# include <sys/time.h>
# include <sys/resource.h>
#endif
-#include <sched.h>
hmm.. moving to virprocess.h....
#if defined(__FreeBSD__) || HAVE_BSD_CPU_AFFINITY
# include <sys/param.h>
@@ -1052,3 +1051,46 @@ virProcessExitWithStatus(int status)
}
exit(value);
}
+
+int
+virProcessSetScheduler(pid_t pid, int policy, int priority)
+{
+ struct sched_param param = {0};
+
+ VIR_DEBUG("pid=%d, policy=%d, priority=%u", pid, policy, priority);
+
+ if (policy == SCHED_FIFO || policy == SCHED_RR) {
+ int min = 0;
+ int max = 0;
+
+ if ((min = sched_get_priority_min(policy)) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Cannot get minimum scheduler priority
value"));
+ return -1;
+ }
+
+ if ((max = sched_get_priority_max(policy)) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Cannot get maximum scheduler priority
value"));
+ return -1;
+ }
+
+ if (priority < min || priority > max) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Scheduler priority %d out of range [%d, %d]"),
+ priority, min, max);
+ return -1;
+ }
+
+ param.sched_priority = priority;
+ }
+
+ if (sched_setscheduler(pid, policy, ¶m) < 0) {
+ virReportSystemError(errno,
+ _("Cannot set scheduler parameters for pid %d"),
+ pid);
+ return -1;
+ }
+
+ return 0;
+}
diff --git a/src/util/virprocess.h b/src/util/virprocess.h
index bcaede5..34b15a4 100644
--- a/src/util/virprocess.h
+++ b/src/util/virprocess.h
@@ -1,7 +1,7 @@
/*
* virprocess.h: interaction with processes
*
- * Copyright (C) 2010-2014 Red Hat, Inc.
+ * Copyright (C) 2010-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -23,6 +23,7 @@
# define __VIR_PROCESS_H__
# include <sys/types.h>
+# include <sched.h>
Does moving this into here work for other build platforms? Is it
required - it doesn't seem so given all that changes doesn't include
anything sched.h specific.
John
# include "internal.h"
# include "virbitmap.h"
@@ -73,4 +74,7 @@ typedef int (*virProcessNamespaceCallback)(pid_t pid, void *opaque);
int virProcessRunInMountNamespace(pid_t pid,
virProcessNamespaceCallback cb,
void *opaque);
+
+int virProcessSetScheduler(pid_t pid, int policy, int priority);
+
#endif /* __VIR_PROCESS_H__ */
8:35 a.m.
New subject: [libvirt] [PATCH 1/3] util: Add virProcessSetScheduler() function for scheduler settings
On Mon, Jan 19, 2015 at 10:12:34PM -0500, John Ferlan wrote:
On 01/13/2015 02:57 AM, Martin Kletzander wrote:
> This function uses sched_setscheduler() function so it works with
> processes and threads as well (even threads not created by us, which is
> what we'll need in the future).
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virprocess.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
> src/util/virprocess.h | 6 +++++-
> 3 files changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fb5d003..de3476e 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1882,6 +1882,7 @@ virProcessSetMaxFiles;
> virProcessSetMaxMemLock;
> virProcessSetMaxProcesses;
> virProcessSetNamespaces;
> +virProcessSetScheduler;
> virProcessTranslateStatus;
> virProcessWait;
>
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index d0a1500..85da5e8 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1,7 +1,7 @@
> /*
> * virprocess.c: interaction with processes
> *
> - * Copyright (C) 2010-2014 Red Hat, Inc.
> + * Copyright (C) 2010-2015 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -32,7 +32,6 @@
> # include <sys/time.h>
> # include <sys/resource.h>
> #endif
> -#include <sched.h>
hmm.. moving to virprocess.h....
>
> #if defined(__FreeBSD__) || HAVE_BSD_CPU_AFFINITY
> # include <sys/param.h>
> @@ -1052,3 +1051,46 @@ virProcessExitWithStatus(int status)
> }
> exit(value);
> }
> +
> +int
> +virProcessSetScheduler(pid_t pid, int policy, int priority)
> +{
> + struct sched_param param = {0};
> +
> + VIR_DEBUG("pid=%d, policy=%d, priority=%u", pid, policy, priority);
> +
> + if (policy == SCHED_FIFO || policy == SCHED_RR) {
> + int min = 0;
> + int max = 0;
> +
> + if ((min = sched_get_priority_min(policy)) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Cannot get minimum scheduler priority
value"));
> + return -1;
> + }
> +
> + if ((max = sched_get_priority_max(policy)) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Cannot get maximum scheduler priority
value"));
> + return -1;
> + }
> +
> + if (priority < min || priority > max) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Scheduler priority %d out of range [%d,
%d]"),
> + priority, min, max);
> + return -1;
> + }
> +
> + param.sched_priority = priority;
> + }
> +
> + if (sched_setscheduler(pid, policy, ¶m) < 0) {
> + virReportSystemError(errno,
> + _("Cannot set scheduler parameters for pid
%d"),
> + pid);
> + return -1;
> + }
> +
> + return 0;
> +}
> diff --git a/src/util/virprocess.h b/src/util/virprocess.h
> index bcaede5..34b15a4 100644
> --- a/src/util/virprocess.h
> +++ b/src/util/virprocess.h
> @@ -1,7 +1,7 @@
> /*
> * virprocess.h: interaction with processes
> *
> - * Copyright (C) 2010-2014 Red Hat, Inc.
> + * Copyright (C) 2010-2015 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -23,6 +23,7 @@
> # define __VIR_PROCESS_H__
>
> # include <sys/types.h>
> +# include <sched.h>
Does moving this into here work for other build platforms? Is it
required - it doesn't seem so given all that changes doesn't include
anything sched.h specific.
I moved this so that whoever includes this file has SCHED_* macros
automatically available. Unfortunately, this breaks the build on
mingw, you're right. I'll try a different approach. Or maybe just
guarding this include by ifndef WIN32 would work since we can't (and
won't) change the scheduler on windows. Otherwise the translation
needs to be done in util/ and the enum has to be renamed and moved
here as well so that this not depend on conf/.
John
>
> # include "internal.h"
> # include "virbitmap.h"
> @@ -73,4 +74,7 @@ typedef int (*virProcessNamespaceCallback)(pid_t pid, void
*opaque);
> int virProcessRunInMountNamespace(pid_t pid,
> virProcessNamespaceCallback cb,
> void *opaque);
> +
> +int virProcessSetScheduler(pid_t pid, int policy, int priority);
> +
> #endif /* __VIR_PROCESS_H__ */
>
8:57 a.m.
New subject: [libvirt] [PATCH 2/3] docs, schema, conf: Add support for setting scheduler parameters of guest threads
In order for QEMU vCPU (and other) threads to run with RT scheduler,
libvirt needs to take care of that so QEMU doesn't have to run privileged.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1178986
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
docs/formatdomain.html.in | 13 +++
docs/schemas/domaincommon.rng | 33 ++++++
src/conf/domain_conf.c | 137 +++++++++++++++++++++++-
src/conf/domain_conf.h | 24 +++++
tests/qemuxml2argvdata/qemuxml2argv-cputune.xml | 4 +
5 files changed, 210 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index d8c31e0..60dad76 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -550,6 +550,8 @@
<quota>-1</quota>
<emulator_period>1000000</emulator_period>
<emulator_quota>-1</emulator_quota>
+ <vcpusched scheduler='fifo' priority='1'/>
+ <iothreadsched scheduler='idle'/>
</cputune>
...
</domain>
@@ -656,6 +658,17 @@
<span class="since">Only QEMU driver support since
0.10.0</span>
</dd>
+ <dt><code>vcpusched</code> and
<code>iothreadsched</code></dt>
+ <dd>
+ The optional <code>vcpusched</code> element specifies the scheduler
+ (values <code>batch</code>, <code>idle</code>,
<code>fifo</code>,
+ <code>rr</code> and <code>other</code>, which is default
and ignored)
+ for all vCPU/IOThread threads. For real-time schedulers
+ (<code>fifo</code>, <code>rr</code>), priority can be
specified as
+ well. The value range for the priority depends on the host kernel.
+ <span class="since">Since 1.2.12</span>
+ </dd>
+
</dl>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index d250e6f..6653737 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -815,10 +815,43 @@
</attribute>
</element>
</zeroOrMore>
+ <zeroOrMore>
+ <element name="vcpusched">
+ <attribute name="vcpu">
+ <ref name="vcpuid"/>
+ </attribute>
+ <ref name="schedparam"/>
+ </element>
+ </zeroOrMore>
+ <zeroOrMore>
+ <element name="iothreadsched">
+ <attribute name="iothread">
+ <ref name="unsignedInt"/>
+ </attribute>
+ <ref name="schedparam"/>
+ </element>
+ </zeroOrMore>
</interleave>
</element>
</define>
+ <define name="schedparam">
+ <attribute name="scheduler">
+ <choice>
+ <value>other</value>
+ <value>batch</value>
+ <value>idle</value>
+ <value>fifo</value>
+ <value>rr</value>
+ </choice>
+ </attribute>
+ <optional>
+ <attribute name="priority">
+ <ref name="unsignedShort"/>
+ </attribute>
+ </optional>
+ </define>
+
<!-- All the NUMA related tunables would go in the numatune -->
<define name="numatune">
<element name="numatune">
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 96d80a9..16adbf1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -795,6 +795,13 @@ VIR_ENUM_IMPL(virDomainLoader,
"rom",
"pflash")
+VIR_ENUM_IMPL(virDomainThreadSched, VIR_DOMAIN_THREAD_SCHED_LAST,
+ "other", /* default */
+ "batch",
+ "idle",
+ "fifo",
+ "rr")
+
/* Internal mapping: subset of block job types that can be present in
* <mirror> XML (remaining types are not two-phase). */
VIR_ENUM_DECL(virDomainBlockJob)
@@ -2260,6 +2267,9 @@ void virDomainDefFree(virDomainDefPtr def)
virDomainVcpuPinDefArrayFree(def->cputune.iothreadspin,
def->cputune.niothreadspin);
+ VIR_FREE(def->cputune.vcpusched);
+ VIR_FREE(def->cputune.iothreadsched);
+
virDomainNumatuneFree(def->numatune);
virSysinfoDefFree(def->sysinfo);
@@ -12653,6 +12663,65 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
return ret;
}
+static int
+virDomainThreadSchedParse(xmlNodePtr node,
+ unsigned int maxid,
+ const char *name,
+ virDomainThreadSchedParamPtr sp)
+{
+ char *tmp = NULL;
+ int sched = 0;
+
+ tmp = virXMLPropString(node, name);
+ if (!tmp) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("invalid value for attribute %s"), name);
+ goto error;
+ }
+
+ if (virStrToLong_uip(tmp, NULL, 10, &sp->id) < 0 ||
+ sp->id >= maxid) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("vcpu must be positive integer smaller than "
+ "maximum of vcpus, not '%s'"),
+ tmp);
+ goto error;
+ }
+ VIR_FREE(tmp);
+
+ tmp = virXMLPropString(node, "scheduler");
+ if (tmp) {
+ if ((sched = virDomainThreadSchedTypeFromString(tmp)) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid vcpu scheduler: '%s'"), tmp);
+ goto error;
+ }
+ sp->scheduler = sched;
+
+ VIR_FREE(tmp);
+ if (sp->scheduler >= VIR_DOMAIN_THREAD_SCHED_FIFO) {
+ tmp = virXMLPropString(node, "priority");
+ if (!tmp) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing scheduler priority"));
+ goto error;
+ }
+ if (virStrToLong_i(tmp, NULL, 10, &sp->priority) < 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid value for element priority"));
+ goto error;
+ }
+ VIR_FREE(tmp);
+ }
+ }
+
+ return 0;
+
+ error:
+ VIR_FREE(tmp);
+ return -1;
+}
+
static virDomainDefPtr
virDomainDefParseXML(xmlDocPtr xml,
xmlNodePtr root,
@@ -13201,6 +13270,51 @@ virDomainDefParseXML(xmlDocPtr xml,
}
VIR_FREE(nodes);
+ if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0)
{
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot extract vcpusched nodes"));
+ goto error;
+ }
+ if (n) {
+ if (n > def->maxvcpus) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("too many vcpusched nodes in cputune"));
+ goto error;
+ }
+
+ if (VIR_ALLOC_N(def->cputune.vcpusched, n) < 0)
+ goto error;
+ def->cputune.nvcpusched = n;
+
+ for (i = 0; i < def->cputune.nvcpusched; i++) {
+ if (virDomainThreadSchedParse(nodes[i], def->maxvcpus, "vcpu",
+ &def->cputune.vcpusched[i]) < 0)
+ goto error;
+ }
+ }
+
+ if ((n = virXPathNodeSet("./cputune/iothreadsched", ctxt, &nodes)) <
0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot extract iothreadsched nodes"));
+ goto error;
+ }
+ if (n) {
+ if (n > def->iothreads) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("too many iothreadsched nodes in cputune"));
+ goto error;
+ }
+
+ if (VIR_ALLOC_N(def->cputune.iothreadsched, n) < 0)
+ goto error;
+ def->cputune.niothreadsched = n;
+
+ for (i = 0; i < def->cputune.niothreadsched; i++) {
+ if (virDomainThreadSchedParse(nodes[i], def->iothreads + 1,
"iothread",
+ &def->cputune.iothreadsched[i]) < 0)
+ goto error;
+ }
+ }
/* analysis of cpu handling */
if ((node = virXPathNode("./cpu[1]", ctxt)) != NULL) {
@@ -19519,7 +19633,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
def->cputune.period || def->cputune.quota ||
def->cputune.emulatorpin ||
def->cputune.emulator_period || def->cputune.emulator_quota ||
- def->cputune.niothreadspin) {
+ def->cputune.niothreadspin ||
+ def->cputune.vcpusched || def->cputune.iothreadsched) {
virBufferAddLit(buf, "<cputune>\n");
cputune = true;
}
@@ -19592,6 +19707,26 @@ virDomainDefFormatInternal(virDomainDefPtr def,
VIR_FREE(cpumask);
}
+ for (i = 0; i < def->cputune.nvcpusched; i++) {
+ virDomainThreadSchedParamPtr sp = &def->cputune.vcpusched[i];
+ virBufferAsprintf(buf, "<vcpusched vcpu='%d'
scheduler='%s'",
+ sp->id,
+ virDomainThreadSchedTypeToString(sp->scheduler));
+ if (sp->priority)
+ virBufferAsprintf(buf, " priority='%d'", sp->priority);
+ virBufferAddLit(buf, "/>\n");
+ }
+
+ for (i = 0; i < def->cputune.niothreadsched; i++) {
+ virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i];
+ virBufferAsprintf(buf, "<iothreadsched iothread='%d'
scheduler='%s'",
+ sp->id,
+ virDomainThreadSchedTypeToString(sp->scheduler));
+ if (sp->priority)
+ virBufferAsprintf(buf, " priority='%d'", sp->priority);
+ virBufferAddLit(buf, "/>\n");
+ }
+
virBufferAdjustIndent(buf, -2);
if (cputune)
virBufferAddLit(buf, "</cputune>\n");
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 5cc42d1..636e2d1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1815,6 +1815,24 @@ typedef enum {
VIR_DOMAIN_CPU_PLACEMENT_MODE_LAST
} virDomainCpuPlacementMode;
+typedef enum {
+ VIR_DOMAIN_THREAD_SCHED_OTHER = 0,
+ VIR_DOMAIN_THREAD_SCHED_BATCH,
+ VIR_DOMAIN_THREAD_SCHED_IDLE,
+ VIR_DOMAIN_THREAD_SCHED_FIFO,
+ VIR_DOMAIN_THREAD_SCHED_RR,
+
+ VIR_DOMAIN_THREAD_SCHED_LAST
+} virDomainThreadSched;
+
+typedef struct _virDomainThreadSchedParam virDomainThreadSchedParam;
+typedef virDomainThreadSchedParam *virDomainThreadSchedParamPtr;
+struct _virDomainThreadSchedParam {
+ unsigned int id;
+ virDomainThreadSched scheduler;
+ int priority;
+};
+
typedef struct _virDomainTimerCatchupDef virDomainTimerCatchupDef;
typedef virDomainTimerCatchupDef *virDomainTimerCatchupDefPtr;
struct _virDomainTimerCatchupDef {
@@ -2002,6 +2020,11 @@ struct _virDomainCputune {
virDomainVcpuPinDefPtr emulatorpin;
size_t niothreadspin;
virDomainVcpuPinDefPtr *iothreadspin;
+
+ size_t nvcpusched;
+ virDomainThreadSchedParamPtr vcpusched;
+ size_t niothreadsched;
+ virDomainThreadSchedParamPtr iothreadsched;
};
typedef struct _virDomainBlkiotune virDomainBlkiotune;
@@ -2807,6 +2830,7 @@ VIR_ENUM_DECL(virDomainRNGModel)
VIR_ENUM_DECL(virDomainRNGBackend)
VIR_ENUM_DECL(virDomainTPMModel)
VIR_ENUM_DECL(virDomainTPMBackend)
+VIR_ENUM_DECL(virDomainThreadSched)
/* from libvirt.h */
VIR_ENUM_DECL(virDomainState)
VIR_ENUM_DECL(virDomainNostateReason)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
b/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
index 813d201..243092b 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
@@ -4,6 +4,7 @@
<memory unit='KiB'>219136</memory>
<currentMemory unit='KiB'>219136</currentMemory>
<vcpu placement='static'>2</vcpu>
+ <iothreads>1</iothreads>
<cputune>
<shares>2048</shares>
<period>1000000</period>
@@ -11,6 +12,9 @@
<vcpupin vcpu='0' cpuset='0'/>
<vcpupin vcpu='1' cpuset='1'/>
<emulatorpin cpuset='1'/>
+ <vcpusched vcpu='0' scheduler='fifo' priority='1'/>
+ <vcpusched vcpu='1' scheduler='batch'/>
+ <iothreadsched iothread='0' scheduler='idle'/>
</cputune>
<os>
<type arch='i686' machine='pc'>hvm</type>
--
2.2.1
4:23 a.m.
New subject: [libvirt] [PATCH 2/3] docs, schema, conf: Add support for setting scheduler parameters of guest threads
On 01/13/2015 02:57 AM, Martin Kletzander wrote:
In order for QEMU vCPU (and other) threads to run with RT scheduler,
libvirt needs to take care of that so QEMU doesn't have to run privileged.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1178986
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
docs/formatdomain.html.in | 13 +++
docs/schemas/domaincommon.rng | 33 ++++++
src/conf/domain_conf.c | 137 +++++++++++++++++++++++-
src/conf/domain_conf.h | 24 +++++
tests/qemuxml2argvdata/qemuxml2argv-cputune.xml | 4 +
5 files changed, 210 insertions(+), 1 deletion(-)
You'll need to rebase - I couldn't get this to apply... So review is
strictly visual and memory based.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index d8c31e0..60dad76 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -550,6 +550,8 @@
<quota>-1</quota>
<emulator_period>1000000</emulator_period>
<emulator_quota>-1</emulator_quota>
+ <vcpusched scheduler='fifo' priority='1'/>
+ <iothreadsched scheduler='idle'/>
</cputune>
...
</domain>
@@ -656,6 +658,17 @@
<span class="since">Only QEMU driver support since
0.10.0</span>
</dd>
+ <dt><code>vcpusched</code> and
<code>iothreadsched</code></dt>
+ <dd>
+ The optional <code>vcpusched</code> element specifies the scheduler
+ (values <code>batch</code>, <code>idle</code>,
<code>fifo</code>,
+ <code>rr</code> and <code>other</code>, which is default
and ignored)
+ for all vCPU/IOThread threads. For real-time schedulers
+ (<code>fifo</code>, <code>rr</code>), priority can be
specified as
+ well. The value range for the priority depends on the host kernel.
+ <span class="since">Since 1.2.12</span>
+ </dd>
+
Why would someone use batch or idle? They're not described.
Upon first reading, I assumed one setting was good for *all* vcpu's and
iothread's; however, later on there's a 'vcpuid' and 'iothreadid'
defined in the rng file. Does it even make sense to have 2 vCPUs using
'fifo' while another 2 are using 'ff'? If not, then why allow choosing
the id? (it's the "for all vCPU/IOThread threads" language that gave me
the impression).
If you did desire different models, then why not model after the
'cpuset' with respect to being able to choose a range. If someone has 16
vCPU's are they going to be happy having to make 16 entries or just one
that indicates 0-15. You should be able to borrow/reuse existing code
for that parsing cpuset...
Would listing priority with batch/idle result in error or be ignored?
Testing will try it and they will flag it unless you say it'll be ignored.
I wouldn't list "other" - it's confusing. I don't think other XML
lists
other. Why would someone list "other" - they would only have this if
they wanted one of the models.
</dl>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index d250e6f..6653737 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -815,10 +815,43 @@
</attribute>
</element>
</zeroOrMore>
+ <zeroOrMore>
+ <element name="vcpusched">
+ <attribute name="vcpu">
+ <ref name="vcpuid"/>
+ </attribute>
+ <ref name="schedparam"/>
+ </element>
+ </zeroOrMore>
+ <zeroOrMore>
+ <element name="iothreadsched">
+ <attribute name="iothread">
+ <ref name="unsignedInt"/>
+ </attribute>
+ <ref name="schedparam"/>
+ </element>
+ </zeroOrMore>
so essentially :
<vcpusched vcpuid=n scheduler='string' priority=n/>
and
<iothreadsched iothread=n scheduler='string' priority=n/>
</interleave>
</element>
</define>
+ <define name="schedparam">
+ <attribute name="scheduler">
+ <choice>
+ <value>other</value>
+ <value>batch</value>
+ <value>idle</value>
+ <value>fifo</value>
+ <value>rr</value>
+ </choice>
+ </attribute>
+ <optional>
+ <attribute name="priority">
+ <ref name="unsignedShort"/>
+ </attribute>
+ </optional>
+ </define>
+
leaving other here is OK... could also go with "none"
<!-- All the NUMA related tunables would go in the numatune
-->
<define name="numatune">
<element name="numatune">
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 96d80a9..16adbf1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -795,6 +795,13 @@ VIR_ENUM_IMPL(virDomainLoader,
"rom",
"pflash")
+VIR_ENUM_IMPL(virDomainThreadSched, VIR_DOMAIN_THREAD_SCHED_LAST,
+ "other", /* default */
+ "batch",
+ "idle",
+ "fifo",
+ "rr")
+
other a/k/a none
/* Internal mapping: subset of block job types that can be present
in
* <mirror> XML (remaining types are not two-phase). */
VIR_ENUM_DECL(virDomainBlockJob)
@@ -2260,6 +2267,9 @@ void virDomainDefFree(virDomainDefPtr def)
virDomainVcpuPinDefArrayFree(def->cputune.iothreadspin,
def->cputune.niothreadspin);
+ VIR_FREE(def->cputune.vcpusched);
+ VIR_FREE(def->cputune.iothreadsched);
+
virDomainNumatuneFree(def->numatune);
virSysinfoDefFree(def->sysinfo);
@@ -12653,6 +12663,65 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
return ret;
}
+static int
+virDomainThreadSchedParse(xmlNodePtr node,
+ unsigned int maxid,
+ const char *name,
+ virDomainThreadSchedParamPtr sp)
+{
+ char *tmp = NULL;
+ int sched = 0;
+
+ tmp = virXMLPropString(node, name);
+ if (!tmp) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("invalid value for attribute %s"), name);
+ goto error;
+ }
+
+ if (virStrToLong_uip(tmp, NULL, 10, &sp->id) < 0 ||
+ sp->id >= maxid) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("vcpu must be positive integer smaller than "
+ "maximum of vcpus, not '%s'"),
+ tmp);
Hmm.... this is challenging... vcpus are numbered 0..n-1; however,
iothreads are numbered 1..n
Use the 'name' instead of 'vcpu' _("Invalid %ssched value
'%s'", name,
tmp).
However, I think the range check should be in the caller. Then again,
why are we allowing someone to choose per cpu?
+ goto error;
+ }
+ VIR_FREE(tmp);
+
+ tmp = virXMLPropString(node, "scheduler");
+ if (tmp) {
+ if ((sched = virDomainThreadSchedTypeFromString(tmp)) < 0) {
Why accept "other"? If you choose to not accept "other", then this
would
be <= 0... That way on output the only way it gets written is if it's
been set to something.
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid vcpu scheduler: '%s'"), tmp);
change 'vcpu scheduler: '%s' to %ssched scheduler='%s' value, where
the
first %s is the 'name' argument.
+ goto error;
+ }
+ sp->scheduler = sched;
+
+ VIR_FREE(tmp);
+ if (sp->scheduler >= VIR_DOMAIN_THREAD_SCHED_FIFO) {
Oh - this is dangerous... Today you've set them up that way, but if some
day something is added after RR that doesn't support priority, then
there'll be an error. Be specific here.
+ tmp = virXMLPropString(node, "priority");
+ if (!tmp) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("missing scheduler priority"));
+ goto error;
+ }
Priority is listed as optional in the rng, but this implies it must be
provided. Also you'll need to document that it's only parsed for fifo/rr...
+ if (virStrToLong_i(tmp, NULL, 10, &sp->priority)
< 0) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("invalid value for element priority"));
+ goto error;
+ }
+ VIR_FREE(tmp);
+ }
+ }
+
+ return 0;
+
+ error:
+ VIR_FREE(tmp);
+ return -1;
+}
+
static virDomainDefPtr
virDomainDefParseXML(xmlDocPtr xml,
xmlNodePtr root,
@@ -13201,6 +13270,51 @@ virDomainDefParseXML(xmlDocPtr xml,
}
VIR_FREE(nodes);
+ if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0)
{
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot extract vcpusched nodes"));
+ goto error;
+ }
+ if (n) {
+ if (n > def->maxvcpus) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("too many vcpusched nodes in cputune"));
+ goto error;
+ }
+
+ if (VIR_ALLOC_N(def->cputune.vcpusched, n) < 0)
+ goto error;
+ def->cputune.nvcpusched = n;
+
+ for (i = 0; i < def->cputune.nvcpusched; i++) {
+ if (virDomainThreadSchedParse(nodes[i], def->maxvcpus, "vcpu",
+ &def->cputune.vcpusched[i]) < 0)
+ goto error;
+ }
linear algorithm, but not linear number to vcpusched... Also, could
follow the 'cpuset' algorithm...
+ }
VIR_FREE(nodes); ?
> +
> + if ((n = virXPathNodeSet("./cputune/iothreadsched", ctxt, &nodes))
< 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot extract iothreadsched nodes"));
> + goto error;
+ }
> + if (n) {
> + if (n > def->iothreads) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("too many iothreadsched nodes in cputune"));
> + goto error;
> + }
> +
> + if (VIR_ALLOC_N(def->cputune.iothreadsched, n) < 0)
> + goto error;
> + def->cputune.niothreadsched = n;
> +
> + for (i = 0; i < def->cputune.niothreadsched; i++) {
> + if (virDomainThreadSchedParse(nodes[i], def->iothreads + 1,
"iothread",
> + &def->cputune.iothreadsched[i])
< 0)
> + goto error;
> + }
+ }
VIR_FREE(nodes); ?
>
> /* analysis of cpu handling */
> if ((node = virXPathNode("./cpu[1]", ctxt)) != NULL) {
> @@ -19519,7 +19633,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> def->cputune.period || def->cputune.quota ||
> def->cputune.emulatorpin ||
> def->cputune.emulator_period || def->cputune.emulator_quota ||
> - def->cputune.niothreadspin) {
> + def->cputune.niothreadspin ||
> + def->cputune.vcpusched || def->cputune.iothreadsched) {
> virBufferAddLit(buf, "<cputune>\n");
> cputune = true;
> }
> @@ -19592,6 +19707,26 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> VIR_FREE(cpumask);
> }
>
> + for (i = 0; i < def->cputune.nvcpusched; i++) {
> + virDomainThreadSchedParamPtr sp = &def->cputune.vcpusched[i];
> + virBufferAsprintf(buf, "<vcpusched vcpu='%d'
scheduler='%s'",
> + sp->id,
> + virDomainThreadSchedTypeToString(sp->scheduler));
> + if (sp->priority)
> + virBufferAsprintf(buf, " priority='%d'",
sp->priority);
> + virBufferAddLit(buf, "/>\n");
+ }
> +
> + for (i = 0; i < def->cputune.niothreadsched; i++) {
> + virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i];
> + virBufferAsprintf(buf, "<iothreadsched iothread='%d'
scheduler='%s'",
> + sp->id,
> + virDomainThreadSchedTypeToString(sp->scheduler));
> + if (sp->priority)
> + virBufferAsprintf(buf, " priority='%d'",
sp->priority);
> + virBufferAddLit(buf, "/>\n");
+ }
> +
> virBufferAdjustIndent(buf, -2);
> if (cputune)
> virBufferAddLit(buf, "</cputune>\n");
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 5cc42d1..636e2d1 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1815,6 +1815,24 @@ typedef enum {
> VIR_DOMAIN_CPU_PLACEMENT_MODE_LAST
> } virDomainCpuPlacementMode;
>
> +typedef enum {
> + VIR_DOMAIN_THREAD_SCHED_OTHER = 0,
> + VIR_DOMAIN_THREAD_SCHED_BATCH,
> + VIR_DOMAIN_THREAD_SCHED_IDLE,
> + VIR_DOMAIN_THREAD_SCHED_FIFO,
> + VIR_DOMAIN_THREAD_SCHED_RR,
> +
> + VIR_DOMAIN_THREAD_SCHED_LAST
> +} virDomainThreadSched;
> +
> +typedef struct _virDomainThreadSchedParam virDomainThreadSchedParam;
> +typedef virDomainThreadSchedParam *virDomainThreadSchedParamPtr;
> +struct _virDomainThreadSchedParam {
> + unsigned int id;
> + virDomainThreadSched scheduler;
> + int priority;
> +};
> +
> typedef struct _virDomainTimerCatchupDef virDomainTimerCatchupDef;
> typedef virDomainTimerCatchupDef *virDomainTimerCatchupDefPtr;
> struct _virDomainTimerCatchupDef {
> @@ -2002,6 +2020,11 @@ struct _virDomainCputune {
> virDomainVcpuPinDefPtr emulatorpin;
> size_t niothreadspin;
> virDomainVcpuPinDefPtr *iothreadspin;
> +
> + size_t nvcpusched;
> + virDomainThreadSchedParamPtr vcpusched;
> + size_t niothreadsched;
> + virDomainThreadSchedParamPtr iothreadsched;
> };
>
> typedef struct _virDomainBlkiotune virDomainBlkiotune;
> @@ -2807,6 +2830,7 @@ VIR_ENUM_DECL(virDomainRNGModel)
> VIR_ENUM_DECL(virDomainRNGBackend)
> VIR_ENUM_DECL(virDomainTPMModel)
> VIR_ENUM_DECL(virDomainTPMBackend)
> +VIR_ENUM_DECL(virDomainThreadSched)
> /* from libvirt.h */
> VIR_ENUM_DECL(virDomainState)
> VIR_ENUM_DECL(virDomainNostateReason)
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
b/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
> index 813d201..243092b 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
> @@ -4,6 +4,7 @@
> <memory unit='KiB'>219136</memory>
> <currentMemory unit='KiB'>219136</currentMemory>
> <vcpu placement='static'>2</vcpu>
> + <iothreads>1</iothreads>
> <cputune>
> <shares>2048</shares>
> <period>1000000</period>
> @@ -11,6 +12,9 @@
> <vcpupin vcpu='0' cpuset='0'/>
> <vcpupin vcpu='1' cpuset='1'/>
> <emulatorpin cpuset='1'/>
> + <vcpusched vcpu='0' scheduler='fifo'
priority='1'/>
> + <vcpusched vcpu='1' scheduler='batch'/>
> + <iothreadsched iothread='0' scheduler='idle'/>
iothread='0' - no such beast. 1..n
read the iothreadpin description
</cputune>
<os>
<type arch='i686' machine='pc'>hvm</type>
I think this should be a new file - eg qemuxml2argv-cpusched.xml with
the accompanying qemuxml2argv-cpusched.args file... That way you're
testing the results with and without your new XML
Since priority is listed as optional for 'fifo' and 'rr', you'll also
need one 'cpusched' with priority defined and one without it defined
using 'fifo'/'rr'. So that's either two more files or more vcpus in
your XML.
John
3:45 p.m.
New subject: [libvirt] [PATCH 2/3] docs, schema, conf: Add support for setting scheduler parameters of guest threads
On Mon, Jan 19, 2015 at 10:23:22PM -0500, John Ferlan wrote:
On 01/13/2015 02:57 AM, Martin Kletzander wrote:
> In order for QEMU vCPU (and other) threads to run with RT scheduler,
> libvirt needs to take care of that so QEMU doesn't have to run privileged.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1178986
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> docs/formatdomain.html.in | 13 +++
> docs/schemas/domaincommon.rng | 33 ++++++
> src/conf/domain_conf.c | 137 +++++++++++++++++++++++-
> src/conf/domain_conf.h | 24 +++++
> tests/qemuxml2argvdata/qemuxml2argv-cputune.xml | 4 +
> 5 files changed, 210 insertions(+), 1 deletion(-)
>
You'll need to rebase - I couldn't get this to apply... So review is
strictly visual and memory based.
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index d8c31e0..60dad76 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -550,6 +550,8 @@
> <quota>-1</quota>
> <emulator_period>1000000</emulator_period>
> <emulator_quota>-1</emulator_quota>
> + <vcpusched scheduler='fifo' priority='1'/>
> + <iothreadsched scheduler='idle'/>
> </cputune>
> ...
> </domain>
> @@ -656,6 +658,17 @@
> <span class="since">Only QEMU driver support since
0.10.0</span>
> </dd>
>
> + <dt><code>vcpusched</code> and
<code>iothreadsched</code></dt>
> + <dd>
> + The optional <code>vcpusched</code> element specifies the
scheduler
> + (values <code>batch</code>, <code>idle</code>,
<code>fifo</code>,
> + <code>rr</code> and <code>other</code>, which is
default and ignored)
> + for all vCPU/IOThread threads. For real-time schedulers
> + (<code>fifo</code>, <code>rr</code>), priority can
be specified as
> + well. The value range for the priority depends on the host kernel.
> + <span class="since">Since 1.2.12</span>
> + </dd>
> +
Why would someone use batch or idle? They're not described.
I just took all the options form sched_setscheduler() and made them
available. But I have no problem removing some of them as we can
always add them later on.
Upon first reading, I assumed one setting was good for *all*
vcpu's and
iothread's; however, later on there's a 'vcpuid' and 'iothreadid'
defined in the rng file. Does it even make sense to have 2 vCPUs using
'fifo' while another 2 are using 'ff'? If not, then why allow choosing
the id? (it's the "for all vCPU/IOThread threads" language that gave me
the impression).
Yes, that's my fault. What you understood fine (and I described) was
a first version. Then, before posting it, I enhanced that based on
response from QEMU guys who say that someone might need per-thread
specifics.
If you did desire different models, then why not model after the
'cpuset' with respect to being able to choose a range. If someone has 16
vCPU's are they going to be happy having to make 16 entries or just one
that indicates 0-15. You should be able to borrow/reuse existing code
for that parsing cpuset...
Would listing priority with batch/idle result in error or be ignored?
Testing will try it and they will flag it unless you say it'll be ignored.
It would be ignored.
I went with all-optional approach, so it's easiest to use. And that
takes me to the fact that one thing is still missing from this. I'd
suggest (myself) to make the vcpu/iothread ID also optional and that
one element without that particular ID would be default for all
threads not specified in other elements. I'm just not sure that goes
fine with our current approach (or rather all our current approaches
as we're *so* inconsistent).
I wouldn't list "other" - it's confusing. I
don't think other XML lists
other. Why would someone list "other" - they would only have this if
they wanted one of the models.
> </dl>
>
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index d250e6f..6653737 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -815,10 +815,43 @@
> </attribute>
> </element>
> </zeroOrMore>
> + <zeroOrMore>
> + <element name="vcpusched">
> + <attribute name="vcpu">
> + <ref name="vcpuid"/>
> + </attribute>
> + <ref name="schedparam"/>
> + </element>
> + </zeroOrMore>
> + <zeroOrMore>
> + <element name="iothreadsched">
> + <attribute name="iothread">
> + <ref name="unsignedInt"/>
> + </attribute>
> + <ref name="schedparam"/>
> + </element>
> + </zeroOrMore>
so essentially :
<vcpusched vcpuid=n scheduler='string' priority=n/>
and
<iothreadsched iothread=n scheduler='string' priority=n/>
> </interleave>
> </element>
> </define>
>
> + <define name="schedparam">
> + <attribute name="scheduler">
> + <choice>
> + <value>other</value>
> + <value>batch</value>
> + <value>idle</value>
> + <value>fifo</value>
> + <value>rr</value>
> + </choice>
> + </attribute>
> + <optional>
> + <attribute name="priority">
> + <ref name="unsignedShort"/>
> + </attribute>
> + </optional>
> + </define>
> +
leaving other here is OK... could also go with "none"
I took other as SCHED_OTHER from sched_setscheduler(). If I'd add
"none", I'd add it before "other" and "other" would then
mean that the
user wants to specifically call sched_setscheduler() with
SCHED_OTHER. That might be useful if you want to use some policy for
all vCPUs except one particular one.
> <!-- All the NUMA related tunables would go in the numatune
-->
> <define name="numatune">
> <element name="numatune">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 96d80a9..16adbf1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -795,6 +795,13 @@ VIR_ENUM_IMPL(virDomainLoader,
> "rom",
> "pflash")
>
> +VIR_ENUM_IMPL(virDomainThreadSched, VIR_DOMAIN_THREAD_SCHED_LAST,
> + "other", /* default */
> + "batch",
> + "idle",
> + "fifo",
> + "rr")
> +
other a/k/a none
> /* Internal mapping: subset of block job types that can be present in
> * <mirror> XML (remaining types are not two-phase). */
> VIR_ENUM_DECL(virDomainBlockJob)
> @@ -2260,6 +2267,9 @@ void virDomainDefFree(virDomainDefPtr def)
> virDomainVcpuPinDefArrayFree(def->cputune.iothreadspin,
> def->cputune.niothreadspin);
>
> + VIR_FREE(def->cputune.vcpusched);
> + VIR_FREE(def->cputune.iothreadsched);
> +
> virDomainNumatuneFree(def->numatune);
>
> virSysinfoDefFree(def->sysinfo);
> @@ -12653,6 +12663,65 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> return ret;
> }
>
> +static int
> +virDomainThreadSchedParse(xmlNodePtr node,
> + unsigned int maxid,
> + const char *name,
> + virDomainThreadSchedParamPtr sp)
> +{
> + char *tmp = NULL;
> + int sched = 0;
> +
> + tmp = virXMLPropString(node, name);
> + if (!tmp) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("invalid value for attribute %s"), name);
> + goto error;
> + }
> +
> + if (virStrToLong_uip(tmp, NULL, 10, &sp->id) < 0 ||
> + sp->id >= maxid) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("vcpu must be positive integer smaller than "
> + "maximum of vcpus, not '%s'"),
> + tmp);
Hmm.... this is challenging... vcpus are numbered 0..n-1; however,
iothreads are numbered 1..n
Yes, but I remember you had a reason for that.
Use the 'name' instead of 'vcpu' _("Invalid
%ssched value '%s'", name,
tmp).
That should be OK for translators. Or at least I hope so.
However, I think the range check should be in the caller. Then again,
why are we allowing someone to choose per cpu?
> + goto error;
> + }
> + VIR_FREE(tmp);
> +
> + tmp = virXMLPropString(node, "scheduler");
> + if (tmp) {
> + if ((sched = virDomainThreadSchedTypeFromString(tmp)) < 0) {
Why accept "other"? If you choose to not accept "other", then this
would
be <= 0... That way on output the only way it gets written is if it's
been set to something.
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid vcpu scheduler: '%s'"),
tmp);
change 'vcpu scheduler: '%s' to %ssched scheduler='%s' value, where
the
first %s is the 'name' argument.
> + goto error;
> + }
> + sp->scheduler = sched;
> +
> + VIR_FREE(tmp);
> + if (sp->scheduler >= VIR_DOMAIN_THREAD_SCHED_FIFO) {
Oh - this is dangerous... Today you've set them up that way, but if some
day something is added after RR that doesn't support priority, then
there'll be an error. Be specific here.
OK, that'll be better.
> + tmp = virXMLPropString(node, "priority");
> + if (!tmp) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("missing scheduler priority"));
> + goto error;
> + }
Priority is listed as optional in the rng, but this implies it must be
provided. Also you'll need to document that it's only parsed for fifo/rr...
> + if (virStrToLong_i(tmp, NULL, 10, &sp->priority) < 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("invalid value for element priority"));
> + goto error;
> + }
> + VIR_FREE(tmp);
> + }
> + }
> +
> + return 0;
> +
> + error:
> + VIR_FREE(tmp);
> + return -1;
> +}
> +
> static virDomainDefPtr
> virDomainDefParseXML(xmlDocPtr xml,
> xmlNodePtr root,
> @@ -13201,6 +13270,51 @@ virDomainDefParseXML(xmlDocPtr xml,
> }
> VIR_FREE(nodes);
>
> + if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes))
< 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot extract vcpusched nodes"));
> + goto error;
> + }
> + if (n) {
> + if (n > def->maxvcpus) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("too many vcpusched nodes in cputune"));
> + goto error;
> + }
> +
> + if (VIR_ALLOC_N(def->cputune.vcpusched, n) < 0)
> + goto error;
> + def->cputune.nvcpusched = n;
> +
> + for (i = 0; i < def->cputune.nvcpusched; i++) {
> + if (virDomainThreadSchedParse(nodes[i], def->maxvcpus,
"vcpu",
> + &def->cputune.vcpusched[i]) <
0)
> + goto error;
> + }
linear algorithm, but not linear number to vcpusched... Also, could
follow the 'cpuset' algorithm...
My idea would be to allocate the parameters per each cpu and just fill
it in like I did with memnode. And for the cpuset=, I was under the
impression that it would just confuse people. Look at the vcpupin and
iothreadpin, we don't put it there...
But I'm not against that.
> + }
VIR_FREE(nodes); ?
Yes!
>> +
>> + if ((n = virXPathNodeSet("./cputune/iothreadsched", ctxt,
&nodes)) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("cannot extract iothreadsched nodes"));
>> + goto error;
>> + }
>> + if (n) {
>> + if (n > def->iothreads) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("too many iothreadsched nodes in
cputune"));
>> + goto error;
>> + }
>> +
>> + if (VIR_ALLOC_N(def->cputune.iothreadsched, n) < 0)
>> + goto error;
>> + def->cputune.niothreadsched = n;
>> +
>> + for (i = 0; i < def->cputune.niothreadsched; i++) {
>> + if (virDomainThreadSchedParse(nodes[i], def->iothreads + 1,
"iothread",
>> + &def->cputune.iothreadsched[i])
< 0)
>> + goto error;
>> + }
> + }
VIR_FREE(nodes); ?
And yes.
>
> /* analysis of cpu handling */
> if ((node = virXPathNode("./cpu[1]", ctxt)) != NULL) {
> @@ -19519,7 +19633,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> def->cputune.period || def->cputune.quota ||
> def->cputune.emulatorpin ||
> def->cputune.emulator_period || def->cputune.emulator_quota ||
> - def->cputune.niothreadspin) {
> + def->cputune.niothreadspin ||
> + def->cputune.vcpusched || def->cputune.iothreadsched) {
> virBufferAddLit(buf, "<cputune>\n");
> cputune = true;
> }
> @@ -19592,6 +19707,26 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> VIR_FREE(cpumask);
> }
>
> + for (i = 0; i < def->cputune.nvcpusched; i++) {
> + virDomainThreadSchedParamPtr sp = &def->cputune.vcpusched[i];
> + virBufferAsprintf(buf, "<vcpusched vcpu='%d'
scheduler='%s'",
> + sp->id,
> + virDomainThreadSchedTypeToString(sp->scheduler));
> + if (sp->priority)
> + virBufferAsprintf(buf, " priority='%d'",
sp->priority);
> + virBufferAddLit(buf, "/>\n");
> + }
> +
> + for (i = 0; i < def->cputune.niothreadsched; i++) {
> + virDomainThreadSchedParamPtr sp = &def->cputune.iothreadsched[i];
> + virBufferAsprintf(buf, "<iothreadsched iothread='%d'
scheduler='%s'",
> + sp->id,
> + virDomainThreadSchedTypeToString(sp->scheduler));
> + if (sp->priority)
> + virBufferAsprintf(buf, " priority='%d'",
sp->priority);
> + virBufferAddLit(buf, "/>\n");
> + }
> +
> virBufferAdjustIndent(buf, -2);
> if (cputune)
> virBufferAddLit(buf, "</cputune>\n");
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 5cc42d1..636e2d1 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1815,6 +1815,24 @@ typedef enum {
> VIR_DOMAIN_CPU_PLACEMENT_MODE_LAST
> } virDomainCpuPlacementMode;
>
> +typedef enum {
> + VIR_DOMAIN_THREAD_SCHED_OTHER = 0,
> + VIR_DOMAIN_THREAD_SCHED_BATCH,
> + VIR_DOMAIN_THREAD_SCHED_IDLE,
> + VIR_DOMAIN_THREAD_SCHED_FIFO,
> + VIR_DOMAIN_THREAD_SCHED_RR,
> +
> + VIR_DOMAIN_THREAD_SCHED_LAST
> +} virDomainThreadSched;
> +
> +typedef struct _virDomainThreadSchedParam virDomainThreadSchedParam;
> +typedef virDomainThreadSchedParam *virDomainThreadSchedParamPtr;
> +struct _virDomainThreadSchedParam {
> + unsigned int id;
> + virDomainThreadSched scheduler;
> + int priority;
> +};
> +
> typedef struct _virDomainTimerCatchupDef virDomainTimerCatchupDef;
> typedef virDomainTimerCatchupDef *virDomainTimerCatchupDefPtr;
> struct _virDomainTimerCatchupDef {
> @@ -2002,6 +2020,11 @@ struct _virDomainCputune {
> virDomainVcpuPinDefPtr emulatorpin;
> size_t niothreadspin;
> virDomainVcpuPinDefPtr *iothreadspin;
> +
> + size_t nvcpusched;
> + virDomainThreadSchedParamPtr vcpusched;
> + size_t niothreadsched;
> + virDomainThreadSchedParamPtr iothreadsched;
> };
>
> typedef struct _virDomainBlkiotune virDomainBlkiotune;
> @@ -2807,6 +2830,7 @@ VIR_ENUM_DECL(virDomainRNGModel)
> VIR_ENUM_DECL(virDomainRNGBackend)
> VIR_ENUM_DECL(virDomainTPMModel)
> VIR_ENUM_DECL(virDomainTPMBackend)
> +VIR_ENUM_DECL(virDomainThreadSched)
> /* from libvirt.h */
> VIR_ENUM_DECL(virDomainState)
> VIR_ENUM_DECL(virDomainNostateReason)
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
b/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
> index 813d201..243092b 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune.xml
> @@ -4,6 +4,7 @@
> <memory unit='KiB'>219136</memory>
> <currentMemory unit='KiB'>219136</currentMemory>
> <vcpu placement='static'>2</vcpu>
> + <iothreads>1</iothreads>
> <cputune>
> <shares>2048</shares>
> <period>1000000</period>
> @@ -11,6 +12,9 @@
> <vcpupin vcpu='0' cpuset='0'/>
> <vcpupin vcpu='1' cpuset='1'/>
> <emulatorpin cpuset='1'/>
> + <vcpusched vcpu='0' scheduler='fifo'
priority='1'/>
> + <vcpusched vcpu='1' scheduler='batch'/>
> + <iothreadsched iothread='0' scheduler='idle'/>
iothread='0' - no such beast. 1..n
read the iothreadpin description
Ouch, what a beast it had become...
> </cputune>
> <os>
> <type arch='i686' machine='pc'>hvm</type>
>
I think this should be a new file - eg qemuxml2argv-cpusched.xml with
the accompanying qemuxml2argv-cpusched.args file... That way you're
testing the results with and without your new XML
Since priority is listed as optional for 'fifo' and 'rr', you'll also
need one 'cpusched' with priority defined and one without it defined
using 'fifo'/'rr'. So that's either two more files or more vcpus in
your XML.
OK, I'll do that.
Thanks for the review,
Martin
8:57 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Add support for setting vCPU and I/O thread scheduler setting
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1178986
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_driver.c | 7 ++++-
src/qemu/qemu_process.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++-
src/qemu/qemu_process.h | 5 +++-
3 files changed, 85 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cdf4173..fdfd4e9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1,7 +1,7 @@
/*
* qemu_driver.c: core driver methods for managing qemu guests
*
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
* Copyright (C) 2006 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -4491,6 +4491,11 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
}
virCgroupFree(&cgroup_vcpu);
+
+ if (qemuProcessSetSchedParams(i, cpupids[i],
+ vm->def->cputune.nvcpusched,
+ vm->def->cputune.vcpusched) < 0)
+ goto cleanup;
}
} else {
for (i = oldvcpus - 1; i >= nvcpus; i--) {
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c18204b..0d69c43 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1,7 +1,7 @@
/*
* qemu_process.c: QEMU process management
*
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -2574,6 +2574,76 @@ 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)
+{
+ int sched = 0;
+ size_t i = 0;
+ virDomainThreadSchedParamPtr s = NULL;
+
+ for (i = 0; i < nsp; i++) {
+ if (id != sp[i].id)
+ continue;
+
+ s = sp + i;
+ break;
+ }
+
+ if (!s)
+ return 0;
+
+ switch (s->scheduler) {
+ case VIR_DOMAIN_THREAD_SCHED_BATCH:
+ sched = SCHED_BATCH;
+ break;
+
+ case VIR_DOMAIN_THREAD_SCHED_IDLE:
+ sched = SCHED_IDLE;
+ break;
+
+ case VIR_DOMAIN_THREAD_SCHED_FIFO:
+ sched = SCHED_FIFO;
+ break;
+
+ case VIR_DOMAIN_THREAD_SCHED_RR:
+ sched = SCHED_RR;
+ break;
+
+ case VIR_DOMAIN_THREAD_SCHED_OTHER:
+ case VIR_DOMAIN_THREAD_SCHED_LAST:
+ return 0;
+ }
+
+ return virProcessSetScheduler(pid, sched, s->priority);
+}
+
+static int
+qemuProcessSetSchedulers(virDomainObjPtr vm)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ size_t i = 0;
+
+ for (i = 0; i < vm->def->vcpus; i++) {
+ if (qemuProcessSetSchedParams(i, priv->vcpupids[i],
+ vm->def->cputune.nvcpusched,
+ vm->def->cputune.vcpusched) < 0)
+ return -1;
+ }
+
+ for (i = 0; i < vm->def->iothreads; i++) {
+ if (qemuProcessSetSchedParams(i, priv->iothreadpids[i],
+ vm->def->cputune.niothreadsched,
+ vm->def->cputune.iothreadsched) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
static int
qemuProcessInitPasswords(virConnectPtr conn,
virQEMUDriverPtr driver,
@@ -4750,6 +4820,10 @@ int qemuProcessStart(virConnectPtr conn,
if (qemuProcessSetIOThreadsAffinity(vm) < 0)
goto cleanup;
+ VIR_DEBUG("Setting scheduler parameters");
+ if (qemuProcessSetSchedulers(vm) < 0)
+ goto cleanup;
+
VIR_DEBUG("Setting any required VM passwords");
if (qemuProcessInitPasswords(conn, driver, vm, asyncJob) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 5948ea4..2e1d393 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -1,7 +1,7 @@
/*
* qemu_process.h: QEMU process management
*
- * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2012, 2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -104,4 +104,7 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver,
int qemuProcessReadLog(int fd, char *buf, int buflen, int off, bool skipchar);
+int qemuProcessSetSchedParams(int id, pid_t pid, size_t nsp,
+ virDomainThreadSchedParamPtr sp);
+
#endif /* __QEMU_PROCESS_H__ */
--
2.2.1
3:39 p.m.
New subject: [libvirt] [PATCH 3/3] qemu: Add support for setting vCPU and I/O thread scheduler setting
On Tue, Jan 13, 2015 at 08:57:04AM +0100, Martin Kletzander wrote:
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1178986
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_driver.c | 7 ++++-
src/qemu/qemu_process.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++-
src/qemu/qemu_process.h | 5 +++-
3 files changed, 85 insertions(+), 3 deletions(-)
Ehm, consider the following diff squashed in O:-)
diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c
index 0d69c43..1702474 100644
--- i/src/qemu/qemu_process.c
+++ w/src/qemu/qemu_process.c
@@ -2627,14 +2627,14 @@ qemuProcessSetSchedulers(virDomainObjPtr vm)
qemuDomainObjPrivatePtr priv = vm->privateData;
size_t i = 0;
- for (i = 0; i < vm->def->vcpus; i++) {
+ for (i = 0; i < priv->nvcpupids; i++) {
if (qemuProcessSetSchedParams(i, priv->vcpupids[i],
vm->def->cputune.nvcpusched,
vm->def->cputune.vcpusched) < 0)
return -1;
}
- for (i = 0; i < vm->def->iothreads; i++) {
+ for (i = 0; i < priv->niothreadpids; i++) {
if (qemuProcessSetSchedParams(i, priv->iothreadpids[i],
vm->def->cputune.niothreadsched,
vm->def->cputune.iothreadsched) < 0)
--
4:33 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Add support for setting vCPU and I/O thread scheduler setting
On 01/13/2015 02:57 AM, Martin Kletzander wrote:
Could use a bit more details here.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/qemu/qemu_driver.c | 7 ++++-
src/qemu/qemu_process.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++-
src/qemu/qemu_process.h | 5 +++-
3 files changed, 85 insertions(+), 3 deletions(-)
No change to qemu_hotplug necessary??
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cdf4173..fdfd4e9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1,7 +1,7 @@
/*
* qemu_driver.c: core driver methods for managing qemu guests
*
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
* Copyright (C) 2006 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -4491,6 +4491,11 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
}
virCgroupFree(&cgroup_vcpu);
+
+ if (qemuProcessSetSchedParams(i, cpupids[i],
+ vm->def->cputune.nvcpusched,
+ vm->def->cputune.vcpusched) < 0)
+ goto cleanup;
}
} else {
for (i = oldvcpus - 1; i >= nvcpus; i--) {
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c18204b..0d69c43 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1,7 +1,7 @@
/*
* qemu_process.c: QEMU process management
*
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -2574,6 +2574,76 @@ 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)
+{
+ int sched = 0;
+ size_t i = 0;
+ virDomainThreadSchedParamPtr s = NULL;
+
+ for (i = 0; i < nsp; i++) {
+ if (id != sp[i].id)
+ continue;
+
+ s = sp + i;
+ break;
+ }
Ug. I think this would be easier with the bitmap like cpuset. I'll wait
for your feedback on 2/3...
Do you plan to add a new command 'vcpusched' (like vcpupin). It's
something that's desired for IOThreads, so perhaps one of us gets to
reuse code :-)
John
+
+ if (!s)
+ return 0;
+
+ switch (s->scheduler) {
+ case VIR_DOMAIN_THREAD_SCHED_BATCH:
+ sched = SCHED_BATCH;
+ break;
+
+ case VIR_DOMAIN_THREAD_SCHED_IDLE:
+ sched = SCHED_IDLE;
+ break;
+
+ case VIR_DOMAIN_THREAD_SCHED_FIFO:
+ sched = SCHED_FIFO;
+ break;
+
+ case VIR_DOMAIN_THREAD_SCHED_RR:
+ sched = SCHED_RR;
+ break;
+
+ case VIR_DOMAIN_THREAD_SCHED_OTHER:
+ case VIR_DOMAIN_THREAD_SCHED_LAST:
+ return 0;
+ }
+
+ return virProcessSetScheduler(pid, sched, s->priority);
+}
+
+static int
+qemuProcessSetSchedulers(virDomainObjPtr vm)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ size_t i = 0;
+
+ for (i = 0; i < vm->def->vcpus; i++) {
+ if (qemuProcessSetSchedParams(i, priv->vcpupids[i],
+ vm->def->cputune.nvcpusched,
+ vm->def->cputune.vcpusched) < 0)
+ return -1;
+ }
+
+ for (i = 0; i < vm->def->iothreads; i++) {
+ if (qemuProcessSetSchedParams(i, priv->iothreadpids[i],
+ vm->def->cputune.niothreadsched,
+ vm->def->cputune.iothreadsched) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
static int
qemuProcessInitPasswords(virConnectPtr conn,
virQEMUDriverPtr driver,
@@ -4750,6 +4820,10 @@ int qemuProcessStart(virConnectPtr conn,
if (qemuProcessSetIOThreadsAffinity(vm) < 0)
goto cleanup;
+ VIR_DEBUG("Setting scheduler parameters");
+ if (qemuProcessSetSchedulers(vm) < 0)
+ goto cleanup;
+
VIR_DEBUG("Setting any required VM passwords");
if (qemuProcessInitPasswords(conn, driver, vm, asyncJob) < 0)
goto cleanup;
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 5948ea4..2e1d393 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -1,7 +1,7 @@
/*
* qemu_process.h: QEMU process management
*
- * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2012, 2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -104,4 +104,7 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver,
int qemuProcessReadLog(int fd, char *buf, int buflen, int off, bool skipchar);
+int qemuProcessSetSchedParams(int id, pid_t pid, size_t nsp,
+ virDomainThreadSchedParamPtr sp);
+
#endif /* __QEMU_PROCESS_H__ */
3:48 p.m.
New subject: [libvirt] [PATCH 3/3] qemu: Add support for setting vCPU and I/O thread scheduler setting
On Mon, Jan 19, 2015 at 10:33:52PM -0500, John Ferlan wrote:
On 01/13/2015 02:57 AM, Martin Kletzander wrote:
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1178986
Could use a bit more details here.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 7 ++++-
> src/qemu/qemu_process.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_process.h | 5 +++-
> 3 files changed, 85 insertions(+), 3 deletions(-)
>
No change to qemu_hotplug necessary??
There's nothing needed there, IIUC.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index cdf4173..fdfd4e9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1,7 +1,7 @@
> /*
> * qemu_driver.c: core driver methods for managing qemu guests
> *
> - * Copyright (C) 2006-2014 Red Hat, Inc.
> + * Copyright (C) 2006-2015 Red Hat, Inc.
> * Copyright (C) 2006 Daniel P. Berrange
> *
> * This library is free software; you can redistribute it and/or
> @@ -4491,6 +4491,11 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> }
>
> virCgroupFree(&cgroup_vcpu);
> +
> + if (qemuProcessSetSchedParams(i, cpupids[i],
> + vm->def->cputune.nvcpusched,
> + vm->def->cputune.vcpusched) < 0)
> + goto cleanup;
> }
> } else {
> for (i = oldvcpus - 1; i >= nvcpus; i--) {
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c18204b..0d69c43 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1,7 +1,7 @@
> /*
> * qemu_process.c: QEMU process management
> *
> - * Copyright (C) 2006-2014 Red Hat, Inc.
> + * Copyright (C) 2006-2015 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -2574,6 +2574,76 @@ 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)
> +{
> + int sched = 0;
> + size_t i = 0;
> + virDomainThreadSchedParamPtr s = NULL;
> +
> + for (i = 0; i < nsp; i++) {
> + if (id != sp[i].id)
> + continue;
> +
> + s = sp + i;
> + break;
> + }
Ug. I think this would be easier with the bitmap like cpuset. I'll wait
for your feedback on 2/3...
Yes, this will look way better in v2.
Do you plan to add a new command 'vcpusched' (like vcpupin).
It's
something that's desired for IOThreads, so perhaps one of us gets to
reuse code :-)
I'm not sure anyone will use that, but it there is demand, then I have
no problem adding that as well.
John
> +
> + if (!s)
> + return 0;
> +
> + switch (s->scheduler) {
> + case VIR_DOMAIN_THREAD_SCHED_BATCH:
> + sched = SCHED_BATCH;
> + break;
> +
> + case VIR_DOMAIN_THREAD_SCHED_IDLE:
> + sched = SCHED_IDLE;
> + break;
> +
> + case VIR_DOMAIN_THREAD_SCHED_FIFO:
> + sched = SCHED_FIFO;
> + break;
> +
> + case VIR_DOMAIN_THREAD_SCHED_RR:
> + sched = SCHED_RR;
> + break;
> +
> + case VIR_DOMAIN_THREAD_SCHED_OTHER:
> + case VIR_DOMAIN_THREAD_SCHED_LAST:
> + return 0;
> + }
> +
> + return virProcessSetScheduler(pid, sched, s->priority);
> +}
> +
> +static int
> +qemuProcessSetSchedulers(virDomainObjPtr vm)
> +{
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + size_t i = 0;
> +
> + for (i = 0; i < vm->def->vcpus; i++) {
> + if (qemuProcessSetSchedParams(i, priv->vcpupids[i],
> + vm->def->cputune.nvcpusched,
> + vm->def->cputune.vcpusched) < 0)
> + return -1;
> + }
> +
> + for (i = 0; i < vm->def->iothreads; i++) {
> + if (qemuProcessSetSchedParams(i, priv->iothreadpids[i],
> + vm->def->cputune.niothreadsched,
> + vm->def->cputune.iothreadsched) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> static int
> qemuProcessInitPasswords(virConnectPtr conn,
> virQEMUDriverPtr driver,
> @@ -4750,6 +4820,10 @@ int qemuProcessStart(virConnectPtr conn,
> if (qemuProcessSetIOThreadsAffinity(vm) < 0)
> goto cleanup;
>
> + VIR_DEBUG("Setting scheduler parameters");
> + if (qemuProcessSetSchedulers(vm) < 0)
> + goto cleanup;
> +
> VIR_DEBUG("Setting any required VM passwords");
> if (qemuProcessInitPasswords(conn, driver, vm, asyncJob) < 0)
> goto cleanup;
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 5948ea4..2e1d393 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -1,7 +1,7 @@
> /*
> * qemu_process.h: QEMU process management
> *
> - * Copyright (C) 2006-2012 Red Hat, Inc.
> + * Copyright (C) 2006-2012, 2015 Red Hat, Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -104,4 +104,7 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver,
>
> int qemuProcessReadLog(int fd, char *buf, int buflen, int off, bool skipchar);
>
> +int qemuProcessSetSchedParams(int id, pid_t pid, size_t nsp,
> + virDomainThreadSchedParamPtr sp);
> +
> #endif /* __QEMU_PROCESS_H__ */
>
8:10 p.m.
New subject: [libvirt] [PATCH 3/3] qemu: Add support for setting vCPU and I/O thread scheduler setting
On 01/20/2015 09:48 AM, Martin Kletzander wrote:
<...snip...>
> Do you plan to add a new command 'vcpusched' (like vcpupin). It's
> something that's desired for IOThreads, so perhaps one of us gets to
> reuse code :-)
>
I'm not sure anyone will use that, but it there is demand, then I have
no problem adding that as well.
With this implementation someone can only edit the XML in order to
enable/add; whereas, a 'virsh vcpusched {scheduler} [vcpuid] [priority]'
would be much more simple... and of course enables a python script API.
Someone with 1-4 vCPU's probably doesn't care. Someone with more
probably starts caring when they've cut-n-pasted too much.
Rome wasn't built in a day, so not required for this pass, but something
to consider.
John
3531
days inactive
3538
days old
11 comments
2 participants
participants (2)
-
John Ferlan -
Martin Kletzander