Skip to content

Fix unlimited memory issue on old kernel #1494

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.

Sign up for GitHub

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

Jump to bottom
Closed
hqhq wants to merge 1 commit into from
Closed

Fix unlimited memory issue on old kernel #1494

hqhq wants to merge 1 commit into from

Conversation

@hqhq
Copy link
Contributor

@hqhq hqhq commented Jun 23, 2017

Fixes #1421

On old kernels (before 3.11), max memory limit is
LLONG_MAX, which is 2^63-1 on most platforms, so we
should not use uint64(-1) for unlimited memory.

Keep using -1 for cgroup API when people want to
set memory unlimited to fix this.

Signed-off-by: Qiang Huang [email protected]

@hqhq
Fix unlimited memory issue on old kernel
6532d80 
Fixes opencontainers#1421

On old kernels (before 3.11), max memory limit is
LLONG_MAX, which is 2^63-1 on most platforms, so we
should not use uint64(-1) for unlimited memory.

Keep using `-1` for cgroup API when people want to
set memory unlimited to fix this.

Signed-off-by: Qiang Huang <[email protected]>
This was referenced Jun 23, 2017
Closed
Closed
Closed
@justincormack
Copy link
Contributor

justincormack commented Jun 23, 2017

This should fix the immediate problem we found during testing, so we should merge once confirmed, as the current code does not work at all on older kernels (you cannot set a memory limit at all).

I still think that having the memory limits as uint64 is not the best solution, as -1 is an allowed value, and the maximum allowed value is 2^63, due to the kernel/userspace split which means that no value larger than this is valid, so an int64 is a much better fit to the externally presented interface from the kernel. There is nowhere that the internals of the value being a uint64 are exposed, so there is no syscall API to these values, only a string API.

@dqminh
Copy link
Contributor

dqminh commented Jun 23, 2017

I still think that having the memory limits as uint64 is not the best solution, as -1 is an allowed value, and the maximum allowed value is 2^63, due to the kernel/userspace split which means that no value larger than this is valid, so an int64 is a much better fit to the externally presented interface from the kernel.

Yah its a bit weird right now because essentially we are doing double-cast, where we translate user input from -1 into uint64, than cast back to -1 again.

The argument before was that we should stick to the internal kernel's value presentation (i.e. uint64), but maybe we should actually just conform to the kernel external interface for these settings (i.e int64)

@crosbymichael
Copy link
Member

crosbymichael commented Jun 23, 2017

@dqminh ya that sounds like a good idea. But does this also need a spec change?

@crosbymichael
Copy link
Member

crosbymichael commented Jun 23, 2017

I think we should change these back to -1. This PR fixes it but it has to add a lot of code to work around the issue. We have been running with -1 for years now and it has worked correctly. Lets either revert the kmem accounting or the entire thing and not add more code around this as cgroups accepts -1

@justincormack
Copy link
Contributor

justincormack commented Jun 23, 2017

I am happy to do a PR to change the spec back to int64, was planning to do one anyway, but I think it helps to discuss here too, and runc is currently badly broken.

@crosbymichael
Copy link
Member

crosbymichael commented Jun 23, 2017

@justincormack tibor already opened a PR for this: #1492

@justincormack
Copy link
Contributor

justincormack commented Jun 23, 2017

@crosbymichael I meant a PR to change the runtime spec, not runc. Will do one now.

justincormack added a commit to justincormack/runtime-spec that referenced this pull request Jun 23, 2017
@justincormack
Make Linux memory allocations int64 not uint64
6990fda 
The kernel ABI to these values is a string, which accepts the value `-1`
to mean "unlimited" or an integer up to 2^63 for an amount of memory in
bytes.

While the internal representation in the kernel is unsigned, this is not
exposed in any ABI directly. Because of the user-kernel memory split, values
over 2^63 are not really useful; indeed that much memory is not supported,
as physical memory is limited to 52 bits in the forthcoming switch to five
level page tables. So it is much more natural to support the value `-1` for
unlimited, especially as the actual number needed to represent the maximum
has varied in different kernel versions, and across 32 and 64 bit architectures,
so determining the value to use is not possible, so it is necessary to write
the string `-1` to the cgroup files.

