Make Linux memory allocations int64 not uint64

[justincormack]

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

• Fix unlimited memory issue on old kernel runc#1494
• Revert memory fields to signed integers runc#1492
• Use uint64 for resources to keep consistency with runtime-spec runc#1375
• setting Resources.MemorySwap unlimited after type change is messy runc#1421

Signed-off-by: Justin Cormack justin.cormack@docker.com

[crosbymichael]

LGTM

[tianon]

LGTM [](https://pullapprove.com/opencontainers/runtime-spec/pull-request/876/?utm_source=github-pr&utm_medium=comment-badge&utm_campaign=opencontainers/runtime-spec)

[wking]

I'm not sure how this PR fits into the current v1.0.0 freeze. I'd like to see it landed in 1.0, but I'm generally in favor of polishing vs. pushing 1.0 out quickly.

[wking]

Your newline is in the middle of the second sentence, but should be after the first period to match our policy.

I'd rather have the swappiness semantics documented in the swappiness entry below (or dropped entirely if you feel the current “sysctl's vm.swappiness” punt is sufficient). Then you can use:

For all parameters except swappiness, values specify…

And in the entries themselves, drop the now-redundant “bytes”. For example:

limit (int64, OPTIONAL) - limit memory usage
reservation (int64, OPTIONAL) - soft limit of memory usage
…

[wking]

Soft limits have some interesting caveats, so I'd be happier with a direct reference to memory.soft_limit_in_bytes. But that's orthogonal to this PR.

[justincormack]

@wking adjusted the wording and line breaks as suggested, and fixed the example which had 0 limits rather than unlimited or unspecified.

[wking]

On Fri, Jun 23, 2017 at 03:20:57PM -0700, Justin Cormack wrote: … and fixed the example which had `0` limits rather than unlimited or unspecified.

You'll also want to make that fix in [1,2]. [1]: https://github.com/opencontainers/runtime-spec/blame/c1a98486c417a03b56277a63a58d3456459e128c/config.md#L692-L693 [2]: https://github.com/opencontainers/runtime-spec/blob/c1a98486c417a03b56277a63a58d3456459e128c/schema/test/config/good/spec-example.json#L242-L243

[justincormack]

@wking amended

[wking]

Is 0 really “almost never”? From the cgroup docs:

Please note that unlike during the global reclaim, limit reclaim enforces that 0 swappiness really prevents from any swapping even if there is a swap storage available.

which seems stronger than the the sysctl docs:

A value of 0 instructs the kernel not to initiate swap until the amount of free and file-backed pages is less than the high water mark in a zone.

And neither one of those mentions a max at 100. I don't doubt that 100 is the current max, but I'd rather punt to kernel docs for the swappiness semantics instead of inlining them with this line.

[justincormack]

I reworded to paraphrase the comment in the code https://github.com/torvalds/linux/blob/master/mm/vmscan.c#L149

[justincormack]

It is a user facing ABI, so the limit of 100 will not change without adding a new parameter.

[wking]

I reworded to paraphrase the comment in the code…

But is exposing kernel comments in the spec a good idea? What if they change the semantics, MUST runtimes translate from the 0–100 range to whatever the local kernel uses?

What's important about this value is that it get written to memory.swappiness. I'd rather leave the config author and host kernel to sort out the memory.swappiness semantics between themselves, and have the runtime (and this spec) just blindly pass the opaque value through.

[justincormack]

If you believe the important thing is what is written to the memory.swappiness we should not be discussing this PR at all, we should change all these values to strings, which is what is actually written to the files.

Percentages, like bytes, are pretty common in human facing interfaces, just like some of the other values are 0 or 1 which we describe as booleans.

[wking]

If you believe the important thing is what is written to the memory.swappiness we should not be discussing this PR at all, we should change all these values to strings, which is what is actually written to the files.

Yeah, and a cgroup config like:

"<controller>": {
    "<filename-suffix>": "<string to write>"
}

For example:

"memory": {
  "swappiness": "60",
  "soft_limit_in_bytes": "-1",
}

would be as expressive as what we have now and a lot easier to document. And it extends to other controllers and properties without needing any spec changes.

The counter-argument is that that approach degenerates into an array-of-syscalls config (e.g. if you set capabilities as integers instead of CAP_* strings. Drawing the line somewhere is useful to keep configs human-readable, but "swappiness": 0 and "swappiness": "0" are equally legible to me.

But overhauling the cgroup-config API is a big change with a 1.0 freeze in place, so in the short term I'd rather just avoid baking more kernel-defined semantics into the spec in situations where the runtime implementation can ignore those semantics.

[mrunalp]

LGTM

[tianon]

LGTM

[hqhq]

See my comments in opencontainers/runc#1492 (comment) , the only benefit we can gain from "uint64 -> int64" is that it makes runc code easier, all other reasons are not making int64 better than uint64, just that int64 is acceptable. I don't know if that's kind of compromise we should make in OCI.

But since there's no real world issue for using int64 for now, let's not back and forth on this anymore, not before 1.0, just want us on the same page on this.

LGTM