RFR: 8300645: Revert julong changes from 8194232 after 8292083
Ioi Lam
iklam at openjdk.org
Mon Feb 13 21:15:31 UTC 2023
On Mon, 13 Feb 2023 19:00:50 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:
>> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 156:
>>
>>> 154:
>>> 155: jlong CgroupV1Subsystem::read_mem_swappiness() {
>>> 156: GET_CONTAINER_INFO(jlong, _memory->controller(), "/memory.swappiness",
>>
>> Why is this changed from julong to jlong? The only caller of this (private) function is to check the returned value against zero. It seems like this function should return julong instead to avoid all the casting and possible overflow/underflow.
>
>> Why is this changed from julong to jlong?
>
> Because `GET_CONTAINER_INFO` might log lines such as these (with `-Xlog:os+container=trace`):
>
> Swappiness is: 18446744073709551614
>
> ... on systems where for example `swapaccount=0` is being set on the kernel comandline. That is actually `-2`, i.e. `OS_CONTAINER_ERROR`. Which is just very confusing.
>
> That file, `memory.swappiness`, actually contains small numbers (if it exists). So there is really no need to read this in as `julong`. With this change it would print on `swapaccount=0` systems:
>
>
> Swappiness is: -2
>
>
> There is really no need to read as `julong`.
Is the motivation of this PR that `GET_CONTAINER_INFO` can't correctly print the error log for unsigned types?
log_trace(os, container)(logstring, (return_type) OSCONTAINER_ERROR); \
If so, I think it's better to fix the logging code so that it can handle unsigned types, and not change all values to signed types. Fixing logging is a lot easier than verifying that your type casting and range clipping is correct.
-------------
PR: https://git.openjdk.org/jdk/pull/12166
More information about the hotspot-runtime-dev
mailing list