[libvirt] [PATCH v4 2/4] perf: introduce a static attr table and refactor virPerfEventEnable() for general purpose
John Ferlan
jferlan at redhat.com
Wed Aug 3 22:18:53 UTC 2016
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 at 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,
>
More information about the libvir-list
mailing list