-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add support for collecting/visualizing Meminfo data #69
Conversation
src/data/meminfodata.rs
Outdated
#[strum(serialize = "High Total")] | ||
HighTotal, | ||
#[strum(serialize = "High Free")] | ||
HighFree, | ||
#[strum(serialize = "Low Total")] | ||
LowTotal, | ||
#[strum(serialize = "Low Free")] | ||
LowFree, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are only applicable on 32-bit platforms with CONFIG_HIGHMEM (https://man7.org/linux/man-pages/man5/proc.5.html and https://cateee.net/lkddb/web-lkddb/HIGHMEM.html)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not want to support these fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The year is 2023.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the 32-bit fields.
let reader = BufReader::new(raw_value.data.as_bytes()); | ||
let meminfo = Meminfo::from_reader(reader)?; | ||
let mut meminfo_data = MeminfoData::new(); | ||
meminfo_data.add(MeminfoKeys::MemTotal.to_string(), meminfo.mem_total); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens on all of these if the kernel doesn't support it? Is the value just zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fields that have unwrap_or_default
are the ones that the kernel may/may not support. If a field is absent, the value is zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, can you just only emit them if they're present. That will also avoid creating graphs with bogus (0) values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. If we compare a kernel with that field and a kernel that doesn't, things might get wonky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not like we already don't have to handle differences in metrics when comparing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll make a JS change where all data types can use an API to remove the 0 graphs if it is 0 for all runs, if compared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All legit zeros is not the same thing as all made-up zeros.
src/data/meminfodata.rs
Outdated
#[strum(serialize = "File Huge Pages")] | ||
FileHugePages, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of these should probably be made to follow that in the linux kernel documentation (https://docs.kernel.org/filesystems/proc.html#meminfo), as there are a few things here that are far apart (like KReclaimable) from similar metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'll arrange them in the kernel documentation order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arranged as the kernel documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emitting made-up values intentionally (even zeros) is not great
Collect and visualize meminfo data. aperf was used to collect data during a full kernel build. We can consider if the data in meminfo is good enough that we might skip vmstat.
aperf_report_fullbuild.tar.gz
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.