RDT monitoring plugin #1970

serhiyx · 2016-09-30T14:02:09Z

The rdtmon plugin collects information provided by monitoring features of Intel Resource Director Technology (Intel(R) RDT) like Cache Monitoring Technology (CMT), Memory Bandwidth Monitoring (MBM). CMT monitors last level cache occupancy (LLC). MBM supports two types of events reporting local and remote memory bandwidth. Local memory bandwidth (MBL) reports the bandwidth of accessing memory associated with the local socket. Remote memory bandwidth (MBR) reports the bandwidth of accessing the remote socket. Also this technology allows to monitor instructions per clock (IPC).
All events are reported on a per core basis. Monitoring of the events can be configured for group of cores.
Plugin uses PQoS library for Intel(R) RDT. https://github.com/01org/intel-cmt-cat

The rdtmon plugin collects information provided by monitoring features of Intel Resource Director Technology (Intel(R) RDT) like Cache Monitoring Technology (CMT), Memory Bandwidth Monitoring (MBM). Change-Id: Ie45344c1035c522fcd918a1dd2427a2da2e173bb Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

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

octo

Hi @serhiyx, thank you very much for your PR! Overall the code looks good, the review comments are mostly simple coding style issues that are easily fixable.

My biggest concern is the mbm type with its six data sources. We need to break this up into multiple "value lists" with a single value each. I've left a more detailed comment inline.

Also I'm unsure about the llc type. What is it representing? Number of bytes in the cache? Could the bytes type serve this purpose?

Best regards,
—octo

octo · 2016-10-03T05:32:28Z

src/rdtmon.c