See also discussion in
- opencontainers/runc#1494
- opencontainers/runc#1492
- opencontainers/runc#1375
- opencontainers/runc#1421

Signed-off-by: Justin Cormack <[email protected]>
@justincormack justincormack mentioned this pull request Jun 23, 2017
Merged
@justincormack
Copy link
Contributor

justincormack commented Jun 23, 2017

Spec PR opened opencontainers/runtime-spec#876

justincormack added a commit to justincormack/runtime-spec that referenced this pull request Jun 23, 2017
@justincormack
Make Linux memory allocations int64 not uint64
97c466a 
The kernel ABI to these values is a string, which accepts the value `-1`
to mean "unlimited" or an integer up to 2^63 for an amount of memory in
bytes.

While the internal representation in the kernel is unsigned, this is not
exposed in any ABI directly. Because of the user-kernel memory split, values
over 2^63 are not really useful; indeed that much memory is not supported,
as physical memory is limited to 52 bits in the forthcoming switch to five
level page tables. So it is much more natural to support the value `-1` for
unlimited, especially as the actual number needed to represent the maximum
has varied in different kernel versions, and across 32 and 64 bit architectures,
so determining the value to use is not possible, so it is necessary to write
the string `-1` to the cgroup files.

See also discussion in
- opencontainers/runc#1494
- opencontainers/runc#1492
- opencontainers/runc#1375
- opencontainers/runc#1421

Signed-off-by: Justin Cormack <[email protected]>
justincormack added a commit to justincormack/runtime-spec that referenced this pull request Jun 23, 2017
@justincormack
Make Linux memory allocations int64 not uint64
5de3024 
The kernel ABI to these values is a string, which accepts the value `-1`
to mean "unlimited" or an integer up to 2^63 for an amount of memory in
bytes.

While the internal representation in the kernel is unsigned, this is not
exposed in any ABI directly. Because of the user-kernel memory split, values
over 2^63 are not really useful; indeed that much memory is not supported,
as physical memory is limited to 52 bits in the forthcoming switch to five
level page tables. So it is much more natural to support the value `-1` for
unlimited, especially as the actual number needed to represent the maximum
has varied in different kernel versions, and across 32 and 64 bit architectures,
so determining the value to use is not possible, so it is necessary to write
the string `-1` to the cgroup files.

See also discussion in
- opencontainers/runc#1494
- opencontainers/runc#1492
- opencontainers/runc#1375
- opencontainers/runc#1421

Signed-off-by: Justin Cormack <[email protected]>
justincormack added a commit to justincormack/runtime-spec that referenced this pull request Jun 23, 2017
@justincormack
Make Linux memory allocations int64 not uint64
e73cd70 
The kernel ABI to these values is a string, which accepts the value `-1`
to mean "unlimited" or an integer up to 2^63 for an amount of memory in
bytes.

While the internal representation in the kernel is unsigned, this is not
exposed in any ABI directly. Because of the user-kernel memory split, values
over 2^63 are not really useful; indeed that much memory is not supported,
as physical memory is limited to 52 bits in the forthcoming switch to five
level page tables. So it is much more natural to support the value `-1` for
unlimited, especially as the actual number needed to represent the maximum
has varied in different kernel versions, and across 32 and 64 bit architectures,
so determining the value to use is not possible, so it is necessary to write
the string `-1` to the cgroup files.

See also discussion in
- opencontainers/runc#1494
- opencontainers/runc#1492
- opencontainers/runc#1375
- opencontainers/runc#1421

Signed-off-by: Justin Cormack <[email protected]>
justincormack added a commit to justincormack/runc that referenced this pull request Jun 24, 2017
@justincormack
Update memory specs to use int64 not uint64
48f04f8 
replace opencontainers#1492 opencontainers#1494
fix opencontainers#1422

