Re: [libvirt] [PATCH v4 2/4] perf: introduce a static attr table and refactor virPerfEventEnable() for general purpose

[John Ferlan]

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 
> ---
>  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 
>  
> -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, ) < 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, _type) < 0) {
> +if (virStrToLong_ui(buf, NULL, 10, _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(_attr, 0, sizeof(rdt_attr));
> -rdt_attr.size = 

[libvirt] [PATCH v4 2/4] perf: introduce a static attr table and refactor virPerfEventEnable() for general purpose

[Qiaowei Ren]

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 
---
 src/util/virperf.c | 160 ++---
 src/util/virperf.h |   6 ++
 2 files changed, 97 insertions(+), 69 deletions(-)

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);
+
 virPerfPtr
 virPerfNew(void)
 {
@@ -72,6 +74,8 @@ virPerfNew(void)
 perf->events[i].enabled = false;
 }
 
+virPerfRdtAttrInit();
+
 return perf;
 }
 
@@ -95,12 +99,21 @@ virPerfFree(virPerfPtr perf)
 
 # include 
 
-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)
 {
-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, ) < 0)
@@ -128,48 +144,74 @@ virPerfRdtEnable(virPerfEventPtr event,
 if ((tmp = strchr(buf, '\n')))
 *tmp = '\0';
 
-if (virStrToLong_ui(buf, NULL, 10, _type) < 0) {
+if (virStrToLong_ui(buf, NULL, 10, _type) < 0) {
 virReportSystemError(errno, "%s",
  _("failed to get rdt event type"));
 goto error;
 }
 VIR_FREE(buf);
 
-memset(_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, ) < 0)
-goto error;
-
-if (virStrToLong_ui(buf, NULL, 10, ) < 0) {
-virReportSystemError(errno, "%s",
- _("failed to get cmt scaling factor"));
-goto error;
-}
-event->efields.cmt.scale = scale;
-
-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;
+}
+
+ 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, _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