-
Notifications
You must be signed in to change notification settings - Fork 196
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
[LibOS] Introduce sys.fds.limit
manifest option
#1973
Conversation
6af3d49
to
be90a36
Compare
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.
Reviewed 2 of 11 files at r1, all commit messages.
Reviewable status: 2 of 11 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
Documentation/manifest-syntax.rst
line 322 at r1 (raw file):
This specifies the maximum number of open file descriptors in the Gramine process. More specifically, this is equivalent to the ``RLIMIT_NOFILE`` resource
"equivalent" sounds like "this works same as that", while this is the RLIMIT_NOFILE
. I.e. we should make it clear that this option sets that limit, so it will be visible to the app.
Code quote:
More specifically, this is equivalent to the ``RLIMIT_NOFILE`` resource
Documentation/devel/features.md
line 2967 at r1 (raw file):
- `RLIMIT_NPROC` -- dummy, no limit by default - `RLIMIT_NOFILE` -- implemented, equal to `sys.fds.limit` {ref}`manifest option <sys-fds-limit>` by default
Please wrap
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.
Reviewed 9 of 11 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
libos/src/bookkeep/libos_handle.c
line 165 at r1 (raw file):
} if (fds_limit_init64 < 0) { log_error("'sys.fds.limit' = %ld is negative", fds_limit_init64);
Suggestion:
'sys.fds.limit' is negative (%ld)
libos/src/sys/libos_getrlimit.c
line 22 at r1 (raw file):
* inside Gramine can get resource limits, but these values will not reflect the actual limits on * the host. Similarly, the application inside Gramine can set resource limits, but these values * will not affect the limits on the host.
This whole paragraph doesn't make much sense IMO. Gramine is like a VM, these limits concern the emulated world, why would the host matter? (especially that it may not be Linux in theory, and that there's no 1-1 mapping between in-Gramine FDs and host FDs/handles)
If you want, you can just remind here that Gramine is emulating Linux, so all the limits regard the world inside Gramine, but I'd skip this altogether.
libos/src/sys/libos_getrlimit.c
line 28 at r1 (raw file):
* call * - RLIMIT_STACK: initially equal to `sys.stack.size` manifest option, but updating this limit * does *not* affect the max stack size of the main thread
Hmm, are we actually serializing the limits in checkpoints? If not, then we should.
Suggestion:
does *not* affect the max stack size of the main thread for processes which are already running
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.
Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
libos/test/regression/rlimit_nofile.c
line 10 at r1 (raw file):
#include <sys/resource.h> #include <sys/stat.h> #include <sys/time.h>
Are all these headers required?
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.
Reviewable status: 7 of 11 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)
Documentation/manifest-syntax.rst
line 322 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
"equivalent" sounds like "this works same as that", while this is the
RLIMIT_NOFILE
. I.e. we should make it clear that this option sets that limit, so it will be visible to the app.
Done.
Documentation/devel/features.md
line 2962 at r1 (raw file):
- `RLIMIT_DATA` -- implemented, affects `brk()` system call - `RLIMIT_STACK` -- dummy, equal to `sys.stack.size` {ref}`manifest option <stack-size>` by default
To @mkow: see for example this wrapping
Documentation/devel/features.md
line 2967 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Please wrap
Wrap how? We have 100 chars per line here, and this second line (with default
) is also wrapped correctly.
libos/src/bookkeep/libos_handle.c
line 165 at r1 (raw file):
} if (fds_limit_init64 < 0) { log_error("'sys.fds.limit' = %ld is negative", fds_limit_init64);
Done.
libos/src/sys/libos_getrlimit.c
line 22 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This whole paragraph doesn't make much sense IMO. Gramine is like a VM, these limits concern the emulated world, why would the host matter? (especially that it may not be Linux in theory, and that there's no 1-1 mapping between in-Gramine FDs and host FDs/handles)
If you want, you can just remind here that Gramine is emulating Linux, so all the limits regard the world inside Gramine, but I'd skip this altogether.
Done. Removed.
libos/src/sys/libos_getrlimit.c
line 28 at r1 (raw file):
Done.
Hmm, are we actually serializing the limits in checkpoints?
Good catch. We have it explicitly for RLIMIT_DATA
(brk stuff):
gramine/libos/src/sys/libos_brk.c
Lines 36 to 40 in 4a73025
/* TODO: this needs a better fix. Currently after fork, in the new child process, `libos_init` | |
* is run, hence this function too - but forked process will get its brk from checkpoints. */ | |
if (brk_region.brk_start) { | |
return 0; | |
} |
It looks like RLIMIT_STACK
is not used in the child process though; it seems to be always overwritten in init_stack()
:
gramine/libos/src/libos_init.c
Line 292 in 4a73025
set_rlimit_cur(RLIMIT_STACK, stack_size); |
I created a separate issue for RLIMIT_STACK
bug: #1974
libos/src/sys/libos_getrlimit.c
line 50 at r1 (raw file):
#define MQ_BYTES_MAX 819200 static struct __kernel_rlimit64 __rlim[RLIM_NLIMITS] __attribute_migratable = {
@mkow Notice how these resource limits are __attribute_migratable
, so they will be checkpointed and migrated to the child, where they will be restored. However, the libos_init()
code may override them if we don't put additional checks in the corresponding code snippets.
libos/test/regression/rlimit_nofile.c
line 10 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Are all these headers required?
Done. Removed some unused headers, added the ones needed for forking.
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.
Reviewable status: 7 of 11 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @kailun-qin, and @mkow)
libos/test/regression/test_libos.py
line 1073 at r2 (raw file):
self.assertIn("(before setrlimit) opened fd: 899", stdout) self.assertIn("new RLIMIT_NOFILE soft limit: 901", stdout) self.assertIn("(after setrlimit) opened fd: 900", stdout)
I must also add the check on the line (in child, after setrlimit) opened fd
libos/test/regression/test_libos.py
line 1082 at r2 (raw file):
self.assertIn("(before setrlimit) opened fd: 4095", stdout) self.assertIn("new RLIMIT_NOFILE soft limit: 4097", stdout) self.assertIn("(after setrlimit) opened fd: 4096", stdout)
I must also add the check on the line (in child, after setrlimit) opened fd
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.
Reviewable status: 7 of 11 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)
libos/src/sys/libos_getrlimit.c
line 28 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done.
Hmm, are we actually serializing the limits in checkpoints?
Good catch. We have it explicitly for
RLIMIT_DATA
(brk stuff):gramine/libos/src/sys/libos_brk.c
Lines 36 to 40 in 4a73025
/* TODO: this needs a better fix. Currently after fork, in the new child process, `libos_init` * is run, hence this function too - but forked process will get its brk from checkpoints. */ if (brk_region.brk_start) { return 0; } It looks like
RLIMIT_STACK
is not used in the child process though; it seems to be always overwritten ininit_stack()
:gramine/libos/src/libos_init.c
Line 292 in 4a73025
set_rlimit_cur(RLIMIT_STACK, stack_size); I created a separate issue for
RLIMIT_STACK
bug: #1974
FYI: #1976
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.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @kailun-qin)
Documentation/devel/features.md
line 2967 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Wrap how? We have 100 chars per line here, and this second line (with
default
) is also wrapped correctly.
Oh wait, those -
mislead me, it's good already.
libos/test/regression/rlimit_nofile.c
line 4 at r1 (raw file):
/* Copyright (C) 2024 Intel Corporation */ #include <err.h>
But you're using errx()
, so this header is needed.
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.
Reviewable status: 9 of 11 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @kailun-qin and @mkow)
libos/test/regression/rlimit_nofile.c
line 4 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
But you're using
errx()
, so this header is needed.
Done.
libos/test/regression/test_libos.py
line 1073 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I must also add the check on the line
(in child, after setrlimit) opened fd
Done.
libos/test/regression/test_libos.py
line 1082 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I must also add the check on the line
(in child, after setrlimit) opened fd
Done.
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.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @kailun-qin)
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.
Reviewed 3 of 4 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
This manifest option allows to modify the original `RLIMIT_NOFILE` resource limit. There is *no* way to propagate this limit from the host; this is a deliberate design choice. Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
aeacda6
to
ca534ce
Compare
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.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
Jenkins, test Jenkins-20.04 please |
Jenkins, test Jenkins-SGX-Sanitizers please |
Jenkins, test Jenkins-SGX-20.04-apps please |
Jenkins, test Jenkins-SGX-20.04-musl please |
Description of the changes
This manifest option allows to modify the original
RLIMIT_NOFILE
resource limit. There is no way to propagate this limit from the host; this is a deliberate design choice.See discussion in #1964.
Fixes #1576.
How to test this PR?
Added two LibOS tests.
This change is