Since opencontainers/runtime-spec#876 the memory
specifications are now `int64`, as that better matches the visible interface where
`-1` is a valid value. Otherwise finding the correct value was difficult as it
was kernel dependent.

Signed-off-by: Justin Cormack <[email protected]>
@justincormack justincormack mentioned this pull request Jun 24, 2017
Merged
justincormack added a commit to justincormack/runc that referenced this pull request Jun 27, 2017
@justincormack
Update memory specs to use int64 not uint64
3d9074e 
replace opencontainers#1492 opencontainers#1494
fix opencontainers#1422

Since opencontainers/runtime-spec#876 the memory
specifications are now `int64`, as that better matches the visible interface where
`-1` is a valid value. Otherwise finding the correct value was difficult as it
was kernel dependent.

Signed-off-by: Justin Cormack <[email protected]>
@dqminh
Copy link
Contributor

dqminh commented Jun 27, 2017

This can be closed now with #1495

@dqminh dqminh closed this Jun 27, 2017
tiborvass pushed a commit to tiborvass/runtime-spec that referenced this pull request Jul 6, 2017
@justincormack @tiborvass
Make Linux memory allocations int64 not uint64
5f473f3 
The kernel ABI to these values is a string, which accepts the value `-1`
to mean "unlimited" or an integer up to 2^63 for an amount of memory in
bytes.

While the internal representation in the kernel is unsigned, this is not
exposed in any ABI directly. Because of the user-kernel memory split, values
over 2^63 are not really useful; indeed that much memory is not supported,
as physical memory is limited to 52 bits in the forthcoming switch to five
level page tables. So it is much more natural to support the value `-1` for
unlimited, especially as the actual number needed to represent the maximum
has varied in different kernel versions, and across 32 and 64 bit architectures,
so determining the value to use is not possible, so it is necessary to write
the string `-1` to the cgroup files.

See also discussion in
- opencontainers/runc#1494
- opencontainers/runc#1492
- opencontainers/runc#1375
- opencontainers/runc#1421

Signed-off-by: Justin Cormack <[email protected]>
(cherry picked from commit e73cd70)
Signed-off-by: Tibor Vass <[email protected]>
@tiborvass tiborvass mentioned this pull request Jul 6, 2017
Merged
tiborvass pushed a commit to tiborvass/runc that referenced this pull request Jul 6, 2017
@justincormack
Update memory specs to use int64 not uint64
654477c 
replace opencontainers#1492 opencontainers#1494
fix opencontainers#1422

Since opencontainers/runtime-spec#876 the memory
specifications are now `int64`, as that better matches the visible interface where
`-1` is a valid value. Otherwise finding the correct value was difficult as it
was kernel dependent.

Signed-off-by: Justin Cormack <[email protected]>
(cherry picked from commit 3d9074e)
Signed-off-by: Tibor Vass <[email protected]>
tiborvass pushed a commit to tiborvass/runc that referenced this pull request Jul 6, 2017
@justincormack
Update memory specs to use int64 not uint64
2a13a01 
replace opencontainers#1492 opencontainers#1494
fix opencontainers#1422

Since opencontainers/runtime-spec#876 the memory
specifications are now `int64`, as that better matches the visible interface where
`-1` is a valid value. Otherwise finding the correct value was difficult as it
was kernel dependent.

Signed-off-by: Justin Cormack <[email protected]>
(cherry picked from commit 3d9074e)
Signed-off-by: Tibor Vass <[email protected]>
adrianreber pushed a commit to adrianreber/runc that referenced this pull request Jul 27, 2017
@justincormack @adrianreber
Update memory specs to use int64 not uint64
eec6814 
replace opencontainers#1492 opencontainers#1494
fix opencontainers#1422

Since opencontainers/runtime-spec#876 the memory
specifications are now `int64`, as that better matches the visible interface where
`-1` is a valid value. Otherwise finding the correct value was difficult as it
was kernel dependent.

Signed-off-by: Justin Cormack <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

setting Resources.MemorySwap unlimited after type change is messy

4 participants

@hqhq @justincormack @dqminh @crosbymichael