On 07/16/2016 04:15 AM, Qiaowei Ren wrote:

This patch rename qemuDomainGetStatsPerfRdt() to qemuDomainGetStatsPerfOneEvent() and update qemuDomainGetStatsPerf() based on this change for multiple/general purpose.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com> --- src/qemu/qemu_driver.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)

This really should be two patches - one to rename the function and one to remove the switch. John

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cda85f6..1fdb7b8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18938,10 +18938,10 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver, #undef QEMU_ADD_COUNT_PARAM

static int -qemuDomainGetStatsPerfRdt(virPerfPtr perf, - virPerfEventType type, - virDomainStatsRecordPtr record, - int *maxparams) +qemuDomainGetStatsPerfOneEvent(virPerfPtr perf, + virPerfEventType type, + virDomainStatsRecordPtr record, + int *maxparams) { char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; uint64_t value = 0; @@ -18977,14 +18977,9 @@ qemuDomainGetStatsPerf(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, if (!virPerfEventIsEnabled(priv->perf, i)) continue;

- switch (i) { - case VIR_PERF_EVENT_CMT: - case VIR_PERF_EVENT_MBMT: - case VIR_PERF_EVENT_MBML: - if (qemuDomainGetStatsPerfRdt(priv->perf, i, record, maxparams) < 0) - goto cleanup; - break; - } + if (qemuDomainGetStatsPerfOneEvent(priv->perf, i, + record, maxparams) < 0) + goto cleanup; }

ret = 0;

Need to keep those commit messages a lot shorter... On 07/16/2016 04:15 AM, Qiaowei Ren wrote:

This patch creates some sort of static table/matrix that would be able to convert the VIR_PERF_EVENT_* into their respective "attr.type" and "attr.config", so that virPerfEventEnable() doesn't have the switch the calling function passes by value the 'type'. This change is for general purpose in future.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com> --- src/util/virperf.c | 160 ++++++++++++++++++++++++++++++----------------------- src/util/virperf.h | 6 ++ 2 files changed, 97 insertions(+), 69 deletions(-)

Another patch which could use some splitting... Rather than go back and forth more on this, I'll generate a sequence of patches based on your code with some tweaks - most of which I'll suggest throughout here. If you git am those patches and git diff them to your current patches you can see the differences... But I'd also expect you could test and validate my adjustments work...

diff --git a/src/util/virperf.c b/src/util/virperf.c index 4661ba3..01c7c70 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -57,6 +57,8 @@ struct virPerf { struct virPerfEvent events[VIR_PERF_EVENT_LAST]; };

+static void virPerfRdtAttrInit(void); +

Avoid forward decls... I think what would be best to do is move virPerfNew and virPerfFree to the bottom of the function after the #endif for "if defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)" That would be it's own separate patch... Then we can start modifying the code to add the new call...

virPerfPtr virPerfNew(void) {

In my changes virPerfNew is at the bottom now so we won't need the forward decl.

@@ -72,6 +74,8 @@ virPerfNew(void) perf->events[i].enabled = false; }

+ virPerfRdtAttrInit(); +

This should return a value, as follows: if (virPerfRdtAttrInit() < 0) virResetLastError(); For the #else code, we just return 0 - so it's a no-op. When the virPerfEventEnable is called an error is returned. The "real" code inside the #if getting a failure would need to be made "as failure proof" as the existing code which would have returned a failure from virPerfEventEnable and for the most part ignore it. Once we call virPerfEventEnable it'll find the attrType == 0 for the RDT events and cause a failure. Alternatively, we could fail virPerfNew if AttrInit fails, but that's a bit different than the existing code.

return perf; }

@@ -95,12 +99,21 @@ virPerfFree(virPerfPtr perf)

# include <linux/perf_event.h>

-static virPerfEventPtr -virPerfGetEvent(virPerfPtr perf, - virPerfEventType type) +static struct virPerfEventAttr { + int type; + unsigned int attrType; + unsigned long long attrConfig; +} attrs[] = { + {.type = VIR_PERF_EVENT_CMT, .attrType = 0, .attrConfig = 1}, + {.type = VIR_PERF_EVENT_MBMT, .attrType = 0, .attrConfig = 2}, + {.type = VIR_PERF_EVENT_MBML, .attrType = 0, .attrConfig = 3}, +}; +typedef struct virPerfEventAttr *virPerfEventAttrPtr; + +static virPerfEventAttrPtr +virPerfGetEventAttr(virPerfEventType type) { - if (!perf) - return NULL; + size_t i;

if (type >= VIR_PERF_EVENT_LAST) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -109,17 +122,20 @@ virPerfGetEvent(virPerfPtr perf, return NULL; }

- return perf->events + type; + for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { + if (i == type) + return attrs + i; + } + + return NULL; }

