Skip to content
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.

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

hugepages plugin: a stats plugin for hugepages #1799

Merged
merged 6 commits into from Aug 22, 2016
Merged

hugepages plugin: a stats plugin for hugepages #1799

merged 6 commits into from Aug 22, 2016

Conversation

maryamtahhan
Copy link
Contributor

Plugin that allows stats for hugepages to be collected.

Change-Id: Ide5ff31c54206cf5ee3d6155899bd53e8edae7bd
Signed-off-by: Jaroslav Safka jaroslavx.safka@intel.com
Signed-off-by: Kim Jones kim-marie.jones@intel.com
Signed-off-by: Maryam Tahhan maryam.tahhan@intel.com

hugepages plugin: a stats plugin for hugepages
178402a 
Plugin that allows stats for hugepages to be collected.

Change-Id: Ide5ff31c54206cf5ee3d6155899bd53e8edae7bd
Signed-off-by: Jaroslav Safka <jaroslavx.safka@intel.com>
Signed-off-by: Kim Jones <kim-marie.jones@intel.com>
Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
hugpages: fix compilation error
fa49565 
Fix error: format '%lu' expects type 'long unsigned int', but
argument 6 has type 'derive_t'. For line 93.

Change-Id: I4f164d238cd4fa6f8a69d78b2906042aa3c5da72
Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
@octo
Copy link
Member

octo commented Jul 20, 2016

Hi @maryamtahhan, thank you very much for your patch! I'll leave some review comments inline.

Best regards,
—octo

octo
octo reviewed Jul 20, 2016
View reviewed changes
configure.ac
@@ -5971,6 +5974,7 @@ AC_PLUGIN([fscache], [$plugin_fscache], [fscache statistics]
AC_PLUGIN([gmond], [$with_libganglia], [Ganglia plugin])
AC_PLUGIN([grpc], [$with_grpc], [gRPC plugin])
AC_PLUGIN([hddtemp], [yes], [Query hddtempd])
AC_PLUGIN([hugepages], [yes], [Hugepages statistics])
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a Linux-only plugin, it should only be enabled when being compiled on Linux. Search configure.ac for plugin_cgroups which is a good example of how to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

@maryamtahhan
Copy link
Contributor Author

@octo Thank you for the review. I will address your comments and post an update

@octo octo added the Waiting label Aug 2, 2016
rubenk
rubenk reviewed Aug 10, 2016
View reviewed changes
README
@@ -128,6 +128,12 @@ Features
- hddtemp
Hard disk temperatures using hddtempd.

- hugepages
Report the total number of hugpages, the number of free/reserved
Copy link
Contributor

Choose a reason for hiding this comment

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

hugepages

Copy link
Member

Choose a reason for hiding this comment

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

#hugops \o/

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed :)

@kimmariejones
hugepages: rework
f91506b 
Changed hugepage metric type and type instances.
Made a single entry in types.db for "free" and
    "used" hugepages.
Calculate the used hugepages.
Removed read_syshugepage_dir; replaced with
    walk_directory.
Cleared up descriptions in .pod.
Made the .conf plugin option names less cryptic.
Linux-only plugin; changed configure.ac accordingly.
Stripped function declarations and header includes
    that are not used.
Tidied up globals and made some globals local.
Removed some hardcoded values and fixed spacing.
Other general tidy-ups.

Change-Id: Id0243397d3fd4ac1be90266b266341318c05c903
Signed-off-by: Kim Jones <kim-marie.jones@intel.com>
rubenk
rubenk reviewed Aug 16, 2016
View reviewed changes
src/collectd.conf.pod
To collect B<hugepages> information, collectd reads directories
"/sys/devices/system/node/*/hugepages" and
"/sys/kernel/mm/hugepages".
Reading of these directories can be disabled by following
Copy link
Contributor

Choose a reason for hiding this comment

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

"the" following options?

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

@kimmariejones
hugepages: rework 2
26c9dd0 
Fix typo in .pod file.
Change char * strings to static const char [].
Change INFO() to DEBUG() in 1 place.
Removed unnecessary initialisations & casts.
Fixed ignored error returns from some funcs.
Actually use result returned from pathconf().

Change-Id: I0b59af4eb8afd29141abfa9233ca1d4a8d19d0d1
Signed-off-by: Kim Jones <kim-marie.jones@intel.com>
@kimmariejones
hugepages: typo fix in 2nd pathconf() check
c4413ca 
Change-Id: Ie4ceb0ec4b55e60fcffb1d59d33a5711366eb2f7
Signed-off-by: Kim Jones <kim-marie.jones@intel.com>
rubenk
rubenk reviewed Aug 16, 2016
View reviewed changes
src/hugepages.c
read_syshugepages(path, result->d_name);
}

closedir(dir);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if readdir returns an error? In that case closedir is called with a NULL argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that I need to check errno here and return -1 if it is set; will add that.

However, dir is opened at line 216. closedir(dir) will still be required even if readdir fails with an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I guess in practice calling closedir with an invalid DIR * is not an issue. POSIX is silent about this, but their example uses the same code as you do: http://pubs.opengroup.org/onlinepubs/009695399/functions/closedir.html

@kimmariejones
hugepages: check for error if readdir returns NULL
51b24f4 
Change-Id: Ib361b648407c7052218c1f6b9b624cb7aa799634
Signed-off-by: Kim Jones <kim-marie.jones@intel.com>
@rubenk
Copy link
Contributor

rubenk commented Aug 17, 2016

Thanks @kimmariejones! I have nothing more to complain about, so I think we're good to go.
@octo Since we've already branched 5.6 at this point, do you want to keep it into master for one release cycle, or can this still go into 5.6?

@rubenk rubenk merged commit 51b24f4 into collectd:master Aug 22, 2016
@rubenk
Copy link
Contributor

rubenk commented Aug 22, 2016

Thanks for your work @maryamtahhan and @kimmariejones. I've just pulled this into master and it will be part of the next collectd release due around christmas.

@rubenk rubenk removed the Waiting label Aug 22, 2016
@maryamtahhan
Copy link
Contributor Author

Thanks @rubenk

@maryamtahhan maryamtahhan deleted the hugepages branch August 24, 2016 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature New plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@maryamtahhan @octo @rubenk @kimmariejones