+
+struct rdtmon_core_group_s {
+  char *desc;
+  int num_cores;


Nit: array indexes and sizes should be of type size_t.

octo · 2016-10-03T05:32:36Z

src/rdtmon.c

+struct rdtmon_ctx_s {
+  rdtmon_core_group_t cgroups[RDTMON_MAX_CORES];
+  struct pqos_mon_data *pgroups[RDTMON_MAX_CORES];
+  int num_groups;


Nit: array indexes and sizes should be of type size_t.

octo · 2016-10-03T05:34:35Z

src/rdtmon.c

+
+    if (ret != PQOS_RETVAL_OK) {
+      ERROR(RDTMON_PLUGIN ": Error starting monitoring (pqos status=%d)", ret);
+      return (-1);


Returning here means that only groups up to a failing group are collected. I think it would be more intuitive if following groups would be collected, too, i.e. if this was a continue instead of a return.

octo · 2016-10-03T05:35:38Z

src/rdtmon.c

+  DEBUG(RDTMON_PLUGIN ": rdtmon_shutdown.");
+
+  if (g_rdtmon == NULL) {
+    ERROR(RDTMON_PLUGIN ": rdtmon_shutdown: plugin not initialized.");


Please simply return zero without an error in this case. An error should already have been printed during initialization.

octo · 2016-10-03T05:39:35Z

src/rdtmon.c

+  plugin_dispatch_values(&vl);
+}
+
+static int rdtmon_read(user_data_t *ud) {


Please add the "unused" attribute to ud: __attribute_((unused)) user_data_t *ud

octo · 2016-10-03T12:10:02Z

src/rdtmon.c

+  cg->desc = desc;
+
+  for (int i = 0; i < num_cores; i++)
+    cg->cores[i] = (unsigned)cores[i];


(rdtmon_core_group_t*)->cores is of type unsigned, all other places appear to use uint64_t. Please change either the callers of this function or the struct to unify this.

octo · 2016-10-03T12:11:31Z

src/rdtmon.c

+  assert(item != NULL);
+
+  for (int j = 0; j < item->values_num; j++) {
+    if (item->values[j].value.string != NULL &&


Please invert this logic to make the loop more readable:

if ((item->values[j].value.string == NULL) || (strlen(item->values[j].value.string) == 0)) continue; /* rest of for-loop */

octo · 2016-10-03T12:14:46Z

src/rdtmon.c

+
+static rdtmon_ctx_t *g_rdtmon = NULL;
+
+static int isdup(const uint64_t *nums, unsigned size, uint64_t val) {


Please use size_t for array sizes and indexes.

octo · 2016-10-03T12:15:04Z

src/rdtmon.c

+ */
+static unsigned strlisttonums(char *s, uint64_t *nums, unsigned max) {
+  int ret;
+  unsigned index = 0;


Please use size_t for array sizes and indexes.

octo · 2016-10-03T12:17:00Z

src/rdtmon.c

+      ret = strtouint64(p + 1, &end);
+      if (ret < 0)
+        return (0);
+      if (start > end) {


Is 8-4 really a syntax you want to support? Please consider returning an error instead.

1. use size_t type for all arrays and indexes 2. change malloc()/memset() to calloc() 3. fix minor code style issues 4. add range validation of core id values 5. use 'bytes' type for LLC value 6. add 'memory_bandwidth' type for MBM values Change-Id: I5e577dcda19bc9799e7b79f9d0334c6f21b60f0d Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>

serhiyx · 2016-10-05T09:22:16Z

hi @octo, thanks for your feedback. I've updated PR with fixes for all review comments.

octo · 2016-10-05T14:37:25Z

Hi @serhiyx, thank you very much for your update! The code looks good to me.

One suggestion: the "mon" in "rdtmon" doesn't add much information. Is this named after something that users of this technology are already familiar with, such as a command line tool exposing the same metrics? If not, what do you think about renaming this plugin to "intel_rdt"?

Best regards,
—octo

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

serhiyx · 2016-10-06T08:17:22Z

Hi @octo ,

"intel_rdt" is good name for this plugin. Will implement the changes and update the review.

Regards,
Serhiy

octo · 2016-10-06T20:42:13Z

Thanks @serhiyx!

The rdtmon plugin collects information provided by monitoring features of Intel Resource Director Technology (Intel(R) RDT) like Cache Monitoring Technology (CMT), Memory Bandwidth Monitoring (MBM). CMT monitors last level cache occupancy (LLC). MBM supports two types of events reporting local and remote memory bandwidth. Local memory bandwidth (MBL) reports the bandwidth of accessing memory associated with the local socket. Remote memory bandwidth (MBR) reports the bandwidth of accessing the remote socket. Also this technology allows to monitor instructions per clock (IPC). All events are reported on a per core basis. Monitoring of the events can be configured for group of cores. Plugin uses PQoS library for Intel(R) RDT. This is collectd/collectd#1970 Change-Id: I3d126e3325c82e7f2a9a95c9152d899f17225f87

serhiyx added 2 commits September 30, 2016 07:08

rdtmon: Fix formatting of collectd pod file.

81b4604

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

octo added Pending feedback New plugin labels Sep 30, 2016

octo requested changes Oct 3, 2016

View reviewed changes

octo added Pending contributor action and removed Pending feedback labels Oct 5, 2016

rdtmon: Rename 'rdtmon' plugin to 'intel_rdt'.

141fd30

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

octo added Pending feedback and removed Pending contributor action labels Oct 6, 2016

octo merged commit 141fd30 into collectd:master Oct 6, 2016

maryamtahhan deleted the feat_rdtmon branch October 25, 2016 08:58

RDT monitoring plugin #1970

RDT monitoring plugin #1970

serhiyx commented Sep 30, 2016

octo left a comment

octo Oct 3, 2016

octo Oct 3, 2016

octo Oct 3, 2016

octo Oct 3, 2016

octo Oct 3, 2016

octo Oct 3, 2016

octo Oct 3, 2016

octo Oct 3, 2016 •

edited

octo Oct 3, 2016

octo Oct 3, 2016

serhiyx commented Oct 5, 2016

octo commented Oct 5, 2016 •

edited

serhiyx commented Oct 6, 2016

octo commented Oct 6, 2016


		static rdtmon_ctx_t *g_rdtmon = NULL;

		static int isdup(const uint64_t *nums, unsigned size, uint64_t val) {

RDT monitoring plugin #1970

RDT monitoring plugin #1970

Conversation

serhiyx commented Sep 30, 2016

octo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octo Oct 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiyx commented Oct 5, 2016

octo commented Oct 5, 2016 • edited

serhiyx commented Oct 6, 2016

octo commented Oct 6, 2016

octo Oct 3, 2016 •

edited

octo commented Oct 5, 2016 •

edited