-static int -virPerfRdtEnable(virPerfEventPtr event, - pid_t pid) +static void virPerfRdtAttrInit(void)

static void virPerfRdtAttrInit(void) is the normal way of doing this. However, this is not a void function... If there's any errors here, then we're not failing and returning any error...

{ - struct perf_event_attr rdt_attr; char *buf = NULL; char *tmp = NULL; - unsigned int event_type, scale; + size_t i; + unsigned int attr_type = 0;

if (virFileReadAll("/sys/devices/intel_cqm/type", 10, &buf) < 0)

This will go to error with some error message set, but then returns a void status.

@@ -128,48 +144,74 @@ virPerfRdtEnable(virPerfEventPtr event, if ((tmp = strchr(buf, '\n'))) *tmp = '\0';

- if (virStrToLong_ui(buf, NULL, 10, &event_type) < 0) { + if (virStrToLong_ui(buf, NULL, 10, &attr_type) < 0) { virReportSystemError(errno, "%s", _("failed to get rdt event type")); goto error;

This goes to error with some error message set, but then returns a void status.

} VIR_FREE(buf);

Currently unnecessary since everything falls through into the error: label...

- memset(&rdt_attr, 0, sizeof(rdt_attr)); - rdt_attr.size = sizeof(rdt_attr); - rdt_attr.type = event_type; - rdt_attr.inherit = 1; - rdt_attr.disabled = 1; - rdt_attr.enable_on_exec = 0; - - switch (event->type) { - case VIR_PERF_EVENT_CMT: - if (virFileReadAll("/sys/devices/intel_cqm/events/llc_occupancy.scale", - 10, &buf) < 0) - goto error; - - if (virStrToLong_ui(buf, NULL, 10, &scale) < 0) { - virReportSystemError(errno, "%s", - _("failed to get cmt scaling factor")); - goto error; - } - event->efields.cmt.scale = scale;

Is this hunk no longer necessary? Also, existing "efields.cmt.scale" is an "int" while "scale" is an unsigned int. Can be read directly too: if (virStrToLong_i(buf, NULL, 10, &event->efields.cmt.scale) < 0) {

- - rdt_attr.config = 1; - break; - case VIR_PERF_EVENT_MBMT: - rdt_attr.config = 2; - break; - case VIR_PERF_EVENT_MBML: - rdt_attr.config = 3; - break; + for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { + if (i == VIR_PERF_EVENT_CMT || + i == VIR_PERF_EVENT_MBMT || + i == VIR_PERF_EVENT_MBML) + attrs[i].attrType = attr_type;

So I assume I can make the 'assumption' that all 3 types will have the same attr_type, so rather than reading for each loop, you're reading just once... In any case, the for loop is unecessary - especially considering future patches where have more events. Just set the 3 attrType's directly: attrs[VIR_PERF_EVENT_CMT].attrType = event_type; attrs[VIR_PERF_EVENT_MBMT].attrType = event_type; attrs[VIR_PERF_EVENT_MBML].attrType = event_type;

+ }

And we fall into the error label! So, it's more of a cleanup label...

+ + error: + VIR_FREE(buf); +} + +static virPerfEventPtr +virPerfGetEvent(virPerfPtr perf, + virPerfEventType type) +{ + if (!perf) + return NULL; + + if (type >= VIR_PERF_EVENT_LAST) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Event '%d' is not supported"), + type); + return NULL; }

- event->fd = syscall(__NR_perf_event_open, &rdt_attr, pid, -1, -1, 0); + return perf->events + type; +} + +int +virPerfEventEnable(virPerfPtr perf, + virPerfEventType type, + pid_t pid) +{ + struct perf_event_attr attr; + virPerfEventPtr event = virPerfGetEvent(perf, type); + virPerfEventAttrPtr event_attr = virPerfGetEventAttr(type); + if (event == NULL || event_attr == NULL) + return -1; + + if (event_attr->attrType == 0 && (type == VIR_PERF_EVENT_CMT || + type == VIR_PERF_EVENT_MBMT || + type == VIR_PERF_EVENT_MBML)) { + virReportSystemError(errno, + _("Unable to open perf event for %s"), + virPerfEventTypeToString(event->type)); + return -1; + } +

This is where the scale code should be put back: if (type == VIR_PERF_EVENT_CMT) { if (virFileReadAll("/sys/devices/intel_cqm/events/llc_occupancy.scale", 10, &buf) < 0) goto error; if (virStrToLong_i(buf, NULL, 10, &event->efields.cmt.scale) < 0) { virReportSystemError(errno, "%s", _("failed to get cmt scaling factor")); goto error; } }

+ memset(&attr, 0, sizeof(attr)); + attr.size = sizeof(attr); + attr.inherit = 1; + attr.disabled = 1; + attr.enable_on_exec = 0; + attr.type = event_attr->attrType; + attr.config = event_attr->attrConfig; + + event->fd = syscall(__NR_perf_event_open, &attr, pid, -1, -1, 0); if (event->fd < 0) { virReportSystemError(errno, - _("Unable to open perf type=%d for pid=%d"), - event_type, pid); + _("Unable to open perf event for %s"), + virPerfEventTypeToString(event->type)); goto error; }

@@ -185,36 +227,10 @@ virPerfRdtEnable(virPerfEventPtr event,

error: VIR_FORCE_CLOSE(event->fd); - VIR_FREE(buf); return -1; }

int -virPerfEventEnable(virPerfPtr perf, - virPerfEventType type, - pid_t pid) -{ - virPerfEventPtr event = virPerfGetEvent(perf, type); - if (event == NULL) - return -1; - - switch (type) { - case VIR_PERF_EVENT_CMT: - case VIR_PERF_EVENT_MBMT: - case VIR_PERF_EVENT_MBML: - if (virPerfRdtEnable(event, pid) < 0) - return -1; - break; - case VIR_PERF_EVENT_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected perf event type=%d"), type); - return -1; - } - - return 0; -} - -int virPerfEventDisable(virPerfPtr perf, virPerfEventType type) { @@ -266,6 +282,12 @@ virPerfReadEvent(virPerfPtr perf, }

#else +static void virPerfRdtAttrInit(void) +{ + virReportSystemError(ENXIO, "%s", + _("Perf not supported on this platform"));

This should do nothing, in fact return 0... Subsequent callers will cause the failure... Otherwise, we set an error message without using it. So just have a "return 0;"

+} + int virPerfEventEnable(virPerfPtr perf ATTRIBUTE_UNUSED, virPerfEventType type ATTRIBUTE_UNUSED, diff --git a/src/util/virperf.h b/src/util/virperf.h index 7163410..acb59ab 100644 --- a/src/util/virperf.h +++ b/src/util/virperf.h

Since nothing here affects virperf.c, it should be separate. I've take some liberties to adjust things slightly as you'll see John

@@ -25,6 +25,12 @@ # include "virutil.h"

typedef enum { + /* Some Intel processor families introduced some RDT (Resource + * Director Technology) features to monitor or control shared + * resource. Among the features, CMT (Cache Monitoring Technology) + * and MBM (Memory Bandwidth Monitoring) is implemented based + * on perf framework in linux kernel. + */ VIR_PERF_EVENT_CMT, VIR_PERF_EVENT_MBMT, VIR_PERF_EVENT_MBML,

On 07/16/2016 04:15 AM, Qiaowei Ren wrote:

With current perf framework, this patch adds support to more perf events, including cache misses, cache references, cpu cycles, instrctions, etc..

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com> --- docs/formatdomain.html.in | 24 ++++++++++++++++++ docs/schemas/domaincommon.rng | 4 +++ include/libvirt/libvirt-domain.h | 39 +++++++++++++++++++++++++++++ src/libvirt-domain.c | 8 ++++++ src/qemu/qemu_driver.c | 4 +++ src/util/virperf.c | 16 +++++++++++- src/util/virperf.h | 5 ++++ tests/genericxml2xmlindata/generic-perf.xml | 4 +++ 8 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 59a8bb9..53a0809 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1842,6 +1842,10 @@ <event name='cmt' enabled='yes'/> <event name='mbmt' enabled='no'/> <event name='mbml' enabled='yes'/> + <event name='cpu_cycles' enabled='no'/> + <event name='instructions' enabled='yes'/> + <event name='cache_references' enabled='no'/> + <event name='cache_misses' enabled='no'/> </perf> ... </pre> @@ -1867,6 +1871,26 @@ <td>bandwidth of memory traffic for a memory controller</td> <td><code>perf.mbml</code></td> </tr> + <tr> + <td><code>cpu_cycles</code></td> + <td>the number of cpu cycles one instruction needs</td> + <td><code>perf.cpu_cycles</code></td> + </tr> + <tr> + <td><code>instructions</code></td> + <td>the count of instructions by applications running on the platform</td> + <td><code>perf.instructions</code></td> + </tr> + <tr> + <td><code>cache_references</code></td> + <td>the count of cache hits by applications running on the platform</td> + <td><code>perf.cache_references</code></td> + </tr> + <tr> + <td><code>cache_misses</code></td> + <td>the count of cache misses by applications running on the platform</td> + <td><code>perf.cache_misses</code></td> + </tr> </table>

<h3><a name="elementsDevices">Devices</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 348dbfe..be8ebb5 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -414,6 +414,10 @@ <value>cmt</value> <value>mbmt</value> <value>mbml</value> + <value>cpu_cycles</value> + <value>instructions</value> + <value>cache_references</value> + <value>cache_misses</value> </choice> </attribute> <attribute name="enabled"> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 7ea93aa..0002d99 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1947,6 +1947,45 @@ void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats); */ # define VIR_PERF_PARAM_MBML "mbml"

+/** + * VIR_PERF_PARAM_CACHE_MISSES: + * + * Macro for typed parameter name that represents cache_misses perf + * event which can be used to measure the count of cache misses by + * applications running on the platform. It corresponds to the + * "perf.cache_misses" field in the *Stats APIs. + */ +# define VIR_PERF_PARAM_CACHE_MISSES "cache_misses" + +/** + * VIR_PERF_PARAM_CACHE_REFERENCES: + * + * Macro for typed parameter name that represents cache_references + * perf event which can be used to measure the count of cache hits + * by applications running on the platform. It corresponds to the + * "perf.cache_references" field in the *Stats APIs. + */ +# define VIR_PERF_PARAM_CACHE_REFERENCES "cache_references" + +/** + * VIR_PERF_PARAM_INSTRUCTIONS: + * + * Macro for typed parameter name that represents instructions perf + * event which can be used to measure the count of instructions + * by applications running on the platform. It corresponds to the + * "perf.instructions" field in the *Stats APIs. + */ +# define VIR_PERF_PARAM_INSTRUCTIONS "instructions" + +/** + * VIR_PERF_PARAM_CPU_CYCLES: + * + * Macro for typed parameter name that represents cpu_cycles perf event + * which can be used to measure how many cpu cycles one instruction needs. + * It corresponds to the "perf.cpu_cycles" field in the *Stats APIs. + */ +# define VIR_PERF_PARAM_CPU_CYCLES "cpu_cycles" + int virDomainGetPerfEvents(virDomainPtr dom, virTypedParameterPtr *params, int *nparams, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 4e71a94..b383cbb 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11452,6 +11452,14 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "perf.mbml" - the amount of data (bytes/s) sent through the memory controller * on the socket as unsigned long long. It is produced by mbml * perf event. + * "perf.cache_misses" - the count of cache misses as unsigned long long. + * It is produced by cache_misses perf event. + * "perf.cache_references" - the count of cache hits as unsigned long long. + * It is produced by cache_references perf event. + * "perf.instructions" - the count of instructions as unsigned long long. + * It is produced by instructions perf event. + * "perf.cpu_cycles" - the number of cpu cycles one instruction needs as unsigned + * long long. It is produced by cpu_cycles perf event.

These needed some formatting tweaks to follow the first 3...

* * Note that entire stats groups or individual stat fields may be missing from * the output in case they are not supported by the given hypervisor, are not diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1fdb7b8..25985b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9616,6 +9616,10 @@ qemuDomainSetPerfEvents(virDomainPtr dom, VIR_PERF_PARAM_CMT, VIR_TYPED_PARAM_BOOLEAN, VIR_PERF_PARAM_MBMT, VIR_TYPED_PARAM_BOOLEAN, VIR_PERF_PARAM_MBML, VIR_TYPED_PARAM_BOOLEAN, + VIR_PERF_PARAM_CPU_CYCLES, VIR_TYPED_PARAM_BOOLEAN, + VIR_PERF_PARAM_INSTRUCTIONS, VIR_TYPED_PARAM_BOOLEAN, + VIR_PERF_PARAM_CACHE_REFERENCES, VIR_TYPED_PARAM_BOOLEAN, + VIR_PERF_PARAM_CACHE_MISSES, VIR_TYPED_PARAM_BOOLEAN, NULL) < 0) return -1;

diff --git a/src/util/virperf.c b/src/util/virperf.c index 01c7c70..f9c6a1b 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -38,7 +38,9 @@ VIR_LOG_INIT("util.perf"); #define VIR_FROM_THIS VIR_FROM_PERF

VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST, - "cmt", "mbmt", "mbml"); + "cmt", "mbmt", "mbml", + "cpu_cycles", "instructions", + "cache_references", "cache_misses");

struct virPerfEvent { int type; @@ -107,6 +109,18 @@ static struct virPerfEventAttr { {.type = VIR_PERF_EVENT_CMT, .attrType = 0, .attrConfig = 1}, {.type = VIR_PERF_EVENT_MBMT, .attrType = 0, .attrConfig = 2}, {.type = VIR_PERF_EVENT_MBML, .attrType = 0, .attrConfig = 3}, + {.type = VIR_PERF_EVENT_CPU_CYCLES, + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_CPU_CYCLES}, + {.type = VIR_PERF_EVENT_INSTRUCTIONS, + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_INSTRUCTIONS}, + {.type = VIR_PERF_EVENT_CACHE_REFERENCES, + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_CACHE_REFERENCES}, + {.type = VIR_PERF_EVENT_CACHE_MISSES, + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_CACHE_MISSES}, }; typedef struct virPerfEventAttr *virPerfEventAttrPtr;

diff --git a/src/util/virperf.h b/src/util/virperf.h index acb59ab..bca49f7 100644 --- a/src/util/virperf.h +++ b/src/util/virperf.h @@ -35,6 +35,11 @@ typedef enum { VIR_PERF_EVENT_MBMT, VIR_PERF_EVENT_MBML,

+ VIR_PERF_EVENT_CPU_CYCLES, + VIR_PERF_EVENT_INSTRUCTIONS, + VIR_PERF_EVENT_CACHE_REFERENCES, + VIR_PERF_EVENT_CACHE_MISSES, +

Added some comments for each. John

VIR_PERF_EVENT_LAST } virPerfEventType;

diff --git a/tests/genericxml2xmlindata/generic-perf.xml b/tests/genericxml2xmlindata/generic-perf.xml index 394d2a6..a914133 100644 --- a/tests/genericxml2xmlindata/generic-perf.xml +++ b/tests/genericxml2xmlindata/generic-perf.xml @@ -16,6 +16,10 @@ <event name='cmt' enabled='yes'/> <event name='mbmt' enabled='no'/> <event name='mbml' enabled='yes'/> + <event name='cpu_cycles' enabled='no'/> + <event name='instructions' enabled='yes'/> + <event name='cache_references' enabled='no'/> + <event name='cache_misses' enabled='no'/> </perf> <devices> </devices>

On 07/16/2016 04:15 AM, Qiaowei Ren wrote:

This patch add 'domstats --perf' description for new events in virsh.pod.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com> --- tools/virsh.pod | 4 ++++ 1 file changed, 4 insertions(+)

This would need to be merged with the previous patch since that's where the fields are introduced.

diff --git a/tools/virsh.pod b/tools/virsh.pod index d7cd10e..da7f24f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -909,6 +909,10 @@ I<--perf> returns the statistics of all enabled perf events: "perf.cmt" - the cache usage in Byte currently used "perf.mbmt" - total system bandwidth from one level of cache "perf.mbml" - bandwidth of memory traffic for a memory controller +"perf.cpu_cycles" - the number of cpu cycles one instruction needs +"perf.instructions" - the count of instructions +"perf.cache_references" - the count of cache hits +"perf.cache_misses" - the count of caches misses

As previously stated - it'd be nice to add a reference to the "perf" section from the domstats --perf section, e.g.: See the B<perf> command for more details about each event.

I<--block> returns information about disks associated with each domain. Using the I<--backing> flag extends this information to

The update to the perf section is missing... You'll see my adjustments shortly - just cleaning up the patches a bit... John