Intel RDT plugin: add support for groups of PIDs. #2891

wandralx · 2018-08-06T08:54:08Z

ChangeLog: intel_rdt: Add support for groups of PIDs monitoring

intel_rdt plugin takes processes’ names as single items or as groups.
Plugin will reconfigure pqos library when given process is terminated or created.

For each process or group of processes following statistics are collected:

LLC occupancy,
IPC,
MBL and MBR.

rjablonx

+1, looks good to me

kwiatrox

Thank you @wandralx, looks good for me.

wandralx · 2018-09-13T09:38:59Z

@rpv-tomsk any thoughts/comments? when can I expect PR to be accepted? Thank you!

wandralx · 2018-09-17T08:48:34Z

@rubenk any thoughts/comments? when can I expect PR to be accepted? Thank you!

rpv-tomsk · 2018-10-19T13:15:41Z

Can I check this plugin on Core2Duo CPU ?

# cat /proc/cpuinfo
processor       : 0
vendor_id       : GenuineIntel
cpu family      : 6
model           : 23
model name      : Intel(R) Core(TM)2 Duo CPU     E8200  @ 2.66GHz
stepping        : 6
cpu MHz         : 2662.464
cache size      : 6144 KB
fdiv_bug        : no
hlt_bug         : no
f00f_bug        : no
coma_bug        : no
fpu             : yes
fpu_exception   : yes
cpuid level     : 10
wp              : yes
flags           : fpu de tsc msr pae cx8 sep cmov pat clflush mmx fxsr sse sse2 ss ht nx constant_tsc aperfmperf pni ssse3 sse4_1 hypervisor
bogomips        : 5324.92
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

wandralx · 2018-10-19T13:49:44Z

@rpv-tomsk thank you for your interest!

Unfortunately no, to be able to check this plugin, you need one of Intel(R) Xeon(R) processors.

For detailed info about required hardware and kernel versions please see README file:
https://github.com/intel/intel-cmt-cat

For info about /proc/cpuinfo flag bits please see:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/x86/intel_rdt_ui.txt

rpv-tomsk

Hi!

Thank you for your big work on Collectd.

I think this plugin and this update is very interesting and useful.

While looking into code, I found ways to do its code more clear and optimized, I hope.

I propose you to rework this utility and plugin:

Use dynamical array and realloc() instead of linked list of pids and rework types:

typedef struct pids_list_s {
  pid_t **pids;
  size_t size;
  size_t allocated;  //only (re)allocate when needed.
} pids_list_t;

typedef struct proc_pids_s {
  proc_comm_t proccess_name;
  pids_list_t *prev;
  pids_list_t *curr;
} proc_pids_t;

So, functions will be simplier, with less arguments:

-int pids_list_diff(pids_list_t *prev, pids_list_t *curr, pids_list_t **added,
-                   size_t *added_num, pids_list_t **removed,
-                   size_t *removed_num) {
+int proc_pids_diff(const proc_pids_t *proc, pids_list_t *added, pids_list_t *removed)

-pids_list_add_pids_list()
+pids_list_clone()

-fetch_pids_for_procs()
+update_proc_pids(proc_pids_t **proc_pids, size_t proc_pids_num); //Use global array and call this only once per read cycle

... and so on...

In plugin code:

*) Move pqos_mon_data into rdt_name_group_s.
*) Update structure a bit:

struct rdt_name_group_s {
  char *desc;                             -- This will to to `plugin_instance`
  //char **names;                      -- Remove in favour of `process_name` of `proc_pids_t`.
  proc_pids_t *proc_pids_array;  -- Remove `_array` suffix here
  size_t proc_pids_num;             -- Was `num_names` before renaming
  size_t monitored_pids_count;  -- Unsure if these two needs to be touched.
  enum pqos_mon_event events;
};

*) Create global dynamical array of proc_pids_t. That will be passed to update_proc_pids(), so /proc will be parsed only once. When doing oconfig_to_ngroups(), proc_pids_t should be allocated, and put into two arrays - one global, and one in ngroup.
*) In update_proc_pids() you can just swap prev/curr->pids pointers and prev/curr->size, and set curr->size = 0 before new pids added into curr.
*) When adding new pids into pids_list_t do realloc() only when needed, by checking allocated field.
*) In read_pids_data() iterate by ngroups as before.
*) In rdt_refresh_ngroup() you can use arrays of pid_t directly, pids_list_to_array() will not be needed.


And so on. There is wide field for optimization and refactoring, so I can't write all details here w/o writing the code. 

What can you say about such rework?

Unfortunately, I have no equipment to test this plugins, all available Xeon processors are too old.

rpv-tomsk · 2018-10-20T07:56:44Z

src/intel_rdt.c

