RFR: JDK-8325139: JFR SwapSpace event - add free swap space information on Linux when running in a container environment

[Severin Gehwolf]

On Thu, 22 Feb 2024 15:32:27 GMT, Matthias Baesken <mbaesken at openjdk.org> wrote:

> The JFR SwapSpace event (added by [JDK-8324287](https://bugs.openjdk.org/browse/JDK-8324287)) has been added but in a Linux containerized environment we currently just report -1, this should be enhanced. There is already some code in the mbeans (but in Java) that does something similar .

Have you considered adding tests? E.g. assert returned values for `jdk.SwapSpace` in `test/hotspot/jtreg/containers/docker/TestJFREvents.java` or something like that. The `totalSize` recording should amount to the `--memory-swap` value when running the container.

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 174:

> 172:   // isSwapEnabled - maybe compute once and save?
> 173:   GET_CONTAINER_INFO(julong, _memory->controller(), "/memory.memsw.limit_in_bytes", "Memory and Swap Limit is: ", JULONG_FORMAT, JULONG_FORMAT, memsw_bytes);
> 174:   GET_CONTAINER_INFO(julong, _memory->controller(), "/memory.swappiness", "Swappiness is: ", JULONG_FORMAT, JULONG_FORMAT, swappiness);

Why are you reading the memory and swap limit by means of the low level files here? I think what you want to know is whether or not swap is enabled. Note that the `GET_CONTAINER_INFO` macro returns a `julong` (sometimes a negative value cast to `julong`). So you'd need to account for that.

I suggest something like this:


   jlong memory_sw_limit = memory_and_swap_limit_in_bytes();
   jlong memory_limit = CgroupSubsystem::memory_limit_in_bytes();
   if (memory_sw_limit > 0 && memory_limit > 0) {
       jlong delta_swap = memory_sw_limit - memory_limit;
       if (delta_swap > 0) {
                  GET_CONTAINER_INFO(julong, _memory->controller(), "/memory.memsw.usage_in_bytes",
                     "mem swap usage is: ", JULONG_FORMAT, JULONG_FORMAT, memory_swap_usage);
                return (jlong)memory_swap_usage;
       } else {
          // no swap
          return 0;
       }
       return memory_usage_in_bytes();
   }

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 188:

> 186:         char* mem_swp_current_str = mem_swp_current_val();
> 187:         jlong swap_current = limit_from_str(mem_swp_current_str);
> 188:         return memory_usage + swap_current;

`limit_from_str()` might return negative values in case of `OS_CONTAINER_ERROR` (`-2`) or `max` => `-1`. Please account for that. For example:

return memory_usage + (swap_current >= 0 ? swap_current : 0);

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 190:

> 188:         return memory_usage + swap_current;
> 189:     }
> 190:     return memory_usage; // case of no memory limits

That comment seems misleading. Perhaps call it `// not supported or unlimited case`

src/hotspot/os/linux/os_linux.cpp line 319:

> 317:       // cgroupv1 : memory.memsw.usage_in_bytes (CgroupV1Subsystem.java)
> 318:       // cgroupv2 : long swapUsage = getLongVal("memory.swap.current", NO_SWAP); memoryUsage + swapUsage;  (CgroupV2Subsystem.java)
> 319:       //jlong memSwapUsage = containerMetrics.getMemoryAndSwapUsage();

Left-over comments?

src/hotspot/os/linux/os_linux.cpp line 328:

> 326:           if (free_swap >= 0) {
> 327:             return free_swap;
> 328:           }

If there is negative `free_swap` we should just return `0`. Suggest to use:


return free_swap >= 0 ? free_swap : 0;

src/hotspot/os/linux/os_linux.cpp line 332:

> 330:       }
> 331:     }
> 332:     return -1; // check the fallback - maybe use something better ?

This comment isn't very useful. What are you trying to convey?

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

PR Review: https://git.openjdk.org/jdk/pull/17966#pullrequestreview-1906431666
PR Review Comment: https://git.openjdk.org/jdk/pull/17966#discussion_r1506110752
PR Review Comment: https://git.openjdk.org/jdk/pull/17966#discussion_r1506115592
PR Review Comment: https://git.openjdk.org/jdk/pull/17966#discussion_r1506063348
PR Review Comment: https://git.openjdk.org/jdk/pull/17966#discussion_r1506125135
PR Review Comment: https://git.openjdk.org/jdk/pull/17966#discussion_r1506122181
PR Review Comment: https://git.openjdk.org/jdk/pull/17966#discussion_r1506128157