intel_pmu: core groups feature #2681

kwiatrox · 2018-02-09T14:03:04Z

New "Cores" option is similar to that available in intel_rdt plugin.
New tool utils_config_cores is added to parse "Cores" option. It is used by intel_pmu and intel_rdt to avoid redundant code.
Having this option will allow to configure monitoring of PMU events only on specific CPUs thus decrease number of opened descriptors and improve performance significantly.

maryamtahhan

Changes look good to me - can you please double check why some of the checks failed?

kwiatrox · 2018-02-10T07:53:26Z

Thanks @maryamtahhan , it looks like for epel6 build saveptr for strtok_r needs to be initialised. Will fix it with next commit.

kwiatrox · 2018-02-12T14:33:49Z

It seems that Jenkins has problems due to environment issue: "Slave went offline during the build".
I don't see any errors related to PR.

maryamtahhan · 2018-02-16T19:16:58Z

@kwiatrox can you rebase this against 5.8.0? It looks good to me it would be great if @rpv-tomsk or @octo could review and approve this is a great addition

Having this option will allow to configure monitoring of PMU events only on specific CPUs thus decrease number of opened descriptors significantly and avoid 'Too many open files' errors. New "Cores" option is similar to that available in intel_rdt plugin. New tool utils_config_cores is added to parse "Cores" option. Change-Id: I8f792e1f2560c4cf19aee101fdb07c925d682778 Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

Use utils_config_cores for core groups configuration so it is in line with intel_pmu and reduce amount of redundant code. Change-Id: If02e2eeea8bcf3e0df705ebcd9a6310b549b5ebe Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

Run script contrib/format.sh to format the code. Replace zu with new collectd macro PRIsz. Change-Id: I167b1065461e924d7ab260a35f85f5ab162c4165 Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

To avoid compilation error on some versions of gcc the saveptr for strtok_r needs to be initilised to NULL. Change-Id: I7b30e51ecae33a6994ba7ea181cac0f33eef023f Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

Add definition for PRIsz macro to make code backward compatible with collectd-5.8 branch. Change-Id: I880340af5ae883a444563422b3e9975b3693683c Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

kwiatrox · 2018-02-20T15:47:08Z

Code is rebased against 5.8 branch and all checks have passed.

vmytnykx

A few minor comments....

vmytnykx · 2018-02-21T17:09:42Z

src/utils_config_cores.h

+/**
+ * collectd - src/utils_config_cores.h
+ *
+ * Copyright(c) 2017 Intel Corporation. All rights reserved.


2018, please check in all new files.

vmytnykx · 2018-02-22T08:40:20Z

src/utils_config_cores.h

+#define UTILS_CONFIG_CORES_H 1
+
+#ifndef PRIsz
+#define PRIsz "zu"


What is the idea of this define? %zu is always used for size_t data type. Why don't use %zu directly in the string?

Look at the PR #2512 .

Great. So it doesn't make sense to duplicate it here. Just use src/daemon/globals.h.

It is not backported to 5.8. So it is necessary to be compatible with 5.8 branch. After merge to master it could be deleted. In current state it goes well with both master and 5.8 branches. It is easier to keep this small define than to replace %zu everywhere later on.

ok, that's fine.

vmytnykx · 2018-02-22T08:40:48Z

src/utils_config_cores.h

+ *
+ *    For empty string "" *cgl is not modified and zero is returned.
+ */
+int config_cores_parse(const oconfig_item_t *ci, core_groups_list_t *cgl);


add forward declaration.

Added include for configfile.h to avoid forward declaration.

vmytnykx · 2018-02-22T08:55:16Z

src/utils_config_cores.c

+            ci->values[i].value.string);
+      goto parse_error;
+    }
+    n = str_list_to_nums(str, cores, STATIC_ARRAY_SIZE(cores));


size_t n =...

I prefer to define variables at the beginning of scope not on first use. It is used in most of the code. Are there any strong arguments for change?

The collectd maintainers suggest to minimize the scope of of declaration of variable as much as possible. E.g. see #1971 (comment) comment. That's the why I suggest to declare it in place where this variable is used first.

vmytnykx · 2018-02-22T09:48:13Z

src/intel_rdt.c

    rdt_free_cgroups();
    ERROR(RDT_PLUGIN ": Error parsing core groups configuration.");
    return -EINVAL;
  }
+  n = g_rdt->cores.num_cgroups;


size_t n = ..

I prefer to define variables at the beginning of scope not on first use. It is used in most of the code and in line with existing implementation. Are there any strong arguments for change?

Fix minor issues found during review. Update year to 2018, add include in utils_config_cores.h to avoid forward declaration. Change-Id: I7d657bab7c97d7193d7977ef129181cad13d73d5 Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

kwiatrox · 2018-05-17T10:07:40Z

Hi @rpv-tomsk, are there chances for this change to be merged into 5.8 branch?
It solves the issue #2600 and in general is expected by few people. Let me know if you need anything from my side. Thanks

rubenk · 2018-05-17T10:39:21Z

src/intel_pmu.c

