RFR: 8357086: os::xxx functions returning memory size should return size_t [v4]

[Stefan Karlsson]

On Wed, 11 Jun 2025 08:35:30 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Anton Artemov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8357086: Fixed spaces in formatting in gc-related code.
>
> Are you up for making an experiment that changes `total_swap_space` and `free_swap_space` to return two values: one the actual value in `size_t` and  the other an error code that gets set whenever we hit an error?
> 
> The two proposed alternatives for this would be:
> 1) returning something containing the two values (struct, Pair, Tuple, array, ...)
> 2) Return the size_t and using an an out put parameter to signal an error (or vice versa)
> 
> I think the fan-out of that will not be too bad and the likely outcome is that it is clearer that the code is propagating errors.

> @stefank I inspected the container-related code once again, and came to conclusion that it is safe to use ssize_t, as you suggested above initially. The `OSCONTAINER_ERROR` return value will not be returned by `free_swap_space()` in os_linux.cpp as well as in anywhere in that file, because in all places there is a check for non-negativity. If negative, then the flow falls back to the host method, which can return value >= -1, i.e. fitting `ssize_t`. I will push these changes soon once tested.

Hmm. Maybe I'm reading this wrong, but to me it looks like you can return -2 via this code:

jlong CgroupV1MemoryController::read_memory_limit_in_bytes(julong phys_mem) {
  julong memlimit;
  CONTAINER_READ_NUMBER_CHECKED(reader(), "/memory.limit_in_bytes", "Memory Limit", memlimit);
  if (memlimit >= phys_mem) {
    verbose_log(memlimit, phys_mem);
    return (jlong)-1;
  } else {
    verbose_log(memlimit, phys_mem);
    return (jlong)memlimit;
  }
}

Note how `CONTAINER_READ_NUMBER_CHECKED` is a macro with a return statement:

#define CONTAINER_READ_NUMBER_CHECKED(controller, filename, log_string, retval)       \
{                                                                                     \
  bool is_ok;                                                                         \
  is_ok = controller->read_number(filename, &retval);                                 \
  if (!is_ok) {                                                                       \
    log_trace(os, container)(log_string " failed: %d", OSCONTAINER_ERROR);            \
    return OSCONTAINER_ERROR;                                                         \
  }                                                                                   \
  log_trace(os, container)(log_string " is: " JULONG_FORMAT, retval);                 \
}

(and `#define OSCONTAINER_ERROR (-2)`)

I think that the compiler would have caught that if we were to perform the experiment to change the cgroup code to return the proposed (size_t, error) pair and fix all callers to deal with that. With that said, if we have Kim's buy-in to put `-2` in `ssize_t` variables then I think that your patch retains the current behavior of the code and then we could defer an experiment like this to a PR that cleans up the cgroup code.

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

PR Comment: https://git.openjdk.org/jdk/pull/25450#issuecomment-2963566574