RFR: 8300645: Revert julong changes from 8194232 after 8292083

[Severin Gehwolf]

On Mon, 13 Feb 2023 20:57:55 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>>> I think the `assert(memswlimit > 0, "invariant");` doesn't give a clear explanation why this is the case. And seeing multiple of this kind of assert in the same file is problematic.
>> 
>> OK, I'll remove the asserts.
>> 
>>> Is it possible for `/memory.limit_in_bytes` to use a number that's larger than `max_jlong`?
>> 
>> Theoretically yes. This code really is accounting for `not-a-fixed-unlimited` value being specified for cgroups v1. So I argue we don't care. `max_jlong` is `9,223,372,036,854,775,807`. `max_julong` is `18,446,744,073,709,551,615`. Both are beyond *exabyte* values and we do compare against the actual physical memory of the system (which I'd argue won't be beyond exabytes any time soon?) anyway. We are dealing with a memory limit here, which are supposed to be *smaller* than the physical host values (common case).
>> 
>>> If so, `GET_CONTAINER_INFO(jlong, ... JLONG_FORMAT)` will fail and this function will return `OSCONTAINER_ERROR`. Previously, this function would return -1.
>>> 
>>> (the fact that `OSCONTAINER_ERROR` (-2) can be returned by this function is also problematic).
>> 
>> We don't care in this case, though. All negative values returned are being treated as "no limit". I.e. in both cases the container limit will be unlimited and there is no change from a users' perspective in that regard.
>> 
>> This is an attempt to fixing log traces, which the user actually *can* see and they don't really make sense, currently. For the theoretical case of the value exceeding `max_jlong` and then returning `OS_CONTAINER_ERROR` we are fine. The physical host memory value will be used instead. Even so, the trace would indicate `-2` in the logs indicating this. It's also not a question of correctness since we can assume that if the value is greater than `2^63-1` we have a unlimited config, unless there are really beyond exabytes memory containers out there.
>> 
>>> I actually think the old code is better. 
>> 
>> The old code wanted to deal with "recognizing unlimited container memory values". This is solved - since [JDK-8292083](https://bugs.openjdk.org/browse/JDK-8292083), by using the physical machine's memory as an upper bound of that value. There is really no point to run containers with a memory limit *larger* than the physical host. So I have to disagree. The old code of [JDK-8194232](https://bugs.openjdk.org/browse/JDK-8194232) was a kludge. The new code (with the physical's machine memory as an upper bound) is more sound.
>> 
>> Cases beyond `max_jlong` values are theoretical at best, and won't make a difference in this code. Once we have a large positive number, we know what to do.
>
>> I argue we don't care. max_jlong is 9,223,372,036,854,775,807
> 
> We do care if such a value can possibly be encountered. We don't want the VM to behave incorrectly. With the current implementation, it's hard to understand what happens when such a large value is encountered.
> 
> (In fact, the new behavior is wrong - the function will return -2 without considering `hier_memlimit`).
> 
> The problem is there's too much mental exercise when reading the code. There's casting, overflow/underflow, handling of special values of -1 and -2, and considerations for logging. Also, the same logic is duplicated in several places, so they can easily get out of sync in the future.
> 
> Is it possible to localize the logic to a utility function? 
> 
> Maybe something like this? All the casting and logging, etc, will be handled inside, in a single place.
> 
> 
> // Returns -1 if the limit is not specified, or there was a failure when reading the limit, or if the specified
> // limit is the same or larger than MIN2(physical_max, jlong_max).
> // Otherwise returns a non-negative value that's smaller than MIN2(physical_max, jlong_max).
> jlong limit = read_memory_limit("/memory.memsw.limit_in_bytes", physical_max);
> 
> 
> `physical_max` will be a `julong`:
> 
> 
> julong physical_max = os::Linux::physical_memory(); 
> - or -
> julong physical_max = os::Linux::host_swap() + os::Linux::physical_memory();

@iklam After some thought, moving to a utility would need to move it into a utility preprocessor `macro` since it uses `GET_CONTAINER_INFO` macros. Would you prefer that over the current version? Or is that not the way to go? Just checking..

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

PR: https://git.openjdk.org/jdk/pull/12166