Improve setting signalsWithMasterHandlers and fix omrsig_protect #2611
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
babsingh
force-pushed
the
fix_java8_gpf_issue
branch
from
9715cad to
4e22d92
Compare
|
fyi - @pshipton this is the fix for the gpf issue. i am testing changes in personal builds. i will remove the WIP after testing. |
pshipton
reviewed
Jun 1, 2018
| @@ -1274,10 +1258,22 @@ registerSignalHandlerWithOS(OMRPortLibrary *portLibrary, uint32_t portLibrarySig | |||
| } | |||
| } | |||
|
|
|||
| issueWriteBarrier(); | |||
|
|
|||
| /* Set the portLibrarySignalNo bit in signalsWithHandlers to record successful registration | |||
| * of the handler. */ | |||
| signalsWithHandlers |= portLibrarySignalNo; | |||
There was a problem hiding this comment.
Seems these updates need to be atomic. Otherwise without a monitor bits could be lost.
There was a problem hiding this comment.
a monitor is always acquired before calling registerSignalHandlerWithOS.
There was a problem hiding this comment.
Is there a lock being held at this point while the flags are being modified?
There was a problem hiding this comment.
the flags are modified within registerSignalHandlerWithOS, and registerHandlerMonitor is always acquired before calls to registerSignalHandlerWithOS. this info is documented in registerSignalHandlerWithOS's description.
Currently, signalsWithMasterHandlers is set at two places: omrsig_register_os_handler and registerMasterHandlers. With this changeset, signalsWithMasterHandlers will only be set within registerSignalHandlerWithOS after an OS handler has been successfully registered. A write barrier is issued before updating signalsWithHandlers and signalsWithMasterHandlers within registerSignalHandlerWithOS. The write barrier will allow us to read signalsWithHandlers and signalsWithMasterHandlers without acquiring registerHandlerMonitor. omrsig_protect serves asynchronous signals, and OS handlers for asynchronous signals are generally registered once at startup. Now onwards, signalsWithMasterHandlers is read without acquiring registerHandlerMonitor within omrsig_protect. This serves two purposes. First, omrsig_protect won't acquire registerHandlerMonitor if a handler is already registered. From past performance numbers, this would save 1828 instructions from the newDirectByteBuffer pathlength. Second, thread->monitor value ends up being NULL if the thread gets aborted during the call to omrthread_monitor_enter(registerHandlerMonitor). This impacts the behavior of omrthread.c::interrupt_waiting_server. In OpenJ9, intermittent segmentation faults are observed during calls to Java_java_lang_Thread_interruptImpl. By not acquiring registerHandlerMonitor if an OS handler is already registered, omrsig_protect avoids conflict with the thread library and prevents segmentation fault in OpenJ9. Signed-off-by: Babneet Singh <[email protected]>
babsingh
force-pushed
the
fix_java8_gpf_issue
branch
from
4e22d92 to
ab151c5
Compare
babsingh
changed the title
|
Removed the WIP tag. Personal builds and local testing have succeeded. |
|
@genie-omr build all |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, signalsWithMasterHandlers is set at two places:
omrsig_register_os_handler and registerMasterHandlers.
With this changeset, signalsWithMasterHandlers will only be set within
registerSignalHandlerWithOS after an OS handler has been successfully
registered.
A write barrier is issued before updating signalsWithHandlers and
signalsWithMasterHandlers within registerSignalHandlerWithOS. The write
barrier will allow us to read signalsWithHandlers and
signalsWithMasterHandlers without acquiring registerHandlerMonitor.
omrsig_protect serves asynchronous signals, and OS handlers for
asynchronous signals are generally registered once at startup. Now
onwards, signalsWithMasterHandlers is read without acquiring
registerHandlerMonitor within omrsig_protect. This serves two purposes.
First, omrsig_protect won't acquire registerHandlerMonitor if a handler
is already registered. From past performance numbers, this would save
1828 instructions from the newDirectByteBuffer pathlength.
Second, thread->monitor value ends up being NULL if the thread gets
aborted during the call to
omrthread_monitor_enter(registerHandlerMonitor). This impacts the
behavior of omrthread.c::interrupt_waiting_server. In OpenJ9,
intermittent segmentation faults are observed during calls to
Java_java_lang_Thread_interruptImpl. By not acquiring
registerHandlerMonitor if an OS handler is already registered,
omrsig_protect avoids conflict with the thread library and prevents
segmentation fault in OpenJ9.
Signed-off-by: Babneet Singh [email protected]