Performance monitoring plugin intel_pmu #2276

serhiyx · 2017-05-12T13:48:59Z

The intel_pmu plugin reads performance counters provided by the Linux kernel perf interface. The plugin uses jevents library to resolve named events to perf events and access perf interface.

The intel_pmu plugin collects information about system performance counters using kernel Linux perf interface. Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

octo

Hi @serhiyx,

thank you very much for your patch! It looks great and I have almost exclusively coding style nits.

Best regards,
—octo

octo · 2017-05-13T07:01:11Z

src/collectd.conf.pod

+
+=over 4
+
+=item B<HWCacheEvents> B<false>|B<true>


Configuration options with this functionality are usually called ReportFoo, for example ReportNumCpu of the CPU plugin.

octo · 2017-05-13T07:01:28Z

src/collectd.conf.pod

+  - branch-loads
+  - branch-load-misses
+
+=item B<KernelPMUEvents> B<false>|B<true>


Dito: ReportKernelPMUEvents

octo · 2017-05-13T07:02:22Z

src/collectd.conf.pod

+  - branch-misses
+  - bus-cycles
+
+=item B<SWEvents> B<false>|B<true>


Dito: ReportSWEvents; please consider expanding "SW" to "Software" to make this option easier to read.

octo · 2017-05-13T07:03:50Z

src/collectd.conf.pod

+
+=item B<HWSpecificEvents> I<events>
+
+This field is a list of comma separated event names. To be able to monitor all


comma separated

Config options can take multiple arguments. Please take a list of strings instead of a single string that is then parsed inside the plugin.

Refactored this functionality to use list of strings provided by config parser. But some subparsing routine is still needed as each entry can contain multiple event names to create group of events.

octo · 2017-05-13T07:05:01Z

src/collectd.conf.pod

+  - alignment-faults
+  - emulation-faults
+
+=item B<HWSpecificEvents> I<events>


With the boolean options all having the form "Report… true|false", I'd suggest to rename this option to HardwareEvents.

octo · 2017-05-13T07:17:39Z

src/intel_pmu.c

+  return (0);
+}
+
+static int pmu_parse_events(struct eventlist *el, char *events) {


As mentioned above: the config parser can give you a list of strings – please use that instead of implementing your own (sub-)parsing.

Refactored this function to receive list of strings provided by config parser. But some subparsing routine is still needed as each entry can contain multiple event names to create group of events.

octo · 2017-05-13T07:22:07Z

src/intel_pmu.c

+            ": Events list is empty. No events were setup for monitoring.");
+  }
+
+  return (0);


Please unify this style. Both, return foo; and return (foo); are okay, just use the same everywhere in the file.

@octo
There is a WIP about parentheses removing (#2277).
Maybe syntax with parentheses should be noted as forbidden / deprecated?

Agreed, with #2277, please remove all parenthesis from this PR.

octo · 2017-05-13T07:23:32Z

src/intel_pmu.c

+
+  for (s = strtok_r(events, ",", &tmp); s; s = strtok_r(NULL, ",", &tmp)) {
+    bool group_leader = false, end_group = false;
+    int len;


This should be a size_t.

octo · 2017-05-13T07:27:26Z

src/intel_pmu.c

+
+  ret = read_all_events(g_ctx.event_list);
+  if (ret != 0) {
+    DEBUG(PMU_PLUGIN ": Failed to read values of all events.");


This should be an ERROR() since you abort and return prematurely.

octo · 2017-05-13T07:27:40Z

src/intel_pmu.c

+
+  ret = pmu_dispatch_data();
+  if (ret != 0) {
+    DEBUG(PMU_PLUGIN ": Failed to dispatch event values.");


Dito: Use ERROR() here.

Change-Id: I6ec3cb1602b96e76b75339a79e7768b39b2c323a Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

rubenk · 2017-05-26T10:35:24Z

contrib/systemd.collectd.service

+# Environment=XDG_CACHE_HOME=/opt
+#
+# By default, the HOME directory is chosen:
+Environment=


Instead of getting this from the environment, would it be possible to turn this into a config option settable in collectd.conf?

This environment variable is not used by the plugin itself. It is used inside jevents library that is used by the plugin.

And there's no way to pass this to jevents except from the environment?

Yes, it is possible. Will rework the solution to use config option and get rid of using of environment variable..

rubenk · 2017-05-26T10:41:15Z

configure.ac

+# --with-libjevents {{{
+with_libjevents_cppflags=""
+with_libjevents_ldflags=""
+AC_ARG_WITH(libjevents, [AS_HELP_STRING([--with-libjevents@<:@=PREFIX@:>@], [Path to libjevents.])],


Please keep the same code style as the rest of configure.ac

rubenk · 2017-05-26T10:41:43Z

configure.ac

+fi
+if test "x$with_libjevents" = "xyes"
+then
+  SAVE_CPPFLAGS="$CPPFLAGS"


AC_CHECK_LIB doesn't use CPPFLAGS, so no need to save them.

rubenk · 2017-05-26T10:43:04Z

configure.ac

+  SAVE_LDFLAGS="$LDFLAGS"
+  SAVE_LIBS="$LIBS"
+  CPPFLAGS="$CPPFLAGS -fPIC"
+  LDFLAGS="$LDFLAGS -shared"


Setting -fPIC and -shared is not needed, you're creating an executable, not a library.

Actually I want to create shared object that is linked with jevents library just to check if jevents was compiled with -fPIC. I've added this logic to avoid errors during collectd installation/compilation - if collectd is compiled on the system with jevents installed we check if jevents version is suitable for us and if not just disable intel_pmu plugin during configure.

I think it's more trouble than it's worth, you've already a note in the README that jevents should be position independent.

Agree. Will remove this.

rubenk · 2017-05-26T10:44:24Z

configure.ac

+      }
+    ]]
+  )],
+  [with_libjevents="yes"], [with_libjevents="no (could not link to libjevents. Check jevents is compiled with -fPIC.)"])


