sysinfo System::new opens too many files #242
Comments
|
Also this only happens on sysinfo 0.10, 0.9 was good |
|
This is something I was afraid that would happen... I added it in this commit. The issue here is that it takes a lot of time opening those specific files, so keeping them around open allowed me to improve the performance on linux. So after doing this, I opened #231 and fixed it in this commit. As you can see, I limit its usage at 90% of the total number of available fds. But from this issue, it appears to be too much. So in here we have three solutions:
What do you think would be better? |
|
Thanks for quick response! 3 sounds most appealing to me, so user can make it 0% (option 1), 50% (option 2), and leave it default (90%). Btw i feel 90% is a little much, if program is opening other files or listen for sockets. |
|
I opened #243. Could you confirm it works for you please? |
|
Hmm, seems not work for me? My cargo.toml: And with default ulimit -n 1024, and my set of ulimit -n 2048 it if i set ulimit to 4000, it gives: This looks 50% limit was not preserved? |
|
Maybe I don't get the system's limit correctly... What linux are you using? Also, can you run this code and show what you get: #[cfg(target_os = "android")]
{
// The constant "RLIMIT_NOFILE" doesn't exist on Android so we have to return a value.
// The default value seems to be 1024 so let's return 50% of it...
println!("new limit1: {}", 1024 - 1024 / 2);
}
#[cfg(not(target_os = "android"))]
unsafe {
let mut limits = libc::rlimit {
rlim_cur: 0,
rlim_max: 0,
};
if libc::getrlimit(libc::RLIMIT_NOFILE, &mut limits) != 0 {
// Most linux system now defaults to 1024.
println!("new limit2: {}", 1024 - 1024 / 2);
return;
}
// We save the value in case the update fails.
let current = limits.rlim_cur;
// The set the soft limit to the hard one.
limits.rlim_cur = limits.rlim_max;
// In this part, we leave minimum 50% of the available file descriptors to the process
// using sysinfo.
println!("new limit3: {}",
if libc::setrlimit(libc::RLIMIT_NOFILE, &limits) == 0 {
limits.rlim_cur - limits.rlim_cur / 2
} else {
current - current / 2
} as _);
))
} |
|
I just realized that my code was incorrect: when opening a file, I was increasing the count instead of decreasing it. Please try again. |
|
I get:
new limit3: 262144
(but with a little modify, i had to remove the final`as _` and the `))` in
following line, otherwise rust complains cannot infer type.)
My system is ubuntu 19.04
…On Wed, Jan 15, 2020 at 1:23 AM Guillaume Gomez ***@***.***> wrote:
Maybe I don't get the system's limit correctly... What linux are you
using? Also, can you run this code and show what you get:
#[cfg(target_os = "android")]
{
// The constant "RLIMIT_NOFILE" doesn't exist on Android so we have to return a value.
// The default value seems to be 1024 so let's return 50% of it...
println!("new limit1: {}", 1024 - 1024 / 2);
}
#[cfg(not(target_os = "android"))]unsafe {
let mut limits = libc::rlimit {
rlim_cur: 0,
rlim_max: 0,
};
if libc::getrlimit(libc::RLIMIT_NOFILE, &mut limits) != 0 {
// Most linux system now defaults to 1024.
println!("new limit2: {}", 1024 - 1024 / 2);
return;
}
// We save the value in case the update fails.
let current = limits.rlim_cur;
// The set the soft limit to the hard one.
limits.rlim_cur = limits.rlim_max;
// In this part, we leave minimum 50% of the available file descriptors to the process
// using sysinfo.
println!("new limit3: {}",
if libc::setrlimit(libc::RLIMIT_NOFILE, &limits) == 0 {
limits.rlim_cur - limits.rlim_cur / 2
} else {
current - current / 2
} as _);
))
}
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#242?email_source=notifications&email_token=ADFFFCFAB2VFECPXEM5SMGLQ53I2TA5CNFSM4KGL2LG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI7UBFQ#issuecomment-574570646>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADFFFCGMEXSHX23N65J3IQTQ53I2TANCNFSM4KGL2LGQ>
.
|
|
This is surprisingly high... But I have the same system and it works as expected on mine... Did you try with my last change on the PR? |
|
Hmm yes, it works:
before system::new Output { status: ExitStatus(ExitStatus(0)), stdout:
"76\n", stderr: "" }
after system::new Output { status: ExitStatus(ExitStatus(0)), stdout:
"4555\n", stderr: "" }
although ulimit is still 1024. so your change looks like increase fd limit
while running?
…On Wed, Jan 15, 2020 at 1:51 AM Guillaume Gomez ***@***.***> wrote:
This is surprisingly high... But I have the same system and it works as
expected on mine... Did you try with my last change on the PR?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#242?email_source=notifications&email_token=ADFFFCEAZ76PDRMYMEN5MFLQ53MD7A5CNFSM4KGL2LG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI7WYXI#issuecomment-574581853>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADFFFCC253MYMX6KGQV2FJTQ53MD7ANCNFSM4KGL2LGQ>
.
|
|
Yes, it's raising the maximum so more FDs are available. In any case, you can still limit it to 0 by using the function |
|
@GuillaumeGomez shows: |
|
Indeed, I made two mistakes:
Both should be fixed now. Can you give it another try please? Really sorry for all the mistakes on my side... |
|
@GuillaumeGomez No worries, I'm very appreciate for your help. Seems still doesn't work for me: (in case it was cargo cache issue i did cargo clean and remove sysinfo from ~/.cargo/git, jump to definition also jumps to your latest version) What is the result running on your machine? use sysinfo::{get_current_pid, Pid, ProcessExt, System, SystemExt};
pub fn show_lsof(prefix: &str) {
let id = std::process::id();
let out = std::process::Command::new("bash")
.arg("-c")
.arg(format!("lsof -p {} | wc -l", id))
.output()
.expect("failed to execute process");
println!("{} {:?}", prefix, out);
}
fn main() {
show_lsof("before system::new");
assert!(sysinfo::set_open_files_limit(0));
let mut sys = System::new();
show_lsof("after system::new");
} |
|
I get: But I link to my local [package]
name = "test-sysinfo"
version = "0.1.0"
authors = ["Guillaume Gomez <guillaume1.gomez@gmail.com>"]
edition = "2018"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[dependencies]
sysinfo = { path = "../sysinfo" }Just to be sure, the last commit hash of |
|
@GuillaumeGomez sorry my fault, seems i didn't clean up thoroughly. after nuke ~/.cargo/git and ~/.cargo/registry it works. clone sysinfo to a local path, checkout to fds-limit branch as you did above also works. Thank you a lot! |
|
Perfect! I'll make a new minor release quickly then. Thanks a lot for your help through this! |
Giving this snippet:
Run it:
it's showing:
The text was updated successfully, but these errors were encountered: