RFR: 8332506: SIGFPE In ObjectSynchronizer::is_async_deflation_needed() [v3]

[David Holmes]

On Sun, 5 Jan 2025 15:18:20 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

>> This PR solves a division by zero problem, that according to the bug report happened in `ObjectSynchronizer::is_async_deflation_needed()`. As it turns out it really happened in the inlined  `monitors_used_above_threshold()` function. The problematic line looked like this:
>> 
>> `size_t monitor_usage = (monitors_used * 100LL) / ceiling;`
>> 
>> Unfortunately the `ceiling` value was increased every time there where too many deflations without any progress. This whould eventually lead to an overflow in the `ceiling` value, and in unlucky circumstances, it would become zero. Thus causing a division by zero crash.
>> 
>> This PR makes sure not to increase the `ceiling` value if `monitor_usage` is below the `MonitorUsedDeflationThreshold`.
>> It also makes sure the ceiling value is never zero, and does not wrap around.
>> 
>> Tested okay in tier1-5 and `test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java`
>
> Fredrik Bredberg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update two after review

I remain concerned that this both addresses the potential overflow, and also changes related logic about the ceiling/threshold in a way that I do not understand. It would help if there was a clear definition of what the "ceiling" means and how it relates to `list->max()` and `list->count()`.

src/hotspot/share/runtime/synchronizer.cpp line 1274:

> 1272:   // Make sure the we use a ceiling value that is not lower than the
> 1273:   // max used by the system, and not zero.
> 1274:   size_t ceiling = MAX3(old_ceiling, list->max(), monitors_used);

The comment does not explain why these three components are checked.

src/hotspot/share/runtime/synchronizer.cpp line 1288:

> 1286:         _no_progress_cnt >= NoAsyncDeflationProgressMax) {
> 1287:       double remainder = (100.0 - MonitorUsedDeflationThreshold) / 100.0;
> 1288:       size_t delta = (size_t)((double)ceiling * remainder) + 1;

Suggestion:

      size_t delta = (size_t)(ceiling * remainder) + 1;

src/hotspot/share/runtime/synchronizer.cpp line 1307:

> 1305:                                ", monitor_usage=" SIZE_FORMAT ", threshold=%d",
> 1306:                                monitors_used, ceiling, monitor_usage, MonitorUsedDeflationThreshold);
> 1307:     return is_above_threshold;

This can now return false if the monitor usage has fallen below the threshold again - is that what should be returned??

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

PR Review: https://git.openjdk.org/jdk/pull/22815#pullrequestreview-2531040262
PR Review Comment: https://git.openjdk.org/jdk/pull/22815#discussion_r1903427689
PR Review Comment: https://git.openjdk.org/jdk/pull/22815#discussion_r1903401962
PR Review Comment: https://git.openjdk.org/jdk/pull/22815#discussion_r1903403788