-
Notifications
You must be signed in to change notification settings - Fork 75
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
Feature: Add oom monitor for cgroupv2. #257
base: main
Are you sure you want to change the base?
Feature: Add oom monitor for cgroupv2. #257
Conversation
7300ca1
to
e83b1d0
Compare
@kzys @dims @caniszczyk @tianon Hi, cgroup v2 version of oom monitor is pushed, please review, thx! |
#229 Implementation of cgroup v1 oom monitor is here. |
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.
Thanks for the contribution!
I left a few comments.
Also clippy on CI suggests a few refactorings.
let key = key.to_string(); | ||
|
||
tokio::spawn(async move { | ||
let inotify = memory_event_fd(&cg_dir).unwrap(); |
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.
You can move this before spawn to handle error properly.
And then move to thread.
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.
Also possibly you could extend the function to return a stream of inotify_event
events to simplify the logic of this loop?
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.
Hmm, sorry, I don't get you. Are you suggesting that I should move certain parts of this loop into a separate function in order to simplify it? If that's the case, how to make the function returns a stream of inotify_event? Using a channel for that purpose?
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.
@mxpv or to modify here as:
fn new_read_inotify_file_function(inotify_file) {
nread = read(inotify_file)
// verify events here
if nread >= size_of::<libc::inotify_event>()
....
}
pub async fn register_memory_event_v2() {
// create inotify file
inotify_file = ...
tokio::spawn() {
loop {
// mv all verifications of events to a new function
if new_read_inotify_file_function(inotify_file) {
parse_kv_file()
.......
}
}
}
}
None => &0, | ||
}; | ||
|
||
let oom_kill = match mem_map.get("oom_kill") { |
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.
nit:
let Some(oom_kill) = mem_map.get("oom_kill") else {
return;
}
same for below.
if nread >= size_of::<libc::inotify_event>() { | ||
match parse_kv_file(&cg_dir, "memory.events").await { | ||
Ok(mem_map) => { | ||
let last_oom_kill = match lastoom_map.get(&key) { |
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.
nit: try lastoom_map.get(&key).copied().unwrap_or(0)
let val = val.parse::<u32>()?; | ||
map.insert(key.to_string(), val); | ||
} | ||
} |
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.
More rusty variation of this:
let map = file_string
.lines()
.filter(|line| line.split_once(" "))
.map(|(k, v) {
let val = val.parse::<u32>()?;
Ok((k.to_string(), val))
})
.collect::<Result<HashMap<_, _>>>()?;
Add oom monitor for cgroupv2.