8351933: Inaccurate masking of TC subfield decrement in ForkJoinPool

[dchuyko]

Please review a tiny fix in the ForkJoinPool. Since JDK 9 (JDK-8134852 [1]) in one case when TC subfield in ctl field is decremented, the applied masking (UMASK, upper bits) may not preserve neighbor RC subfield sometimes. In JDKs prior to 19 FJP may stop executing tasks, which requires a long running application restart [2]. Since 19 it is even harder to reproduce because of the separate parallelism field.

The fix is to replace 'UMASK & (c - TC_UNIT)' with '(c & RC_MASK) | ((c - TC_UNIT) & TC_MASK)' which preserves the RC part of the compareAndSetCtl() candidate argument. On 17u and 11u that repairs known tests and applications. This PR is for the mainline, and I intend to backport it to 21u, 17u and 11u.

[1] https://bugs.openjdk.org/browse/JDK-8134852
[2] https://bugs.openjdk.org/browse/JDK-8330017

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

• JDK-8351933: Inaccurate masking of TC subfield decrement in ForkJoinPool (Bug - P4)

Reviewers

• Chen Liang (@liach - Reviewer) Review applies to e5517c7d
• Doug Lea (@DougLea - Reviewer)
• Alan Bateman (@AlanBateman - Reviewer)

Reviewers without OpenJDK IDs

• @Lujie1996 (no known openjdk.org user name / role) Review applies to e5517c7d

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24034/head:pull/24034
$ git checkout pull/24034

Update a local copy of the PR:
$ git checkout pull/24034
$ git pull https://git.openjdk.org/jdk.git pull/24034/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24034

View PR using the GUI difftool:
$ git pr show -t 24034

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24034.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back dchuyko! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@dchuyko This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8351933: Inaccurate masking of TC subfield decrement in ForkJoinPool

Reviewed-by: dl, alanb, liach

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 206 new commits pushed to the master branch:

• 8f64ccc: 8350485: C2: factor out common code in Node::grow() and Node::out_grow()
• c2be19c: 8351902: RISC-V: Several tests fail after JDK-8351145
• e57b272: 8350578: Refactor useless Parse and Template Assertion Predicate elimination code by using a PredicateVisitor
• 577ede7: 8352302: Test sun/security/tools/jarsigner/TimestampCheck.java is failing
• 20d4fe3: 8351464: Shenandoah: Hang on ShenandoahController::handle_alloc_failure when run test TestAllocHumongousFragment#generational
• 8e53063: 8352275: Clean up dead code in jpackage revealed with improved negative test coverage
• c8a11f2: 8352293: jpackage tests build rpm packages on Ubuntu test machines after JDK-8351372
• 4a02de8: 8352098: -Xrunjdwp fails on static JDK
• 355b2f3: 8351374: Improve comment about queue.remove timeout in CleanerImpl.run
• a3540be: 8352163: [AIX] SIGILL in AttachOperation::ReplyWriter::write_fully after 8319055
• ... and 196 more: https://git.openjdk.org/jdk/compare/c988d7d6476807bf71a977dc771017915b708ca3...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@dchuyko The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (973c84cd)
• 00: Full (e5517c7d)

[Lujie1996]

Hi Dmitry. Thank you for putting up the pull request.

[dchuyko]

While this has been included into upcoming FJP update for better delayed task handling DougLea@9b51b7a (JDK-8319447), a separate commit will make the fix much easier in update releases.

[liach]

It seems this is an overflow for the TC part of the flag. The RC part seems to be able to overflow too, but because it occupies the higher bits its overflow will never affect the TC. And this is the only site where TC is masked with UMASK after overflow-capable addition. Should we reexamine all uses of UMASK to mask only RC/TC explicitly?

[dchuyko]

It seems this is an overflow for the TC part of the flag. The RC part seems to be able to overflow too, but because it occupies the higher bits its overflow will never affect the TC. And this is the only site where TC is masked with UMASK after overflow-capable addition.

Yes, from this point RC (in)accurate masking can be considered cosmetic, but this might be the source of this bug with TC.

Should we reexamine all uses of UMASK to mask only RC/TC explicitly?

'(v.stackPred & SP_MASK) | (UC_MASK & c)' patterns seem ok to me, but for all subfield arithmetic it would be more clear to see all subfields recombined.

[dchuyko]

So I'd propose to consider RC masking separately, and maybe as a part of larger FJP update (such as JDK-8319447), without a need to backport.

[liach]

The issue looks valid to me and the fix looks good. Would be nice if another reviewer can double check.

[liach]

/reviewers 2 reviewer

[liach]

Note that some reviewers may be busy with the ongoing JavaOne. Please wait for another reviewer in area to double check.

[openjdk]

@liach
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[AlanBateman]

As noted, this is fixed by the FJP changes in JDK-8319447. If the change is going in separately then best to take the exact change to tryTrim to avoid merge issues.

[dchuyko]

Agree, reformatted the code. That doesn't affect any backports (already prepared).

[DougLea]

Yes, this is the right fix; thanks!

[dchuyko]

Thanks for reviews!

[dchuyko]

/integrate

[openjdk]

Going to push as commit fed34e4.
Since your change was applied there have been 206 commits pushed to the master branch:

• 8f64ccc: 8350485: C2: factor out common code in Node::grow() and Node::out_grow()
• c2be19c: 8351902: RISC-V: Several tests fail after JDK-8351145
• e57b272: 8350578: Refactor useless Parse and Template Assertion Predicate elimination code by using a PredicateVisitor
• 577ede7: 8352302: Test sun/security/tools/jarsigner/TimestampCheck.java is failing
• 20d4fe3: 8351464: Shenandoah: Hang on ShenandoahController::handle_alloc_failure when run test TestAllocHumongousFragment#generational
• 8e53063: 8352275: Clean up dead code in jpackage revealed with improved negative test coverage
• c8a11f2: 8352293: jpackage tests build rpm packages on Ubuntu test machines after JDK-8351372
• 4a02de8: 8352098: -Xrunjdwp fails on static JDK
• 355b2f3: 8351374: Improve comment about queue.remove timeout in CleanerImpl.run
• a3540be: 8352163: [AIX] SIGILL in AttachOperation::ReplyWriter::write_fully after 8319055
• ... and 196 more: https://git.openjdk.org/jdk/compare/c988d7d6476807bf71a977dc771017915b708ca3...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dchuyko Pushed as commit fed34e4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.