I don't understand this. If libjevents is not compiled with -fPIC, it is not a shared library.

By default jevents library is compiled without -fPIC. But this flag is required when jevents library is linked with pmu plugin (intel_pmu.so) to avoid the following error:

/usr/bin/ld: /usr/local/lib64/libjevents.a(cache.o): relocation R_X86_64_32S against .bss' can not be used when making a shared object; recompile with -fPIC`

rubenk · 2017-05-26T10:45:10Z

configure.ac

+  BUILD_WITH_LIBJEVENTS_CPPFLAGS="$with_libjevents_cppflags"
+  BUILD_WITH_LIBJEVENTS_LDFLAGS="$with_libjevents_ldflags"
+  BUILD_WITH_LIBJEVENTS_LIBS="-ljevents"
+  AC_SUBST(BUILD_WITH_LIBJEVENTS_CPPFLAGS)


You should not use AC_SUBST in a conditional statement. Please move it below the fi.

- cleanup of configure.ac - add config option EventList to avoid dependency on environment variables Change-Id: I987095b6e71bcf05103e773793c0a40c815f729b Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

rubenk · 2017-05-30T10:11:23Z

README

@@ -240,7 +245,7 @@ Features

    - netapp
      Plugin to query performance values from a NetApp storage system using the
-      “Manage ONTAP” SDK provided by NetApp.
+      “Manage ONTAP” SDK provided by NetApp.


What changed on this line?

must have been fixed by my editor. no-break space symbol (c2a0) was changed to space symbol (20).
Should I revert this change?

Heh, nice one. No problem, please leave it in, I prefer spaces anyway :)

rubenk · 2017-05-30T10:11:59Z

configure.ac

+AC_ARG_WITH([libjevents],
+  [AS_HELP_STRING([--with-libjevents@<:@=PREFIX@:>@], [Path to libjevents.])],
+  [
+    if test "x$withval" != "xno" && test "x$withval" != "xyes"


put if and then on the same line please.

rubenk · 2017-05-30T10:12:14Z

configure.ac

+  SAVE_CPPFLAGS="$CPPFLAGS"
+  CPPFLAGS="$CPPFLAGS $with_libjevents_cppflags"
+
+  AC_CHECK_HEADERS(jevents.h, [with_libjevents="yes"], [with_libjevents="no (jevents.h not found)"])


please quote all the arguments.

rubenk · 2017-05-30T10:12:43Z

configure.ac

+  SAVE_LDFLAGS="$LDFLAGS"
+  LDFLAGS="$LDFLAGS $with_libjevents_ldflags"
+
+  AC_CHECK_LIB(jevents, json_events, [with_libjevents="yes"], [with_libjevents="no (Can't find libjevents)"])


please quote all the arguments, and break this long line.

rubenk · 2017-05-30T10:13:00Z

configure.ac

+  BUILD_WITH_LIBJEVENTS_LDFLAGS="$with_libjevents_ldflags"
+  BUILD_WITH_LIBJEVENTS_LIBS="-ljevents"
+fi
+AC_SUBST(BUILD_WITH_LIBJEVENTS_CPPFLAGS)


please quote the argument.

Change-Id: Ie69b921a253f418ac7b87248faa29d9d35c647a0 Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

rubenk · 2017-06-09T12:55:53Z

Makefile.am

+if BUILD_PLUGIN_INTEL_PMU
+pkglib_LTLIBRARIES += intel_pmu.la
+intel_pmu_la_SOURCES = src/intel_pmu.c
+intel_pmu_la_CFLAGS = $(AM_CFLAGS) $(BUILD_WITH_LIBJEVENTS_CPPFLAGS)


BUILD_WITH_LIBJEVENTS_CPPFLAGS should be added to intel_pmu_la_CPPFLAGS.

rubenk · 2017-06-09T13:00:46Z

src/intel_pmu.c

+    DEBUG(PMU_PLUGIN ":     size      : %d", e->attr.size);
+  }
+
+  return;


no need to return from a void function.

rubenk · 2017-06-09T13:01:03Z

src/intel_pmu.c

+    DEBUG(PMU_PLUGIN ":   hardware_events[%zu]: %s", i, g_ctx.hw_events[i]);
+  }
+
+  return;


