Skip to content

Comments

procfs: add parsing of sys/vm#176

Merged
pgier merged 1 commit intoprometheus:masterfrom
TheMeier:sys_vm
Jun 27, 2019
Merged

procfs: add parsing of sys/vm#176
pgier merged 1 commit intoprometheus:masterfrom
TheMeier:sys_vm

Conversation

@TheMeier
Copy link
Contributor

Read data from /proc/sys/vm entries
Closes: #157

Theres still work to be done for lowmem_reserve_ratio, actually i don't understand the format yet fully especially when dealing with NUMA zones.

@TheMeier TheMeier force-pushed the sys_vm branch 2 times, most recently from 79f28ad to 5832b1b Compare June 10, 2019 11:49
Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple minor nits.
@discordianfish @mdlayher What do you think regarding the use of reflect here?

@TheMeier TheMeier force-pushed the sys_vm branch 3 times, most recently from 22cbd4c to b6c426b Compare June 11, 2019 14:01
@TheMeier TheMeier force-pushed the sys_vm branch 4 times, most recently from 53daa2c to 19f28d7 Compare June 11, 2019 15:22
Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

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

@TheMeier PR #178 has been merged, so you can take a look at that one for avoiding the use of reflect.

@TheMeier TheMeier changed the title procfs: add parsing of sys/vm WIP: procfs: add parsing of sys/vm Jun 23, 2019
@TheMeier TheMeier force-pushed the sys_vm branch 5 times, most recently from bb4bbc5 to fdc82b8 Compare June 23, 2019 08:25
@TheMeier
Copy link
Contributor Author

TheMeier commented Jun 23, 2019

@pgier i changed the MR using the same pattern. I used your util ValueParser.Pint64 although I don't use pointers here, I wonder if that should be changed also.

Also simply unpacking/re-packing the fixtures.ttar with the provided makefile leads to re-ordering.

@TheMeier TheMeier changed the title WIP: procfs: add parsing of sys/vm procfs: add parsing of sys/vm Jun 23, 2019
@pgier
Copy link
Collaborator

pgier commented Jun 24, 2019

The fixtures file ordering should be fixed by #185
Edit: if you temporarily change the base branch of the PR and then change it back to master, it should update the diff to remove the bad fixtures stuff.

Read data from /proc/sys/vm entries
Closes: prometheus#157

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

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

LGTM. Although after thinking about possible nil pointers similar to #187, I'm wondering if we should be using pointer types for the int64 values. @TheMeier what are the chances of some of the sys/vm files containing non-integer values and causing nil pointer dereference? @mdlayher wdyt?

@TheMeier
Copy link
Contributor Author

@pgier hm fair question the documentation at https://www.kernel.org/doc/Documentation/sysctl/vm.txt doesn't really say anything about data format.
Looking around a bit in the kernel source i found a bunch of the settings defined as CTL_INT https://github.com/torvalds/linux/blob/f1c2f8857c5aa6c92aa903bc06437503422e5925/kernel/sysctl_binary.c#L147-L174 which is a define to bin_intvec which returns ssize_t so those should be fine.
Some others i randomly checked are defined as int ot u64.
My intuition is it should be quite safe, but to really answer the question someone (me?) should check the definition of each setting in the kernel sources.

@pgier pgier merged commit 6547b3e into prometheus:master Jun 27, 2019
@TheMeier
Copy link
Contributor Author

LGTM. Although after thinking about possible nil pointers similar to #187, I'm wondering if we should be using pointer types for the int64 values. @TheMeier what are the chances of some of the sys/vm files containing non-integer values and causing nil pointer dereference? @mdlayher wdyt?

Actually I just realized I should have used pointers here. In case the valueparser comes back with nil the current implementation will lead to:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference

Will open an issue and patch...

bobrik pushed a commit to bobrik/procfs that referenced this pull request Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for /proc/sys/vm

3 participants