+      return;
+    }
+    for (size_t j = 0; j < cgroup->num_cores; j++)
+      snprintf(cores + strlen(cores), cores_size - strlen(cores), " %d",


please check for failure

rubenk · 2018-05-17T10:42:08Z

src/intel_pmu.c

@@ -270,20 +325,17 @@ static int pmu_config(oconfig_item_t *ci) {
  return 0;
 }

-static void pmu_submit_counter(int cpu, char *event, counter_t value,
+static void pmu_submit_counter(char *cgroup, char *event, counter_t value,


cgroup is not modified, can you make it const?

rubenk · 2018-05-17T10:45:19Z

I've left a few comments inline. Once these are resolved I'll merge.

Just for future reference, new functionality land in master, the stable branches should just receive bug fixes.
I'll make an exception this time since you already did rebase on 5.8.

Check for failure from snprintf. Make cgroup and event const in pmu_submit_counter. Change-Id: I5547375da26c3a63b76588b733e844e3199e9bb8 Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

kwiatrox · 2018-05-18T12:32:41Z

@rubenk, thanks for the review and comments. I've committed next patch to resolve them.

rubenk · 2018-05-18T16:44:02Z

Thank you @kwiatrox!

kwiatrox mentioned this pull request Feb 9, 2018

intel_pmu: support for reading aggregate metrics across cores and adding custom metrics #2600

Open

maryamtahhan approved these changes Feb 9, 2018

View reviewed changes

kwiatrox added 4 commits February 19, 2018 08:11

intel_pmu,intel_rdt plugins: apply formatting rules

c0f6e19

Run script contrib/format.sh to format the code. Replace zu with new collectd macro PRIsz. Change-Id: I167b1065461e924d7ab260a35f85f5ab162c4165 Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

intel_pmu: fix epel6 compilation issue

0a600b5

To avoid compilation error on some versions of gcc the saveptr for strtok_r needs to be initilised to NULL. Change-Id: I7b30e51ecae33a6994ba7ea181cac0f33eef023f Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

kwiatrox force-pushed the feat_pmu_cores branch 2 times, most recently from 0a600b5 to 82b0d43 Compare February 19, 2018 11:45

intel_pmu plugin: fix for compatibility issue with collectd 5.8

12d7e13

Add definition for PRIsz macro to make code backward compatible with collectd-5.8 branch. Change-Id: I880340af5ae883a444563422b3e9975b3693683c Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

kwiatrox changed the base branch from master to collectd-5.8 February 20, 2018 08:34

collectd-bot added this to the 5.8 milestone Feb 20, 2018

kwiatrox force-pushed the feat_pmu_cores branch from 82b0d43 to 12d7e13 Compare February 20, 2018 08:35

vmytnykx reviewed Feb 22, 2018

View reviewed changes

intel_pmu plugin: fix for minor issues

4dba7ed

Fix minor issues found during review. Update year to 2018, add include in utils_config_cores.h to avoid forward declaration. Change-Id: I7d657bab7c97d7193d7977ef129181cad13d73d5 Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

vmytnykx approved these changes Feb 26, 2018

View reviewed changes

maryamtahhan approved these changes Feb 27, 2018

View reviewed changes

rubenk reviewed May 17, 2018

View reviewed changes

intel_pmu plugin: fix for PR comments

94d47fe

Check for failure from snprintf. Make cgroup and event const in pmu_submit_counter. Change-Id: I5547375da26c3a63b76588b733e844e3199e9bb8 Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

rubenk merged commit 753955e into collectd:collectd-5.8 May 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

intel_pmu: core groups feature #2681

intel_pmu: core groups feature #2681

kwiatrox commented Feb 9, 2018

maryamtahhan left a comment

kwiatrox commented Feb 10, 2018

kwiatrox commented Feb 12, 2018

maryamtahhan commented Feb 16, 2018

kwiatrox commented Feb 20, 2018

vmytnykx left a comment

vmytnykx Feb 21, 2018

vmytnykx Feb 22, 2018

kwiatrox Feb 22, 2018

vmytnykx Feb 22, 2018

kwiatrox Feb 22, 2018

vmytnykx Feb 22, 2018

vmytnykx Feb 22, 2018

kwiatrox Feb 26, 2018

vmytnykx Feb 22, 2018

kwiatrox Feb 22, 2018

vmytnykx Feb 22, 2018

vmytnykx Feb 22, 2018 •

edited

Loading

vmytnykx Feb 22, 2018

kwiatrox Feb 22, 2018

vmytnykx Feb 22, 2018

kwiatrox commented May 17, 2018

rubenk May 17, 2018

rubenk May 17, 2018

rubenk commented May 17, 2018

kwiatrox commented May 18, 2018

rubenk commented May 18, 2018

intel_pmu: core groups feature #2681

intel_pmu: core groups feature #2681

Conversation

kwiatrox commented Feb 9, 2018

maryamtahhan left a comment

Choose a reason for hiding this comment

kwiatrox commented Feb 10, 2018

kwiatrox commented Feb 12, 2018

maryamtahhan commented Feb 16, 2018

kwiatrox commented Feb 20, 2018

vmytnykx 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

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

vmytnykx Feb 22, 2018 • edited Loading

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

kwiatrox commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenk commented May 17, 2018

kwiatrox commented May 18, 2018

rubenk commented May 18, 2018

vmytnykx Feb 22, 2018 •

edited

Loading