Increase NOFILE limit for tox-bootstrapd#1234
Conversation
|
|
bea0e2f to
1bdc736
Compare
Codecov Report
@@ Coverage Diff @@
## master #1234 +/- ##
========================================
+ Coverage 83% 83.1% +<.1%
========================================
Files 82 82
Lines 14685 14684 -1
========================================
+ Hits 12192 12203 +11
+ Misses 2493 2481 -12
Continue to review full report at Codecov.
|
1bdc736 to
48e699f
Compare
iphydf
left a comment
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @nurupo)
.travis/cmake-freebsd-stage1, line 0 at r1 (raw file):
Any idea why reviewable thinks this is a binary file? Are there any weird control characters in it accidentally?
other/bootstrap_daemon/tox-bootstrapd.sh, line 41 at r1 (raw file):
cat << EOF Warning: Your system has very few filedescriptors available in total.
"file descriptors"
other/bootstrap_daemon/tox-bootstrapd.sh, line 44 at r1 (raw file):
Maybe you should try raising that by adding 'fs.file-max=100000' to your /etc/sysctl.conf file. Feel free to pick any number that you deem appropriate.
Typically, users may not have an idea what value is appropriate. We should recommend 32768, which is an arbitrary but somewhat widely spread number.
other/bootstrap_daemon/src/tox-bootstrapd.c, line 452 at r1 (raw file):
"Consider raising the limit to at least %ju or the recommended %ju. " "Continuing using the current limit (%ju).\n", (uintmax_t)limit.rlim_cur, (uintmax_t)rlim_min, (uintmax_t)rlim_suggested, (uintmax_t)limit.rlim_cur);
uint32_t or even "unsigned" should be enough, so we can use %u instead of %ju (I'm not sure how portable that is - mingw32 seems to have some issues with it according to robinli).
nurupo
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @nurupo)
other/bootstrap_daemon/tox-bootstrapd.sh, line 44 at r1 (raw file):
Previously, iphydf wrote…
Typically, users may not have an idea what value is appropriate. We should recommend 32768, which is an arbitrary but somewhat widely spread number.
That's a bit different. We recommend 32768 limit for the tox-bootstrapd process, however what we are talking in here about, the fs.file-max of /etc/sysctl.conf, is the limit of the entire Linux system, e.g. how many file descriptors can the entire system have open at the same time, all of the processes combined (summed). That's the same value that /proc/sys/fs/file-max reports, which we check a few lines above to make sure we don't exhaust the system of all file descriptors. Well, the system can still run out of fds if there are a lot of other programs/daemons running, but at least we make an attempt to pick a reasonable number.
other/bootstrap_daemon/src/tox-bootstrapd.c, line 452 at r1 (raw file):
Previously, iphydf wrote…
uint32_t or even "unsigned" should be enough, so we can use %u instead of %ju (I'm not sure how portable that is - mingw32 seems to have some issues with it according to robinli).
rlim_t is defined as a some unspecified unsigned integer type. On some systems it's unsigned long, on other it's unsigned long long. I initially used %llu but got a format warning saying that I use format for unsigned long long for a value of unsigned long type on either Linux or FreeBSD build on Travis-CI. If you make it %lu, you'd get a format warning on some systems sayingthat you are using unsigned long format for a value of the type unsigned long long. It appears like casting it to the biggest unsigned integer type and printing it out as the biggest u signed integer type -- %ju format string, I think it's defined in C99 -- is the best option. If %ju is not portable enough (MSVC has spotty C99 support), we should come up with something else. I don't see your suggestion of %u working though, it would cause the same format warnings.
iphydf
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @nurupo and @iphydf)
other/bootstrap_daemon/tox-bootstrapd.sh, line 44 at r1 (raw file):
Previously, nurupo wrote…
That's a bit different. We recommend 32768 limit for the tox-bootstrapd process, however what we are talking in here about, the
fs.file-maxof/etc/sysctl.conf, is the limit of the entire Linux system, e.g. how many file descriptors can the entire system have open at the same time, all of the processes combined (summed). That's the same value that/proc/sys/fs/file-maxreports, which we check a few lines above to make sure we don't exhaust the system of all file descriptors. Well, the system can still run out of fds if there are a lot of other programs/daemons running, but at least we make an attempt to pick a reasonable number.
Ok, sounds reasonable.
other/bootstrap_daemon/src/tox-bootstrapd.c, line 452 at r1 (raw file):
Previously, nurupo wrote…
rlim_tis defined as a some unspecified unsigned integer type. On some systems it'sunsigned long, on other it'sunsigned long long. I initially used%llubut got a format warning saying that I use format forunsigned long longfor a value ofunsigned longtype on either Linux or FreeBSD build on Travis-CI. If you make it%lu, you'd get a format warning on some systems sayingthat you are usingunsigned longformat for a value of the typeunsigned long long. It appears like casting it to the biggest unsigned integer type and printing it out as the biggest u signed integer type --%juformat string, I think it's defined in C99 -- is the best option. If%juis not portable enough (MSVC has spotty C99 support), we should come up with something else. I don't see your suggestion of%uworking though, it would cause the same format warnings.
I'll be more specific. I thought I was, by putting this comment on the line containing "uintmax_t". More specifically I meant: replace uintmax_t with either uint32_t or unsigned, so that %u can be used.
nurupo
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @nurupo)
other/bootstrap_daemon/src/tox-bootstrapd.c, line 452 at r1 (raw file):
Previously, iphydf wrote…
I'll be more specific. I thought I was, by putting this comment on the line containing "uintmax_t". More specifically I meant: replace uintmax_t with either uint32_t or unsigned, so that %u can be used.
You realize that you are suggesting potentially downcasting, for example, 64-bit unsigned integer to 32-bit unsigned integer? For example, rlim_t can be defined as a 64-bit unsigned long longon a system, but we would downcast it to unsigned which is 32-bit on that particular system. The issue with downcasting is that you might truncate/overflow the value in that log message. While I wouldn't expect rlim_t to be a such large number that it wouldn't fit in either uint32_t or unsigned, for some reason its exact type/width are not defined in Linux, FreeBSD and OpenBSD manpages, and some systems do use 64-bit unsigned integers for it, in fact one (or more) of our systems on Travis-CI does, so the fd number could technically be set to a 64-bit unsigned integer (or even 128 bit if some weird ABI with 128 bit integers comes out, as the manpages don't specify exact type/width of rlimit_t), so downcasting, while it would generally work, is not safe, it might result in us printing the wrong number.
|
Looks like MSVC supports The |
|
Oh, and for everyone reading this who is not iphy and thus probably doesn't understand the context, we kind of want to make tox-bootstrapd run on Windows in the future, possibly, maybe, so that's why we a bit concerned about MSVC despite it being linux/unix (posix?) daemon. |
|
Huh, this documentation for MSVC 2017 (and 2015?) C Runtime says that you can use |
iphydf
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @nurupo)
other/bootstrap_daemon/src/tox-bootstrapd.c, line 452 at r1 (raw file):
Previously, nurupo wrote…
You realize that you are suggesting potentially downcasting, for example, 64-bit unsigned integer to 32-bit unsigned integer? For example,
rlim_tcan be defined as a 64-bitunsigned long longon a system, but we would downcast it tounsignedwhich is 32-bit on that particular system. The issue with downcasting is that you might truncate/overflow the value in that log message. While I wouldn't expectrlim_tto be a such large number that it wouldn't fit in eitheruint32_torunsigned, for some reason its exact type/width are not defined in Linux, FreeBSD and OpenBSD manpages, and some systems do use 64-bit unsigned integers for it, in fact one (or more) of our systems on Travis-CI does, so the fd number could technically be set to a 64-bit unsigned integer (or even 128 bit if some weird ABI with 128 bit integers comes out, as the manpages don't specify exact type/width ofrlimit_t), so downcasting, while it would generally work, is not safe, it might result in us printing the wrong number.
Yes. Yet I don't expect systems to have set a specific number greater than 4 billion that is not numeric_limits<rlim_t>::max(). Regardless, rlim is not going to work on Windows, so it doesn't really matter.
|
Can you rebase on master? |
Huh. Somehow I went along with
without realizing that this won't be an issue as we would compile out that portion of the code on Windows. |
d220dc5 to
e264a8d
Compare
nurupo
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @nurupo)
.travis/cmake-freebsd-stage1, line at r1 (raw file):
Previously, iphydf wrote…
Any idea why reviewable thinks this is a binary file? Are there any weird control characters in it accidentally?
There are control characters on lines 60 and 66, they are there intentionally.
other/bootstrap_daemon/tox-bootstrapd.sh, line 41 at r1 (raw file):
Previously, iphydf wrote…
"file descriptors"
Fixed.
|
Removing the |
e264a8d to
7fe6f0f
Compare
|
Oh, and rebased on master. |
|
Can you rebase again, and then merge? |
tox-bootstrapd can use around 600 TCP sockets during TCP server's normal functioning. Many systems default to having a soft limit of 1024 open file descriptors, which we are close to reaching, so it was suggested we bump that limit to a higher number. iphy suggested increasing it to 32768.
7fe6f0f to
d89f83f
Compare
|
Rebased. I can merge it, I guess. git checkout master
git merge --ff-only bootstrapd-increase-nofileright? Seems to produce the right result locally. |
tox-bootstrapd can use around 600 TCP sockets during TCP server's normal
functioning. Many systems default to having a soft limit of 1024 open file
descriptors, which we are close to reaching, so it was suggested we bump that
limit to a higher number. iphy suggested increasing it to 32768.
Related to the discussion at #1227.
This change is