6:27 a.m.
This was previously posted & acked here:
https://www.redhat.com/archives/libvir-list/2016-December/msg00795.html
but then i forgot to push it and also notice a bug in the first place.
There were conflicts against current master too, so I'm reposting for
new ACK..
Daniel P. Berrange (2):
perf: get rid of pointless virPerfGetEventAttr() method
perf: get rid of pointless virPerfGetEvent() method
src/util/virperf.c | 185 +++++++++++++++++++++--------------------------------
1 file changed, 73 insertions(+), 112 deletions(-)
--
2.9.3
6:27 a.m.
New subject: [libvirt] [PATCH v2 1/2] perf: get rid of pointless virPerfGetEventAttr() method
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(a)redhat.com>
---
src/util/virperf.c | 143 +++++++++++++++++++++++++----------------------------
1 file changed, 66 insertions(+), 77 deletions(-)
diff --git a/src/util/virperf.c b/src/util/virperf.c
index e39cebb..7366efc 100644
--- a/src/util/virperf.c
+++ b/src/util/virperf.c
@@ -71,102 +71,91 @@ 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
},
- {.type = VIR_PERF_EVENT_CPU_CLOCK,
- .attrType = PERF_TYPE_SOFTWARE,
+ {.attrType = PERF_TYPE_SOFTWARE,
.attrConfig = PERF_COUNT_SW_CPU_CLOCK},
- {.type = VIR_PERF_EVENT_TASK_CLOCK,
- .attrType = PERF_TYPE_SOFTWARE,
+ {.attrType = PERF_TYPE_SOFTWARE,
.attrConfig = PERF_COUNT_SW_TASK_CLOCK},
- {.type = VIR_PERF_EVENT_PAGE_FAULTS,
- .attrType = PERF_TYPE_SOFTWARE,
+ {.attrType = PERF_TYPE_SOFTWARE,
.attrConfig = PERF_COUNT_SW_PAGE_FAULTS},
- {.type = VIR_PERF_EVENT_CONTEXT_SWITCHES,
- .attrType = PERF_TYPE_SOFTWARE,
+ {.attrType = PERF_TYPE_SOFTWARE,
.attrConfig = PERF_COUNT_SW_CONTEXT_SWITCHES},
- {.type = VIR_PERF_EVENT_CPU_MIGRATIONS,
- .attrType = PERF_TYPE_SOFTWARE,
+ {.attrType = PERF_TYPE_SOFTWARE,
.attrConfig = PERF_COUNT_SW_CPU_MIGRATIONS},
- {.type = VIR_PERF_EVENT_PAGE_FAULTS_MIN,
- .attrType = PERF_TYPE_SOFTWARE,
+ {.attrType = PERF_TYPE_SOFTWARE,
.attrConfig = PERF_COUNT_SW_PAGE_FAULTS_MIN},
- {.type = VIR_PERF_EVENT_PAGE_FAULTS_MAJ,
- .attrType = PERF_TYPE_SOFTWARE,
+ {.attrType = PERF_TYPE_SOFTWARE,
.attrConfig = PERF_COUNT_SW_PAGE_FAULTS_MAJ},
- {.type = VIR_PERF_EVENT_ALIGNMENT_FAULTS,
- .attrType = PERF_TYPE_SOFTWARE,
+ {.attrType = PERF_TYPE_SOFTWARE,
.attrConfig = PERF_COUNT_SW_ALIGNMENT_FAULTS},
- {.type = VIR_PERF_EVENT_EMULATION_FAULTS,
- .attrType = PERF_TYPE_SOFTWARE,
+ {.attrType = PERF_TYPE_SOFTWARE,
.attrConfig = PERF_COUNT_SW_EMULATION_FAULTS},
};
+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)
{
@@ -224,9 +213,9 @@ 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->enabled)
@@ -237,7 +226,7 @@ virPerfEventEnable(virPerfPtr perf,
type == VIR_PERF_EVENT_MBML)) {
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
_("unable to enable host cpu perf event for %s"),
- virPerfEventTypeToString(event->type));
+ virPerfEventTypeToString(type));
return -1;
}
--
2.9.3
8:26 a.m.
New subject: [libvirt] [PATCH v2 1/2] perf: get rid of pointless virPerfGetEventAttr() method
On Wed, Apr 12, 2017 at 12:27:11PM +0100, Daniel P. Berrange wrote:
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(a)redhat.com>
---
src/util/virperf.c | 143 +++++++++++++++++++++++++----------------------------
1 file changed, 66 insertions(+), 77 deletions(-)
diff --git a/src/util/virperf.c b/src/util/virperf.c
index e39cebb..7366efc 100644
--- a/src/util/virperf.c
+++ b/src/util/virperf.c
[...]
+ [VIR_PERF_EVENT_STALLED_CYCLES_BACKEND] = {
+ .attrType = PERF_TYPE_HARDWARE,
+ .attrConfig = PERF_COUNT_HW_STALLED_CYCLES_BACKEND
+ },
+ [VIR_PERF_EVENT_REF_CPU_CYCLES] = {
Up to VIR_PERF_EVENT_REF_CPU_CYCLES, the initialization is indexed
by the enum value...
# 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
},
- {.type = VIR_PERF_EVENT_CPU_CLOCK,
- .attrType = PERF_TYPE_SOFTWARE,
+ {.attrType = PERF_TYPE_SOFTWARE,
.attrConfig = PERF_COUNT_SW_CPU_CLOCK},
... but the rest does not.
ACK with the inconsitency fixed.
Jan
8:33 a.m.
New subject: [libvirt] [PATCH v2 1/2] perf: get rid of pointless virPerfGetEventAttr() method
On Wed, Apr 12, 2017 at 03:26:36PM +0200, Ján Tomko wrote:
On Wed, Apr 12, 2017 at 12:27:11PM +0100, Daniel P. Berrange wrote:
> 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(a)redhat.com>
> ---
> src/util/virperf.c | 143 +++++++++++++++++++++++++----------------------------
> 1 file changed, 66 insertions(+), 77 deletions(-)
>
> diff --git a/src/util/virperf.c b/src/util/virperf.c
> index e39cebb..7366efc 100644
> --- a/src/util/virperf.c
> +++ b/src/util/virperf.c
[...]
> + [VIR_PERF_EVENT_STALLED_CYCLES_BACKEND] = {
> + .attrType = PERF_TYPE_HARDWARE,
> + .attrConfig = PERF_COUNT_HW_STALLED_CYCLES_BACKEND
> + },
> + [VIR_PERF_EVENT_REF_CPU_CYCLES] = {
Up to VIR_PERF_EVENT_REF_CPU_CYCLES, the initialization is indexed
by the enum value...
> # 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
> },
> - {.type = VIR_PERF_EVENT_CPU_CLOCK,
> - .attrType = PERF_TYPE_SOFTWARE,
> + {.attrType = PERF_TYPE_SOFTWARE,
> .attrConfig = PERF_COUNT_SW_CPU_CLOCK},
... but the rest does not.
ACK with the inconsitency fixed.
Opps, yes, my mistake when resolving the conflict
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
6:27 a.m.
New subject: [libvirt] [PATCH v2 2/2] perf: get rid of pointless virPerfGetEvent() method
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(a)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 7366efc..a04bb67 100644
--- a/src/util/virperf.c
+++ b/src/util/virperf.c
@@ -50,7 +50,6 @@ VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST,
"alignment_faults", "emulation_faults");
struct virPerfEvent {
- int type;
int fd;
bool enabled;
union {
@@ -188,23 +187,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,
@@ -212,12 +194,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->enabled)
return 0;
@@ -256,14 +235,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;
}
@@ -280,9 +259,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;
@@ -290,7 +267,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;
}
@@ -302,11 +279,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
@@ -314,8 +287,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) {
@@ -386,7 +359,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
8:26 a.m.
New subject: [libvirt] [PATCH v2 2/2] perf: get rid of pointless virPerfGetEvent() method
On Wed, Apr 12, 2017 at 12:27:12PM +0100, Daniel P. Berrange wrote:
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(a)redhat.com>
---
src/util/virperf.c | 44 ++++++++------------------------------------
1 file changed, 8 insertions(+), 36 deletions(-)
ACK
Jan
2781
days inactive
2781
days old
5 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Ján Tomko