8000 add option to collect the count of active memory maps for linux processes by dothebart · Pull Request #2454 · collectd/collectd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

add option to collect the count of active memory maps for linux processes #2454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Sep 29, 2017
Merged

add option to collect the count of active memory maps for linux processes #2454

merged 4 commits into from
Sep 29, 2017

Conversation

dothebart
Copy link
Contributor

In linux the maximum count of memory mapped files is configured via /proc/sys/vm/max_map_count which is 65530 by default.

With bigger machines that are capable of running many application threads this may quickly become a sparse resource - therefore I'd like to be able to track it.

rpv-tomsk
rpv-tomsk reviewed Sep 27, 2017
View reviewed changes
src/processes.c Outdated
@@ -738,7 +752,7 @@ static void ps_submit_proc_list(procstat_t *ps) {
plugin_dispatch_values(&vl);

if ((ps->io_rchar != -1) && (ps->io_wchar != -1)) {
sstrncpy(vl.type, "io_octets", sizeof(vl.type));
sstrncpy(vl.type, "ps_disk_octets", sizeof(vl.type));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. Looks as an unrelated change.

rpv-tomsk
rpv-tomsk reviewed Sep 27, 2017
View reviewed changes
src/processes.c Outdated
@@ -761,13 +775,20 @@ static void ps_submit_proc_list(procstat_t *ps) {
plugin_dispatch_values(&vl);
}

if (ps->num_fd > 0) {
if (ps->num_fd > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change also unneeded.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to note that clang-format has the final say in this. I'd say it would undo this change.

rpv-tomsk
rpv-tomsk reviewed Sep 27, 2017
View reviewed changes
src/collectd.conf.in Outdated
@@ -1170,6 +1170,7 @@
#<Plugin processes>
# CollectFileDescriptor true
# CollectContextSwitch true
# CollectMemoryMaps true
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, fix indent.

@rpv-tomsk
Copy link
Contributor

Hi! Thanks for your work.

I want to notice, you missed adding documentation for a new option.

@octo octo added the Feature label Sep 27, 2017
octo
octo reviewed Sep 27, 2017
View reviewed changes
src/types.db Outdated
@@ -129,6 +129,7 @@ latency value:GAUGE:0:U
links value:GAUGE:0:U
load shortterm:GAUGE:0:5000, midterm:GAUGE:0:5000, longterm:GAUGE:0:5000
memory_bandwidth value:DERIVE:0:U
memory_maps value:GAUGE:0:U
Copy link
Member

Choose a reason for hiding this comment

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

Would the existing file_handles type also work? E.g. as "memory/file_handles-mapped"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I've added this to make it appear on top level in a descriptive name. How would I reference the file_handles type to achieve this?

@octo
Copy link
Member
octo commented Sep 27, 2017

Thanks @dothebart for submitting this!
Thanks @rpv-tomsk for your review! Is much appreciated!

octo
octo reviewed Sep 28, 2017
View reviewed changes
src/processes.c Outdated
@@ -768,6 +782,13 @@ static void ps_submit_proc_list(procstat_t *ps) {
plugin_dispatch_values(&vl);
}

if (ps->num_maps > 0) {
sstrncpy(vl.type, "memory_maps", sizeof(vl.type));
Copy link
Member

Choose a reason for hiding this comment

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

How would I reference the file_handles type to achieve this?

To use the existing file_handles type:

sstrncpy(vl.type, "file_handles", sizeof(vl.type));
sstrncpy(vl.type_instance, "mapped", sizeof(vl.type_instance));

@dothebart
Copy link
Contributor Author 
Check your system clock
+ make -sk check
make: *** No rule to make target 'check'.
Build step 'Execute shell' marked build as failure```
Doesn't seem to be the fault of the patch?

@dothebart dothebart mentioned this pull request Sep 29, 2017
Closed
6 tasks
octo
octo approved these changes Sep 29, 2017
View reviewed changes
Copy link
Member
@octo octo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @dothebart!

@octo octo merged commit dc71a8c into collectd:master Sep 29, 2017
@dothebart
Copy link
Contributor Author

any hints on a timeframe when this will occur in a release? weeks? months?

@octo
Copy link
Member
octo commented Sep 29, 2017

Weeks. We plan to release 5.8 this year, likely first half of December.

@octo octo added this to the 5.8 milestone Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpv-tomsk rpv-tomsk rpv-tomsk left review comments

@octo octo octo approved these changes

Assignees
No one assigned
Labels
Feature
Projects
None yet
Milestone
5.8
Development

Successfully merging this pull request may close these issues.
3 participants
@dothebart @rpv-tomsk @octo
0