+  DEBUG(RDT_PLUGIN ": Available events to monitor: %#x", events);
+
+  rdt->num_ngroups = n;
+  for (int i = 0; i < n; i++) {


This implementation does not take into account that one option can be used in config several times.
Validation can be put into rdt_config(), after all options are processed in 'for' cycle. Other changes may also be required.

This helper function is called by rdt_config only

This implementation does not take into account that one option can be used in config several times.

This implementation does not take into account that one option can be used in config several times.

Yes, currently we support only single Processes or Cores option. We plan to extend that in the future.

rpv-tomsk · 2018-10-20T08:20:28Z

src/utils_proc_pids.c

+  int init_result = initialize_proc_pids(
+      procs_names_array, procs_names_array_size, proc_pids_array);
+  if (0 != init_result)
+    return -1;


closedir(proc_dir) is missing here.

rpv-tomsk · 2018-10-20T08:25:48Z

src/utils_proc_pids.c

+      continue;
+    }
+
+    /* Try to find comm in input procs array (proc_pids_array has same names) */


proc_pids_array has same names - that is unclear.

If names are the same, why do we need to copy they in initialize_proc_pids() in each cycle?
You can just do direct compare with procs_names_array[i] and you even need not to have proccess_name in proc_pids_s then. And, as there is only one field in that structure, then that structure is not needed too.

But I have another proposal to rework this utility and plugin.

rpv-tomsk · 2018-10-20T08:58:18Z

src/utils_proc_pids.c

