RFR: 8321931: memory_swap_current_in_bytes reports 0 as "unlimited"

[Gerard Ziemski]

On Wed, 17 Jan 2024 07:52:24 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> > > I'll have a look tomorrow. In general unlimited means `-1` not `0`. cg v1 doesn't explicitly set it to a specified value if it is unlimited. It's a very large number. For cg v2 it's usually the string `max` as specified by the kernel docs of interface files.
> > 
> > 
> > Thank you for taking a look.
> 
> According to https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git/tree/Documentation/admin-guide/cgroup-v2.rst
> 
> ```
>  memory.swap.max
> 	A read-write single value file which exists on non-root
> 	cgroups.  The default is "max".
> 
> 	Swap usage hard limit.  If a cgroup's swap usage reaches this
> 	limit, anonymous memory of the cgroup will not be swapped out.
> ```
> 
> E.g.,
> 
> ```
> $ cat /sys/fs/cgroup/user.slice/memory.swap.max
> max
> ```
> 
> I don't think there's a file under /sys/fs/cgroup where the numberic value of "0" means "unlimited". Instead, "0" means zero or "not allowed at all".
> 
> Otherwise, how can you express "don't allow any swapping for this container"?
> 
> I think the proper fix is:
> 
> ```
> void OSContainer::print_container_helper(outputStream* st, jlong j, const char* metrics) {
>   st->print("%s: ", metrics);
> - if (j > 0) {
> + if (j >= 0) {
> ```
> 
>  > I think I saw this special treatment of 0 value somewhere in source code. I will have to look.
> 
> It's worthwhile scanning the source code to for such cases and possibly fixing them.
> 
> In most cases, I see that -1 means unlimited, and -2 (OSCONTAINER_ERROR) means error:
> 
> ```
> jlong CgroupSubsystem::limit_from_str(char* limit_str) {
>   if (limit_str == nullptr) {
>     return OSCONTAINER_ERROR;
>   }
>   // Unlimited memory in cgroups is the literal string 'max' for
>   // some controllers, for example the pids controller.
>   if (strcmp("max", limit_str) == 0) {
>     os::free(limit_str);
>     return (jlong)-1;
>   }
>   julong limit;
>   if (sscanf(limit_str, JULONG_FORMAT, &limit) != 1) {
>     os::free(limit_str);
>     return OSCONTAINER_ERROR;
>   }
>   os::free(limit_str);
>   return (jlong)limit;
> }
> ```

My original fix was just being extra careful and was fixing non-limit type values only.

However, looking at https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git/tree/Documentation/admin-guide/cgroup-v2.rst I noticed a section on `Limits` which says:

`Limits are in the range [0, max] and defaults to "max", which is noop.
`

so 0 is a valid value, with no special meaning, so your suggestion on accepting 0 in all cases looks valid to me. Looks like the original implementation got this incorrect.

Thank you for the review and feedback!

-------------

PR Comment: https://git.openjdk.org/jdk/pull/17314#issuecomment-1898745428