8365606: Container code should not be using jlong/julong #27743
Conversation
|
👋 Welcome back sgehwolf! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Testing details:cgroup v2 F42 (docker): cgroup v2 F39 (docker): cgroup v1 RHEL 8 (podman): Note: The skipped test on RHEL 8 is |
|
/label add hotspot-jfr |
|
@jerboaa |
Webrevs
|
|
GHA test failure on Windows x64 is unrelated (this is a Linux only patch): |
|
There seems to be some collision here with JDK-8367319 |
Thanks, yes, I'm aware. We'll resolve conflicts once we know which one goes in first. |
|
Could anybody please help review this! Thank you. |
|
/reviewers 2 |
|
I'm not surprised about the lack of reviews because it's long and the result is rather ugly. (Sorry, but I had to say it.) This is less jarring to read: ... called as: |
|
I'm currently looking at this, just as a quick FYI. I'm fine with the proposed interface as it is (boolean return + passed reference). Since the adjacent |
I agree. Its not pretty, but consistent with what we did elsewhere. Nobody wants to do that discussion again. |
tstuefe
left a comment
tstuefe
left a comment
There was a problem hiding this comment.
Mostly looks good to me. Some remnants of raw UINT64_FORMAT are left. I did not see any type clashes.
I hold off the final review until it is clear that the interfaces are agreed upon.
| bool CgroupSubsystem::active_processor_count(int& value) { | ||
| int cpu_count; | ||
| int result; | ||
| int result = -1; |
There was a problem hiding this comment.
Why not get rid of result and use value throughout like you did in the cached case?
| return false; \ | ||
| } \ | ||
| log_trace(os, container)(log_string " is: " JULONG_FORMAT, retval); \ | ||
| log_trace(os, container)(log_string " is: " UINT64_FORMAT, retval); \ |
There was a problem hiding this comment.
Here and in other places: don't use raw UINT64_FORMAT; use PHYS_MEM_TYPE_FORMAT instead.
| is_ok = controller->read_string(filename, retval, buf_size); \ | ||
| if (!is_ok) { \ | ||
| log_trace(os, container)(log_string " failed: %d", OSCONTAINER_ERROR); \ | ||
| log_trace(os, container)(log_string " failed: -2"); \ |
There was a problem hiding this comment.
Why this change? Did the constant value change?
Sorry, I was unaware of any previous discussion. I was suggesting a less impactful way to make the change, taking advantage of the recent adoption of C++17, which allows for cleaner code. But I won't stand in the way of consensus. |
FWIW, I'd be interested in seeing a small example of what that would look like with C++17. There were a lot of discussion about the style, but it wasn't because we wanted to figure out the color of the bike shed but rather how to write safer code that makes it less likely to accidentally introduce bugs because of type conflation. |
This. A function that returns its value as a side effect on a reference parameter is (at best) a code smell. |
fitzsim
left a comment
fitzsim
left a comment
There was a problem hiding this comment.
I am not a reviewer. This looks good to me, overall. I commented with some items to consider.
| * All results of division are rounded up to the next whole number. | ||
| * | ||
| * If quotas have not been specified, return the | ||
| * number of active processors in the system. |
There was a problem hiding this comment.
This paragraph uses the "return" language that you adjusted in the next paragraph. It should probably also refer to the reference argument instead.
| * If quotas have not been specified, return the | ||
| * number of active processors in the system. | ||
| * | ||
| * If quotas have been specified, the resulting number |
There was a problem hiding this comment.
Tiny nit, but "the resulting number" => "the number", since you say "the result reference" on the next line.
| cpu_count = os::Linux::active_processor_count(); | ||
| result = CgroupUtil::processor_count(contrl->controller(), cpu_count); | ||
| if (!CgroupUtil::processor_count(contrl->controller(), cpu_count, result)) { | ||
| return false; |
There was a problem hiding this comment.
value will be returned unchanged from its passed-in value here. I wonder if it would be safer to explicitly set it to 0 when returning false. Also, could value be given an unsigned type, like uint64_t?
| * Return the limit of available memory for this process. | ||
| * Return the limit of available memory for this process in the provided | ||
| * physical_memory_size_type reference. If there was no limit value set in the underlying | ||
| * interface files value_unlimited is returned. |
There was a problem hiding this comment.
I think quote value_unlimited here to hint that it is a constant defined elsewhere.
Can the limit ever be 0, and if not, should there be a new assert for > 0 like for cpu_count?
| * Determine the memory and swap limit metric. Returns a positive limit value strictly | ||
| * lower than the physical memory and swap limit iff there is a limit. Otherwise a | ||
| * negative value is returned indicating the determined status. | ||
| * Determine the memory and swap limit metric. Returns a positive limit value or |
There was a problem hiding this comment.
"Returns" language should probably be updated here too.
| // cast to int since the read value might be negative | ||
| // and we want to avoid logging -1 as a large unsigned value. | ||
| int quota_int = (int)quota; | ||
| int quota_int = static_cast<int>(quota); |
There was a problem hiding this comment.
It seems like quota is either a positive number or disabled. I wonder if result can be treated as a uint64_t, and this log message special-cased to detect -1 read from /cpu.cfs_quota_us as disabled. I guess the calling code would need another way to differentiate "disabled" from other values... maybe with 0? Just a thought to maybe simplify the type logic here. Likewise for period and shares.
caspernorrbin
left a comment
caspernorrbin
left a comment
There was a problem hiding this comment.
Thanks for taking this on! I think this is a solid improvement, and it's great to finally see the Java types phased out. I had a few thoughts/comments, mostly around the os-level code and the logging of constants, since those constants don't really have much meaning anymore. Otherwise great work!
| physical_memory_size_type mem_usage = 0; | ||
| if (!OSContainer::memory_usage_in_bytes(mem_usage)) { | ||
| return false; | ||
| } | ||
| value = mem_usage; | ||
| return true; |
There was a problem hiding this comment.
Can we collapse this and just set the value reference directly instead in the container functions? Something like:
if (OSContainer::is_containerized()) {
return OSContainer::memory_usage_in_bytes(mem_usage);
}| physical_memory_size_type avail_mem = 0; | ||
| if (OSContainer::is_containerized() && OSContainer::available_memory_in_bytes(avail_mem)) { | ||
| log_trace(os)("available container memory: " PHYS_MEM_TYPE_FORMAT, avail_mem); | ||
| value = avail_mem; |
There was a problem hiding this comment.
Should be able to pass in value directly instead of using avail_mem.
| physical_memory_size_type free_mem = 0; | ||
| if (OSContainer::is_containerized() && OSContainer::available_memory_in_bytes(free_mem)) { | ||
| log_trace(os)("free container memory: " PHYS_MEM_TYPE_FORMAT, free_mem); | ||
| value = free_mem; |
There was a problem hiding this comment.
Should be able to pass in value directly instead of using free_mem.
| if (OSContainer::is_containerized()) { | ||
| jlong mem_swap_limit = OSContainer::memory_and_swap_limit_in_bytes(); | ||
| jlong mem_limit = OSContainer::memory_limit_in_bytes(); | ||
| if (mem_swap_limit >= 0 && mem_limit >= 0) { | ||
| jlong delta_limit = mem_swap_limit - mem_limit; | ||
| if (delta_limit <= 0) { | ||
| value = 0; | ||
| physical_memory_size_type mem_limit = 0; | ||
| physical_memory_size_type mem_swap_limit = 0; | ||
| if (OSContainer::memory_limit_in_bytes(mem_limit) && | ||
| OSContainer::memory_and_swap_limit_in_bytes(mem_swap_limit) && | ||
| mem_limit != value_unlimited && | ||
| mem_swap_limit != value_unlimited) { | ||
| if (mem_limit >= mem_swap_limit) { | ||
| value = 0; // no swap, thus no free swap | ||
| return true; | ||
| } | ||
| jlong mem_swap_usage = OSContainer::memory_and_swap_usage_in_bytes(); | ||
| jlong mem_usage = OSContainer::memory_usage_in_bytes(); | ||
| if (mem_swap_usage > 0 && mem_usage > 0) { | ||
| jlong delta_usage = mem_swap_usage - mem_usage; | ||
| if (delta_usage >= 0) { | ||
| jlong free_swap = delta_limit - delta_usage; | ||
| value = free_swap >= 0 ? static_cast<physical_memory_size_type>(free_swap) : 0; | ||
| physical_memory_size_type swap_limit = mem_swap_limit - mem_limit; | ||
| physical_memory_size_type mem_swap_usage = 0; | ||
| physical_memory_size_type mem_usage = 0; | ||
| if (OSContainer::memory_and_swap_usage_in_bytes(mem_swap_usage) && | ||
| OSContainer::memory_usage_in_bytes(mem_usage)) { | ||
| physical_memory_size_type swap_usage = value_unlimited; | ||
| if (mem_usage > mem_swap_usage) { | ||
| swap_usage = 0; // delta usage must not be negative | ||
| } else { | ||
| swap_usage = mem_swap_usage - mem_usage; | ||
| } | ||
| // free swap is based on swap limit (upper bound) and swap usage | ||
| if (swap_usage >= swap_limit) { | ||
| value = 0; // free swap must not be negative | ||
| return true; | ||
| } | ||
| value = swap_limit - swap_usage; | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This whole function is getting a bit too long in my opinion.
Maybe everything inside the if OSContainer::is_containerized() {} could be moved into a new function OSContainer::available_swap_in_bytes, similar to the already existing OSContainer::available_memory_in_bytes. That way, we could abstract away all the OSContainer calls.
The only consequence would be that the log_trace wouldn't work any more. I couldn't find any test that depends on the exact output, so it could perhaps be split up instead.
| int active_cpus = os::Linux::active_processor_count(); | ||
| if (OSContainer::is_containerized() && OSContainer::active_processor_count(active_cpus)) { | ||
| log_trace(os)("active_processor_count: determined by OSContainer: %d", | ||
| active_cpus); |
There was a problem hiding this comment.
When running containerized, we would now always fetch the os cpu count at least once.
CgroupSubsystem::active_processor_count, which this calls down to has the cache to actively avoid getting the cpu count too frequently, and only gets the number of cpus with os::Linux::active_processor_count when the cache expires.
I don't know if this is still an issue today, but since it's there I still think we should avoid getting the cpus if unnecessary.
| /* cpu_quota | ||
| * | ||
| * Return the number of microseconds per period | ||
| * process is guaranteed to run. | ||
| * | ||
| * return: | ||
| * quota time in microseconds | ||
| * -1 for no quota | ||
| * OSCONTAINER_ERROR for not supported | ||
| * true if the result reference has been set | ||
| * false on error | ||
| */ |
There was a problem hiding this comment.
The beginning part of the comment isn't updated to mention the result reference, unlike the other comments.
| if (!is_ok) { | ||
| log_trace(os, container)("CPU Usage failed: %d", OSCONTAINER_ERROR); | ||
| return OSCONTAINER_ERROR; | ||
| log_trace(os, container)("CPU Usage failed: -2"); |
There was a problem hiding this comment.
Do we need to keep the -2 here? Or could we perhaps change to a better message?
| if (!reader()->read_number_handle_max("/memory.swap.max", swap_limit_val)) { | ||
| // Some container tests rely on this trace logging to happen. | ||
| log_trace(os, container)("Swap Limit failed: %d", OSCONTAINER_ERROR); | ||
| log_trace(os, container)("Swap Limit failed: -2"); |
There was a problem hiding this comment.
Do we need to keep the -2 here? Or could we perhaps change to a better message?
| * Calculate the maximum number of tasks available to the process. Set the | ||
| * value in the passed in 'value' reference. The value might be -1 when | ||
| * there is no limit. |
There was a problem hiding this comment.
How can we get -1? Or do you mean (uint64_t)-1?
| // intentionally not using the macro so as to not log a | ||
| // negative value as a large unsiged int | ||
| if (!reader()->read_number("/cpu.cfs_quota_us", quota)) { | ||
| log_trace(os, container)("CPU Quota failed: -2"); |
There was a problem hiding this comment.
Do we need to keep the -2 here? Or could we perhaps change to a better message?
Thanks for the comments. So what's the consensus then? As far as API surface is concerned I've modelled it after JDK-8357086. It introduces the side-effect/code smell issue. Do we want to re-open this discussion or proceed with this here. It's not clear to me. |
|
@jerboaa this pull request can not be integrated into git checkout jdk-8365606-jlong-julong-refactor
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
7 participants
Please review this revised version of getting rid of
jlongandjulongin internal HotSpot code. The single remaining usage is usingos::elapsed_counter()which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as
uint64_tand afterwards interpreted in the way that make sense for the API implementations. For example,cpuvalues will essentially be treated asints as before, potentially returning a negative value-1for unlimited. For memory sizes the typephysical_memory_size_typehas been chosen. When there is no limit for a specific memory size a valuevalue_unlimitedis being returned.All error cases have been changed to returning
falsein the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return abooland require a reference to be passed in for thevaluethat is being asked for.All usages of the API have been changed to use the revised API. There is no more usages for
OSCONTAINER_ERROR(`-2) in HotSpot code.While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g.
os::Linux::active_processor_count()). I've filed JDK-8369503 to get this cleaned-up as this patch was already getting large.Testing (looking good):
jdk.SwapSpaceevent) andVM.infodiagnostic command.Thoughts? Opinions?
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27743/head:pull/27743$ git checkout pull/27743Update a local copy of the PR:
$ git checkout pull/27743$ git pull https://git.openjdk.org/jdk.git pull/27743/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27743View PR using the GUI difftool:
$ git pr show -t 27743Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27743.diff
Using Webrev
Link to Webrev Comment