no need to return from a void function.

rubenk · 2017-06-09T13:01:52Z

src/intel_pmu.c

+}
+
+static int pmu_config(oconfig_item_t *ci) {
+  int ret = 0;


can you reduce the scope by moving this into the for loop?

rubenk · 2017-06-09T13:02:06Z

src/intel_pmu.c

+      ret = cf_util_get_boolean(child, &g_ctx.sw_events);
+    } else {
+      ERROR(PMU_PLUGIN ": Unknown configuration parameter \"%s\".", child->key);
+      ret = (-1);


no need for the braces.

rubenk · 2017-06-09T13:03:22Z

src/intel_pmu.c

+  plugin_dispatch_values(&vl);
+}
+
+static int pmu_dispatch_data(void) {


this function always returns 0, make it void?

rubenk · 2017-06-09T13:04:04Z

src/intel_pmu.c

+  }
+
+  ret = pmu_dispatch_data();
+  if (ret != 0) {


this is a no-op, pmu_dispatch_data always returns 0.

rubenk · 2017-06-09T13:19:11Z

src/intel_pmu.c

+  ret = read_all_events(g_ctx.event_list);
+  if (ret != 0) {
+    ERROR(PMU_PLUGIN ": Failed to read values of all events.");
+    return 0;


why not return an error?

rubenk · 2017-06-09T13:19:37Z

src/intel_pmu.c

+  ret = pmu_dispatch_data();
+  if (ret != 0) {
+    ERROR(PMU_PLUGIN ": Failed to dispatch event values.");
+    return 0;


why not return an error?

rubenk · 2017-06-09T13:25:22Z

src/intel_pmu.c

+    e->attr.type = type;
+    e->attr.config = events[i].config;
+    e->attr.size = PERF_ATTR_SIZE_VER0;
+    e->next = NULL;


this is a no-op, the struct has already been zeroed by calloc.

rubenk · 2017-06-09T13:26:52Z

src/intel_pmu.c

+}
+
+static int pmu_add_events(struct eventlist *el, uint32_t type,
+                          event_info_t *events, int count) {


count should be size_t.

rubenk · 2017-06-09T13:33:01Z

@serhiyx I left a few more review comments. Once these are resolved I think this is good to go in. Feel free to also add yourself to AUTHORS if you want to.

Change-Id: I172ad8535edda429cd58e0d6198aa19af76151eb Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

rubenk · 2017-06-13T08:18:46Z

@octo I think this is good to go in, and think all your remarks have been addressed. Do you want to merge this?

octo · 2017-07-19T08:02:17Z

Thank you very much for your great work, @serhiyx!

serhiyx added 2 commits May 12, 2017 14:40

intel_pmu: Implement performance monitoring plugin

08e419e

The intel_pmu plugin collects information about system performance counters using kernel Linux perf interface. Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

intel_pmu: formatting by clang-format

b1931a0

Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

octo requested changes May 13, 2017

View reviewed changes

octo added the New plugin label May 13, 2017

octo self-assigned this May 19, 2017

intel_pmu: address PR comments

7a8d537

Change-Id: I6ec3cb1602b96e76b75339a79e7768b39b2c323a Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

rubenk reviewed May 26, 2017

View reviewed changes

intel_pmu: address PR comments

7853bfa

- cleanup of configure.ac - add config option EventList to avoid dependency on environment variables Change-Id: I987095b6e71bcf05103e773793c0a40c815f729b Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

rubenk reviewed May 30, 2017

View reviewed changes

intel_pmu: configure.ac code style cleanup

7ce65d4

Change-Id: Ie69b921a253f418ac7b87248faa29d9d35c647a0 Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

rubenk reviewed Jun 9, 2017

View reviewed changes

src/intel_pmu.c Outdated

DEBUG(PMU_PLUGIN ": size : %d", e->attr.size);

}

return;

Copy link

Contributor

rubenk Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to return from a void function.

rubenk reviewed Jun 9, 2017

View reviewed changes

src/intel_pmu.c Outdated

}

ret = pmu_dispatch_data();

if (ret != 0) {

Copy link

Contributor

rubenk Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a no-op, pmu_dispatch_data always returns 0.

rubenk reviewed Jun 9, 2017

View reviewed changes

intel_pmu: address PR comments

ebffc2b

Change-Id: I172ad8535edda429cd58e0d6198aa19af76151eb Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

octo approved these changes Jul 19, 2017

View reviewed changes

octo merged commit ebffc2b into collectd:master Jul 19, 2017

maryamtahhan deleted the feat_intel_pmu branch July 19, 2017 14:19

octo added this to the 5.8 milestone Oct 11, 2017

maryamtahhan mentioned this pull request Oct 12, 2017

Next collectd release #2420

Closed

19 tasks


		=item B<HWSpecificEvents> I<events>

		This field is a list of comma separated event names. To be able to monitor all

Performance monitoring plugin intel_pmu #2276

Performance monitoring plugin intel_pmu #2276

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment