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

[Severin Gehwolf]

On Thu, 29 Feb 2024 09:09:05 GMT, Matthias Baesken <mbaesken at openjdk.org> wrote:

>> 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();
>>    }
>
> Thanks for the suggestion, I adjusted the coding .  Should I add some UL logging in case of negative values returned?

I don't think that's needed.

>> 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);
>
> Makes sense. I adjusted the coding - btw should we add some UL logging in the case of failures/negative values ?

Up to you. It's probably not worth it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17966#discussion_r1507500872
PR Review Comment: https://git.openjdk.org/jdk/pull/17966#discussion_r1507501719