[libvirt] [PATCH 0/3] perf: Unbreak virsh and XML parser
Yet another breakage in the perf event code. Add missing docs, fix schemas and rewrite the XML parser since it was broken. Peter Krempa (3): qemu: perf: Don't set state of first event for every other event docs: Add at least some docs and fix schema entry for perf events conf: Fix perf event parser docs/formatdomain.html.in | 44 +++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 2 ++ src/conf/domain_conf.c | 47 ++++++++++++----------------- src/qemu/qemu_driver.c | 2 +- tests/genericxml2xmlindata/generic-perf.xml | 22 ++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 6 files changed, 90 insertions(+), 29 deletions(-) create mode 100644 tests/genericxml2xmlindata/generic-perf.xml -- 2.8.3
A bug in the code used the value of the first perf event as state for all the mentioned one rather than extracting individual ones. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1346730 --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52df21e..d293513 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9742,7 +9742,7 @@ qemuDomainSetPerfEvents(virDomainPtr dom, if (def) { for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; - enabled = params->value.b; + enabled = param->value.b; type = virPerfEventTypeFromString(param->field); if (!enabled && virPerfEventDisable(priv->perf, type) < 0) -- 2.8.3
On 06/15/2016 11:15 AM, Peter Krempa wrote:
A bug in the code used the value of the first perf event as state for all the mentioned one rather than extracting individual ones.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1346730 --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK John
On Thu, Jun 16, 2016 at 14:01:59 -0400, John Ferlan wrote:
On 06/15/2016 11:15 AM, Peter Krempa wrote:
A bug in the code used the value of the first perf event as state for all the mentioned one rather than extracting individual ones.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1346730 --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
There are two instances of the same bug, one more in the persistentDef section. I added it to this patch. I'll also post a follow up that cleans the code up since we don't really need to loop the array twice.
There was no documentation at all for the XML part. I added at least some. The 2.0.0 introduction date is deliberate as the parser for the XML is broken. The schema file was missing entries for 'mbml' and 'mbmt'. --- docs/formatdomain.html.in | 44 +++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 2 ++ 2 files changed, 46 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index dd9f153..e21c355 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1822,6 +1822,50 @@ </dd> </dl> + <h3><a name="elementsPerf">Performance monitoring events</a></h3> + + <p> + Some platforms allow monitoring of performance of the virtual machine and + the code executed inside. To enable the performance monitoring events + you can either specify them in the <code>perf</code> element or enable + them via <code>virDomainSetPerfEvents</code> API. The performance values + are then retrieved using the virConnectGetAllDomainStats API. + <span class="since">Since 2.0.0</span> + </p> + +<pre> + ... + <perf> + <event name='cmt' enabled='yes'/> + <event name='mbmt' enabled='no'/> + <event name='mbml' enabled='yes'/> + </perf> + ... +</pre> + + <table class="top_table"> + <tr> + <th>event name</th> + <th>Description</th> + <th>stats parameter name</th> + </tr> + <tr> + <td><code>cmt</code></td> + <td>usage of l3 cache in bytes by applications running on the platform</td> + <td><code>perf.cmt</code></td> + </tr> + <tr> + <td><code>mbmt</code></td> + <td>total system bandwidth from one level of cache</td> + <td><code>perf.mbmt</code></td> + </tr> + <tr> + <td><code>mbml</code></td> + <td>bandwidth of memory traffic for a memory controller</td> + <td><code>perf.mbml</code></td> + </tr> + </table> + <h3><a name="elementsDevices">Devices</a></h3> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2e07505..162c2e0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -412,6 +412,8 @@ <attribute name="name"> <choice> <value>cmt</value> + <value>mbmt</value> + <value>mbml</value> </choice> </attribute> <attribute name="enabled"> -- 2.8.3
On 06/15/2016 11:15 AM, Peter Krempa wrote:
There was no documentation at all for the XML part. I added at least some. The 2.0.0 introduction date is deliberate as the parser for the XML is broken.
The schema file was missing entries for 'mbml' and 'mbmt'. --- docs/formatdomain.html.in | 44 +++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 2 ++ 2 files changed, 46 insertions(+)
ACK to what's here, but can we add mbmt and mbml to virsh.pod too? mbmt/mbml were added in commit id '90b9995d1' John
The parser was totaly broken. Fix it by rewriting it. Add tests so that it doesn't happen. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1346723 --- src/conf/domain_conf.c | 47 ++++++++++++----------------- tests/genericxml2xmlindata/generic-perf.xml | 22 ++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 3 files changed, 43 insertions(+), 28 deletions(-) create mode 100644 tests/genericxml2xmlindata/generic-perf.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9504e5f..215538a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13056,57 +13056,48 @@ virDomainPMStateParseXML(xmlXPathContextPtr ctxt, static int virDomainPerfEventDefParseXML(virDomainPerfDefPtr perf, - xmlNodePtr node, - xmlXPathContextPtr ctxt) + xmlNodePtr node) { char *name = NULL; char *enabled = NULL; - int enabled_type; - int name_type; + int event; int ret = -1; - xmlNodePtr oldnode = ctxt->node; - - ctxt->node = node; - if (!(name = virXPathString("string(./@name)", ctxt))) { - virReportError(VIR_ERR_CONF_SYNTAX, "%s", - _("missing name for event")); + if (!(name = virXMLPropString(node, "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("missing perf event name")); goto cleanup; } - if ((name_type = virPerfEventTypeFromString(name)) < 0) { - virReportError(VIR_ERR_CONF_SYNTAX, - _("%s is not a supported event name"), name); + if ((event = virPerfEventTypeFromString(name)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("'unsupported perf event '%s'"), name); goto cleanup; } - if (!(enabled = virXPathString("string(./@enabled)", ctxt))) { - virReportError(VIR_ERR_CONF_SYNTAX, - _("missing state for cipher named %s"), name); + if (perf->events[event] != VIR_TRISTATE_BOOL_ABSENT) { + virReportError(VIR_ERR_XML_ERROR, + _("perf event '%s' was already specified"), name); goto cleanup; } - if ((enabled_type = virTristateBoolTypeFromString(enabled)) < 0) { - virReportError(VIR_ERR_CONF_SYNTAX, - _("%s is not a supported enabled state"), enabled); + if (!(enabled = virXMLPropString(node, "enabled"))) { + virReportError(VIR_ERR_XML_ERROR, + _("missing state of perf event '%s'"), name); goto cleanup; } - if (perf->events[VIR_PERF_EVENT_CMT] != VIR_TRISTATE_BOOL_ABSENT) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("A domain definition can have no more than " - "one event node with name %s"), - virTristateBoolTypeToString(name_type)); - + if ((perf->events[event] = virTristateBoolTypeFromString(enabled)) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid state '%s' of perf event '%s'"), + enabled, name); goto cleanup; } - perf->events[VIR_PERF_EVENT_CMT] = enabled_type; ret = 0; + cleanup: VIR_FREE(name); VIR_FREE(enabled); - ctxt->node = oldnode; return ret; } @@ -13126,7 +13117,7 @@ virDomainPerfDefParseXML(virDomainDefPtr def, goto cleanup; for (i = 0; i < n; i++) { - if (virDomainPerfEventDefParseXML(def->perf, nodes[i], ctxt) < 0) + if (virDomainPerfEventDefParseXML(def->perf, nodes[i]) < 0) goto cleanup; } diff --git a/tests/genericxml2xmlindata/generic-perf.xml b/tests/genericxml2xmlindata/generic-perf.xml new file mode 100644 index 0000000..394d2a6 --- /dev/null +++ b/tests/genericxml2xmlindata/generic-perf.xml @@ -0,0 +1,22 @@ +<domain type='qemu'> + <name>foo</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <perf> + <event name='cmt' enabled='yes'/> + <event name='mbmt' enabled='no'/> + <event name='mbml' enabled='yes'/> + </perf> + <devices> + </devices> +</domain> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index d492fb4..1a7a668 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -95,6 +95,8 @@ mymain(void) DO_TEST_FULL("name-slash-parse", 0, false, TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); + DO_TEST("perf"); + virObjectUnref(caps); virObjectUnref(xmlopt); -- 2.8.3
On 06/15/2016 11:15 AM, Peter Krempa wrote:
The parser was totaly broken. Fix it by rewriting it. Add tests so that it doesn't happen.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1346723 --- src/conf/domain_conf.c | 47 ++++++++++++----------------- tests/genericxml2xmlindata/generic-perf.xml | 22 ++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 3 files changed, 43 insertions(+), 28 deletions(-) create mode 100644 tests/genericxml2xmlindata/generic-perf.xml
ACK, John FWIW: There is a tests/domainschemadata/domain-perf-simple.xml from commit id 'afe833e9b'....
On Thu, Jun 16, 2016 at 14:11:39 -0400, John Ferlan wrote:
On 06/15/2016 11:15 AM, Peter Krempa wrote:
The parser was totaly broken. Fix it by rewriting it. Add tests so that it doesn't happen.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1346723 --- src/conf/domain_conf.c | 47 ++++++++++++----------------- tests/genericxml2xmlindata/generic-perf.xml | 22 ++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 3 files changed, 43 insertions(+), 28 deletions(-) create mode 100644 tests/genericxml2xmlindata/generic-perf.xml
ACK,
John
FWIW:
There is a tests/domainschemadata/domain-perf-simple.xml from commit id 'afe833e9b'....
I'll drop it in a follow up. That file doesn't make sense since it's not excercising the XML code and we can do it by the test I've added.
participants (2)
-
John Ferlan -
Peter Krempa