[libvirt] [PATCH 0/2] Simplify perf events handling a little
This does two simplifications to the perf events code. Daniel P. Berrange (2): perf: get rid of pointless virPerfGetEventAttr() method perf: get rid of pointless virPerfGetEvent() method src/util/virperf.c | 162 +++++++++++++++++++++-------------------------------- 1 file changed, 65 insertions(+), 97 deletions(-) -- 2.9.3
The virPerfGetEventAttr method contains a totally pointless loop. Remove it, verify the array size statically, and then just use an array index to access the perf event. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virperf.c | 120 ++++++++++++++++++++++++++--------------------------- 1 file changed, 58 insertions(+), 62 deletions(-) diff --git a/src/util/virperf.c b/src/util/virperf.c index f64692b..c8279c9 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -67,75 +67,73 @@ struct virPerf { # include <linux/perf_event.h> struct virPerfEventAttr { - int type; unsigned int attrType; unsigned long long attrConfig; }; static struct virPerfEventAttr 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}, - {.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}, - {.type = VIR_PERF_EVENT_BRANCH_INSTRUCTIONS, - .attrType = PERF_TYPE_HARDWARE, - .attrConfig = PERF_COUNT_HW_BRANCH_INSTRUCTIONS}, - {.type = VIR_PERF_EVENT_BRANCH_MISSES, - .attrType = PERF_TYPE_HARDWARE, - .attrConfig = PERF_COUNT_HW_BRANCH_MISSES}, - {.type = VIR_PERF_EVENT_BUS_CYCLES, - .attrType = PERF_TYPE_HARDWARE, - .attrConfig = PERF_COUNT_HW_BUS_CYCLES}, - {.type = VIR_PERF_EVENT_STALLED_CYCLES_FRONTEND, - .attrType = PERF_TYPE_HARDWARE, - .attrConfig = PERF_COUNT_HW_STALLED_CYCLES_FRONTEND}, - {.type = VIR_PERF_EVENT_STALLED_CYCLES_BACKEND, - .attrType = PERF_TYPE_HARDWARE, - .attrConfig = PERF_COUNT_HW_STALLED_CYCLES_BACKEND}, - {.type = VIR_PERF_EVENT_REF_CPU_CYCLES, + [VIR_PERF_EVENT_CMT] = { + .attrType = 0, + .attrConfig = 1 + }, + [VIR_PERF_EVENT_MBMT] = { + .attrType = 0, + .attrConfig = 2 + }, + [VIR_PERF_EVENT_MBML] { + .attrType = 0, + .attrConfig = 3 + }, + [VIR_PERF_EVENT_CPU_CYCLES] = { + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_CPU_CYCLES + }, + [VIR_PERF_EVENT_INSTRUCTIONS] = { + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_INSTRUCTIONS + }, + [VIR_PERF_EVENT_CACHE_REFERENCES] = { + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_CACHE_REFERENCES + }, + [VIR_PERF_EVENT_CACHE_MISSES] = { + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_CACHE_MISSES + }, + [VIR_PERF_EVENT_BRANCH_INSTRUCTIONS] = { + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_BRANCH_INSTRUCTIONS + }, + [VIR_PERF_EVENT_BRANCH_MISSES] = { + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_BRANCH_MISSES + }, + [VIR_PERF_EVENT_BUS_CYCLES] = { + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_BUS_CYCLES + }, + [VIR_PERF_EVENT_STALLED_CYCLES_FRONTEND] = { + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_STALLED_CYCLES_FRONTEND + }, + [VIR_PERF_EVENT_STALLED_CYCLES_BACKEND] = { + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_STALLED_CYCLES_BACKEND + }, + [VIR_PERF_EVENT_REF_CPU_CYCLES] = { # ifdef PERF_COUNT_HW_REF_CPU_CYCLES - .attrType = PERF_TYPE_HARDWARE, - .attrConfig = PERF_COUNT_HW_REF_CPU_CYCLES + .attrType = PERF_TYPE_HARDWARE, + .attrConfig = PERF_COUNT_HW_REF_CPU_CYCLES # else - .attrType = 0, - .attrConfig = 0, + .attrType = 0, + .attrConfig = 0, # endif }, }; +verify(ARRAY_CARDINALITY(attrs) == VIR_PERF_EVENT_LAST); typedef struct virPerfEventAttr *virPerfEventAttrPtr; -static virPerfEventAttrPtr -virPerfGetEventAttr(virPerfEventType type) -{ - size_t i; - if (type >= VIR_PERF_EVENT_LAST) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Event '%d' is not supported"), - type); - return NULL; - } - - for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { - if (i == type) - return attrs + i; - } - - return NULL; -} - - static int virPerfRdtAttrInit(void) { @@ -193,17 +191,15 @@ virPerfEventEnable(virPerfPtr perf, char *buf = NULL; struct perf_event_attr attr; virPerfEventPtr event = virPerfGetEvent(perf, type); - virPerfEventAttrPtr event_attr = virPerfGetEventAttr(type); + virPerfEventAttrPtr event_attr = &attrs[type]; - if (!event || !event_attr) + if (!event) return -1; - if (event_attr->attrType == 0 && (type == VIR_PERF_EVENT_CMT || - type == VIR_PERF_EVENT_MBMT || - type == VIR_PERF_EVENT_MBML)) { + if (event_attr->attrType == 0) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, _("unable to enable host cpu perf event for %s"), - virPerfEventTypeToString(event->type)); + virPerfEventTypeToString(type)); return -1; } -- 2.9.3
The virPerfGetEvent method pointlessly checks for a NULL parameter and the range of an enum value. The whole point of using an enum is that we can avoid such checks. Just replace calls to virPerfGetEvent, with perf->events[type] array access. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virperf.c | 44 ++++++++------------------------------------ 1 file changed, 8 insertions(+), 36 deletions(-) diff --git a/src/util/virperf.c b/src/util/virperf.c index c8279c9..edb9f54 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -46,7 +46,6 @@ VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST, "stalled_cycles_backend", "ref_cpu_cycles"); struct virPerfEvent { - int type; int fd; bool enabled; union { @@ -166,23 +165,6 @@ virPerfRdtAttrInit(void) } -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; - } - - return perf->events + type; -} - int virPerfEventEnable(virPerfPtr perf, virPerfEventType type, @@ -190,12 +172,9 @@ virPerfEventEnable(virPerfPtr perf, { char *buf = NULL; struct perf_event_attr attr; - virPerfEventPtr event = virPerfGetEvent(perf, type); + virPerfEventPtr event = &(perf->events[type]); virPerfEventAttrPtr event_attr = &attrs[type]; - if (!event) - return -1; - if (event_attr->attrType == 0) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, _("unable to enable host cpu perf event for %s"), @@ -229,14 +208,14 @@ virPerfEventEnable(virPerfPtr perf, if (event->fd < 0) { virReportSystemError(errno, _("unable to open host cpu perf event for %s"), - virPerfEventTypeToString(event->type)); + virPerfEventTypeToString(type)); goto error; } if (ioctl(event->fd, PERF_EVENT_IOC_ENABLE) < 0) { virReportSystemError(errno, _("unable to enable host cpu perf event for %s"), - virPerfEventTypeToString(event->type)); + virPerfEventTypeToString(type)); goto error; } @@ -253,9 +232,7 @@ int virPerfEventDisable(virPerfPtr perf, virPerfEventType type) { - virPerfEventPtr event = virPerfGetEvent(perf, type); - if (event == NULL) - return -1; + virPerfEventPtr event = &(perf->events[type]); if (!event->enabled) return 0; @@ -263,7 +240,7 @@ virPerfEventDisable(virPerfPtr perf, if (ioctl(event->fd, PERF_EVENT_IOC_DISABLE) < 0) { virReportSystemError(errno, _("unable to disable host cpu perf event for %s"), - virPerfEventTypeToString(event->type)); + virPerfEventTypeToString(type)); return -1; } @@ -275,11 +252,7 @@ virPerfEventDisable(virPerfPtr perf, bool virPerfEventIsEnabled(virPerfPtr perf, virPerfEventType type) { - virPerfEventPtr event = virPerfGetEvent(perf, type); - if (event == NULL) - return false; - - return event->enabled; + return perf->events[type].enabled; } int @@ -287,8 +260,8 @@ virPerfReadEvent(virPerfPtr perf, virPerfEventType type, uint64_t *value) { - virPerfEventPtr event = virPerfGetEvent(perf, type); - if (event == NULL || !event->enabled) + virPerfEventPtr event = &perf->events[type]; + if (!event->enabled) return -1; if (saferead(event->fd, value, sizeof(uint64_t)) < 0) { @@ -359,7 +332,6 @@ virPerfNew(void) return NULL; for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { - perf->events[i].type = i; perf->events[i].fd = -1; perf->events[i].enabled = false; } -- 2.9.3
On Fri, Dec 16, 2016 at 11:00:37AM +0000, Daniel P. Berrange wrote:
This does two simplifications to the perf events code.
Daniel P. Berrange (2): perf: get rid of pointless virPerfGetEventAttr() method perf: get rid of pointless virPerfGetEvent() method
src/util/virperf.c | 162 +++++++++++++++++++++-------------------------------- 1 file changed, 65 insertions(+), 97 deletions(-)
ACK series. Erik
-- 2.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Daniel P. Berrange -
Erik Skultety