+    int read_result =
+        read_proc_name(procfs_path, entry, comm, sizeof(proc_comm_t));
+    if (read_result <= 0) {
+      ERROR(UTIL_NAME ": Comm file skipped. Read result: %d", read_result);


Typical system can have processes starting and finishing in unpredictable time.
Race conditions can occur. Logging ERROR for each such case of terminated process is excessive.
I think, no message is required here at all, like in processes plugin.

rpv-tomsk · 2018-10-20T09:26:32Z

src/intel_rdt.c

+groups_refresh:
+  for (size_t i = 0; i < g_rdt->num_ngroups; i++) {
+    int refresh_result =
+        rdt_refresh_ngroup(&(g_rdt->ngroups[i]), g_rdt->pngroups[i]);


This will call fetch_pids_for_procs() for each configured group, and will scan /proc multiple times.
I think that is suboptimal and I have proposal how to optimize this.

proc_pids_update moved before the for loop. Now list of pids is fetched only once.

sunkuranganath · 2019-02-26T16:35:15Z

Thanks @rpv-tomsk for the review. Working on these internally. Will have few updates very soon.

rpv-tomsk · 2019-02-26T16:41:00Z

Great news!

adds intel_rdt_tests file with unit-tests of newly introduced functions. Change-Id: I0e26322caa19f57b7d660807646bd437586cd80d Signed-off-by: Starzyk, MateuszX <mateuszx.starzyk@intel.com>

Add temporary functions to enable further development of rdt plugin. These stubbed functions will be removed once libpqos extends it's API. Change-Id: I39dd428e39fd6055774dfc39b4c0690aedee4ebe Signed-off-by: Starzyk, MateuszX <mateuszx.starzyk@intel.com>

"Processes" option added. Change-Id: I7f62a51fc06e0c7503caa7860807f1c8e09b2fa1 Signed-off-by: Andralojc, WojciechX <wojciechx.andralojc@intel.com>

Change-Id: I569d72d0f12dca314e865f0b9236dc09a053c5e5 Signed-off-by: Andralojc, WojciechX <wojciechx.andralojc@intel.com>

Change-Id: I677c8215f89c04cedcb4c9e7d630f187a76a6cc7 Signed-off-by: Andralojc, WojciechX <wojciechx.andralojc@intel.com>

Change-Id: Ib5a7d318bde69ee8f496b39ca74b3ee4e58d8f5b Signed-off-by: Starzyk, MateuszX <mateuszx.starzyk@intel.com>

Refactor certain functions to be testable. Change-Id: I5adb6ed1b076f0cebc70641f04dd101d4e89b9cc Signed-off-by: Starzyk, MateuszX <mateuszx.starzyk@intel.com>

Change-Id: I3737a39c40d89a8782ef9c3d8195351f0fdeb278

… static to dynamic Change-Id: I3936fe78cfcbb4ce053d38ff1a3236c21eac9fd7 Signed-off-by: Michal Aleksinski <michalx.aleksinski@intel.com>

Extract pids config to seperate file. Extract pids config tests to seperate file. Change-Id: I2978db56b15a126eced12f40baac631517303e04 Signed-off-by: Mateusz Starzyk <mateuszx.starzyk@intel.com>

Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com> Change-Id: I325ea0728dd1eb65776f239b0d7977cc02f9c8db

Change-Id: I161c8df1e6a4c4c4e0dd4e20601323ef7a1c18f5

Change-Id: Ibcf8c2fd6d1b490cbeeef7480c49da84389104f4

Change-Id: Ibb0dc6fe59eb77a217ab3b45ec0c1437bc9eb281

Change-Id: Ia7c9bf5dca2b871b05c9ed3876059e6d34bd943e

Change-Id: Ia932f377c5194240aab910d052e422b99ea542a5

rpv-tomsk · 2019-03-06T13:30:05Z

src/intel_rdt.c

+  DEBUG(RDT_PLUGIN ": Available events to monitor: %#x", events);
+
+  rdt->num_ngroups = n;
+  for (int i = 0; i < n; i++) {


This implementation does not take into account that one option can be used in config several times.

…ds list Change-Id: Ic99acd51be69d2678f25bdde9ca37011ad24fe99

kwiatrox · 2019-03-07T14:48:19Z

Hi @rpv-tomsk, Currently we support only single use of Processes or Cores option. This is the case also for some other plugins. It's a good idea, but maybe we should improve it in the future, not in this PR.
We just would like to include it in 5.9 release.

sunkuranganath · 2019-03-11T18:45:27Z

I would go with @kwiatrox suggestion. Could we have this accepted for 5.9 release and have the updates done just after as a new PR?

rpv-tomsk · 2019-03-20T04:11:49Z

Hi @rpv-tomsk, Currently we support only single use of Processes or Cores option.
This is the case also for some other plugins. It's a good idea, but
maybe we should improve it in the future, not in this PR.
We just would like to include it in 5.9 release.

I just want to include only qualitative code. I think there is no much code changes needed to allow multiple use of Processes option, or to deny such explicitly. This reduces configuration errors.
From my POV "maybe" and "in the future" are not allowed for such small changes.

sunkuranganath · 2019-03-20T14:19:56Z

Thanks @rpv-tomsk. We see 5.9 release hasnt happened yet and so we are working on the updates as indicated.

Add a check to avoid placing multiple Cores or Processes options in intel_rdt configuration in order to avoid misuse and errors. Change-Id: I82504a650bf9c568862da9f129a84a3079990496 Signed-off-by: Kamil Wiatrowski <kamilx.wiatrowski@intel.com>

rpv-tomsk · 2019-04-03T04:00:09Z

Hi, Team.

I will not merge none of PR untill it will be possible for me to merge my PR too.

Thanks for your attention.

mrunge · 2019-04-04T05:58:44Z

Thank you all for your contribution.

rjablonx previously approved these changes Aug 8, 2018

View reviewed changes

kwiatrox previously approved these changes Aug 9, 2018

View reviewed changes

rpv-tomsk suggested changes Oct 20, 2018

View reviewed changes

octo changed the title ~~Added support for pqos library 2.0 feature: monitoring groups of PIDs.~~ Intel RDT plugin: add support for groups of PIDs. Oct 25, 2018

octo added the Feature label Oct 25, 2018

aleksinx dismissed stale reviews from kwiatrox and rjablonx via 1200a6d March 6, 2019 05:51

aleksinx force-pushed the feat_intel_rdt branch from c68ca40 to 1200a6d Compare March 6, 2019 05:51

mstarzyx and others added 16 commits March 6, 2019 06:06

intel_rdt: adds pids extraction for given processes list

62be067

adds intel_rdt_tests file with unit-tests of newly introduced functions. Change-Id: I0e26322caa19f57b7d660807646bd437586cd80d Signed-off-by: Starzyk, MateuszX <mateuszx.starzyk@intel.com>

intel_rdt: Update config parser for PID monitoring

fcb1c01

"Processes" option added. Change-Id: I7f62a51fc06e0c7503caa7860807f1c8e09b2fa1 Signed-off-by: Andralojc, WojciechX <wojciechx.andralojc@intel.com>

intel_rdt: PID groups change detection

8d772b8

Change-Id: I569d72d0f12dca314e865f0b9236dc09a053c5e5 Signed-off-by: Andralojc, WojciechX <wojciechx.andralojc@intel.com>

intel_rdt: Backwards compatibility, libpqos v1

1225966

Change-Id: I677c8215f89c04cedcb4c9e7d630f187a76a6cc7 Signed-off-by: Andralojc, WojciechX <wojciechx.andralojc@intel.com>

intel_rdt: collect data for monitored processes

3ec575b

Change-Id: Ib5a7d318bde69ee8f496b39ca74b3ee4e58d8f5b Signed-off-by: Starzyk, MateuszX <mateuszx.starzyk@intel.com>

intel_rdt: update unit-tests for process monitoring

7f6e8b7

Refactor certain functions to be testable. Change-Id: I5adb6ed1b076f0cebc70641f04dd101d4e89b9cc Signed-off-by: Starzyk, MateuszX <mateuszx.starzyk@intel.com>

Corrected issues reported by klockwork

8ee9c6d

Change-Id: I3737a39c40d89a8782ef9c3d8195351f0fdeb278

intel-rdt: Changed memory allocation of new and lost pids tables from…

f9a460d

… static to dynamic Change-Id: I3936fe78cfcbb4ce053d38ff1a3236c21eac9fd7 Signed-off-by: Michal Aleksinski <michalx.aleksinski@intel.com>

intel_rdt: refactor pids monitoring code.

df49cf6

Extract pids config to seperate file. Extract pids config tests to seperate file. Change-Id: I2978db56b15a126eced12f40baac631517303e04 Signed-off-by: Mateusz Starzyk <mateuszx.starzyk@intel.com>

Fix for CI error in test code on epel6 platform.

b1486b3

Signed-off-by: Wojciech Andralojc <wojciechx.andralojc@intel.com> Change-Id: I325ea0728dd1eb65776f239b0d7977cc02f9c8db

intel_rdt: close file descriptor on error

e0d53a0

Change-Id: I161c8df1e6a4c4c4e0dd4e20601323ef7a1c18f5

intel_rdt: updated pqos mocks for pqos version 3.0

e23e9fa

Change-Id: Ibcf8c2fd6d1b490cbeeef7480c49da84389104f4

intel_rdt: refactored proc utils to increase efficiency

959add7

Change-Id: Ibb0dc6fe59eb77a217ab3b45ec0c1437bc9eb281

utils_proc_pids: moved to utils directory

bcb9e82

Change-Id: Ia7c9bf5dca2b871b05c9ed3876059e6d34bd943e

intel_rdt: Fixed compile error in debug build

d271957

Change-Id: Ia932f377c5194240aab910d052e422b99ea542a5

rpv-tomsk suggested changes Mar 6, 2019

View reviewed changes

intel_rdt: Added check if name group is defined before updating of pi…

4d08ad7

…ds list Change-Id: Ic99acd51be69d2678f25bdde9ca37011ad24fe99

aleksinx force-pushed the feat_intel_rdt branch from 1200a6d to 4d08ad7 Compare March 6, 2019 16:23

kwiatrox previously approved these changes Mar 11, 2019

View reviewed changes

sunkuranganath mentioned this pull request Mar 19, 2019

RFC: Commit access for outside contributors #2958

Open

kwiatrox dismissed their stale review via 693416b April 2, 2019 12:05

rjablonx approved these changes Apr 2, 2019

View reviewed changes

sunkuranganath approved these changes Apr 2, 2019

View reviewed changes

rpv-tomsk approved these changes Apr 3, 2019

View reviewed changes

mrunge merged commit f3d8a8a into collectd:master Apr 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intel RDT plugin: add support for groups of PIDs. #2891

Intel RDT plugin: add support for groups of PIDs. #2891

wandralx commented Aug 6, 2018 •

edited

rjablonx left a comment

kwiatrox left a comment

wandralx commented Sep 13, 2018

wandralx commented Sep 17, 2018

rpv-tomsk commented Oct 19, 2018

wandralx commented Oct 19, 2018

rpv-tomsk left a comment

rpv-tomsk Oct 20, 2018

aleksinx Oct 25, 2018

rpv-tomsk Mar 6, 2019

aleksinx Mar 7, 2019

rpv-tomsk Oct 20, 2018

rpv-tomsk Oct 20, 2018

rpv-tomsk Oct 20, 2018

rpv-tomsk Oct 20, 2018

aleksinx Mar 6, 2019

sunkuranganath commented Feb 26, 2019

rpv-tomsk commented Feb 26, 2019

rpv-tomsk Mar 6, 2019

kwiatrox commented Mar 7, 2019

sunkuranganath commented Mar 11, 2019

rpv-tomsk commented Mar 20, 2019

sunkuranganath commented Mar 20, 2019

rpv-tomsk commented Apr 3, 2019

mrunge commented Apr 4, 2019

Intel RDT plugin: add support for groups of PIDs. #2891

Intel RDT plugin: add support for groups of PIDs. #2891

Conversation

wandralx commented Aug 6, 2018 • edited

rjablonx left a comment

Choose a reason for hiding this comment

kwiatrox left a comment

Choose a reason for hiding this comment

wandralx commented Sep 13, 2018

wandralx commented Sep 17, 2018

rpv-tomsk commented Oct 19, 2018

wandralx commented Oct 19, 2018

rpv-tomsk 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

sunkuranganath commented Feb 26, 2019

rpv-tomsk commented Feb 26, 2019

Choose a reason for hiding this comment

kwiatrox commented Mar 7, 2019

sunkuranganath commented Mar 11, 2019

rpv-tomsk commented Mar 20, 2019

sunkuranganath commented Mar 20, 2019

rpv-tomsk commented Apr 3, 2019

mrunge commented Apr 4, 2019

wandralx commented Aug 6, 2018 •

edited