Attention AIX developers - factoring out POSIX signal code (JDK-8252324)
Thomas Stüfe
thomas.stuefe at gmail.com
Tue Sep 1 22:45:56 UTC 2020
Hi Martin,
off hand looks good. Thanks for taking care of this.
Cheers, Thomas
On Tue, Sep 1, 2020 at 10:11 PM Doerr, Martin <martin.doerr at sap.com> wrote:
> Hi Gerard,
>
>
>
> it compiles without errors and warnings with the following patch. I don’t
> think we still need this old stuff, but I hope Thomas can find some time to
> double-check.
>
> I haven’t run it through our testing, yet.
>
>
>
> Best regards,
>
> Martin
>
>
>
>
>
> diff -r ec42084221a6 src/hotspot/os/aix/os_aix.cpp
>
> --- a/src/hotspot/os/aix/os_aix.cpp Tue Sep 01 21:54:57 2020 +0200
>
> +++ b/src/hotspot/os/aix/os_aix.cpp Tue Sep 01 22:05:45 2020 +0200
>
> @@ -1563,61 +1563,16 @@
>
> os::signal_notify(sig);
>
> }
>
>
>
> -void* os::user_handler() {
>
> - return CAST_FROM_FN_PTR(void*, UserHandler);
>
> -}
>
> -
>
> extern "C" {
>
> typedef void (*sa_handler_t)(int);
>
> typedef void (*sa_sigaction_t)(int, siginfo_t *, void *);
>
> }
>
>
>
> -void* os::signal(int signal_number, void* handler) {
>
> - struct sigaction sigAct, oldSigAct;
>
> -
>
> - sigfillset(&(sigAct.sa_mask));
>
> -
>
> - // Do not block out synchronous signals in the signal handler.
>
> - // Blocking synchronous signals only makes sense if you can really
>
> - // be sure that those signals won't happen during signal handling,
>
> - // when the blocking applies. Normal signal handlers are lean and
>
> - // do not cause signals. But our signal handlers tend to be "risky"
>
> - // - secondary SIGSEGV, SIGILL, SIGBUS' may and do happen.
>
> - // On AIX, PASE there was a case where a SIGSEGV happened, followed
>
> - // by a SIGILL, which was blocked due to the signal mask. The process
>
> - // just hung forever. Better to crash from a secondary signal than to
> hang.
>
> - sigdelset(&(sigAct.sa_mask), SIGSEGV);
>
> - sigdelset(&(sigAct.sa_mask), SIGBUS);
>
> - sigdelset(&(sigAct.sa_mask), SIGILL);
>
> - sigdelset(&(sigAct.sa_mask), SIGFPE);
>
> - sigdelset(&(sigAct.sa_mask), SIGTRAP);
>
> -
>
> - sigAct.sa_flags = SA_RESTART|SA_SIGINFO;
>
> -
>
> - sigAct.sa_handler = CAST_TO_FN_PTR(sa_handler_t, handler);
>
> -
>
> - if (sigaction(signal_number, &sigAct, &oldSigAct)) {
>
> - // -1 means registration failed
>
> - return (void *)-1;
>
> - }
>
> -
>
> - return CAST_FROM_FN_PTR(void*, oldSigAct.sa_handler);
>
> -}
>
> -
>
> -void os::signal_raise(int signal_number) {
>
> - ::raise(signal_number);
>
> -}
>
> -
>
> //
>
> // The following code is moved from os.cpp for making this
>
> // code platform specific, which it is by its very nature.
>
> //
>
>
>
> -// Will be modified when max signal is changed to be dynamic
>
> -int os::sigexitnum_pd() {
>
> - return NSIG;
>
> -}
>
> -
>
> // a counter for each possible signal value
>
> static volatile jint pending_signals[NSIG+1] = { 0 };
>
>
>
> @@ -1687,11 +1642,6 @@
>
> local_sem_init();
>
> }
>
>
>
> -void os::signal_notify(int sig) {
>
> - Atomic::inc(&pending_signals[sig]);
>
> - local_sem_post();
>
> -}
>
> -
>
> static int check_pending_signals() {
>
> for (;;) {
>
> for (int i = 0; i < NSIG + 1; i++) {
>
> @@ -1728,9 +1678,6 @@
>
> }
>
> }
>
>
>
> -int os::signal_wait() {
>
> - return check_pending_signals();
>
> -}
>
>
>
>
> ////////////////////////////////////////////////////////////////////////////////
>
> // Virtual Memory
>
> diff -r ec42084221a6 src/hotspot/os/posix/signals_posix.cpp
>
> --- a/src/hotspot/os/posix/signals_posix.cpp Tue Sep 01 21:54:57 2020
> +0200
>
> +++ b/src/hotspot/os/posix/signals_posix.cpp Tue Sep 01 22:05:45 2020
> +0200
>
> @@ -95,12 +95,7 @@
>
> #endif
>
>
>
> // sun.misc.Signal support
>
> -#if !defined(AIX)
>
> - static Semaphore* sig_semaphore = NULL;
>
> -#else
>
> - static sem_t sig_semaphore;
>
> - static msemaphore* p_sig_msem = 0;
>
> -#endif
>
> +static Semaphore* sig_semaphore = NULL;
>
> // a counter for each possible signal value
>
> static volatile jint pending_signals[NSIG+1] = { 0 };
>
>
>
> @@ -272,71 +267,17 @@
>
> // sun.misc.Signal support
>
>
>
> static void local_sem_init() {
>
> -#if !defined(AIX)
>
> sig_semaphore = new Semaphore();
>
> -#else
>
> - if (os::Aix::on_aix()) {
>
> - int rc = ::sem_init(&sig_semaphore, 0, 0);
>
> - guarantee(rc != -1, "sem_init failed");
>
> - } else {
>
> - // Memory semaphores must live in shared mem.
>
> - guarantee0(p_sig_msem == NULL);
>
> - p_sig_msem = (msemaphore*)os::reserve_memory(sizeof(msemaphore),
> NULL);
>
> - guarantee(p_sig_msem, "Cannot allocate memory for memory semaphore");
>
> - guarantee(::msem_init(p_sig_msem, 0) == p_sig_msem, "msem_init
> failed");
>
> - }
>
> -#endif
>
> }
>
>
>
> // Wrapper functions for: sem_init(), sem_post(), sem_wait()
>
> -// On AIX, we use sem_init(), sem_post(), sem_wait()
>
> -// On Pase, we need to use msem_lock() and msem_unlock(), because Posix
> Semaphores
>
> -// do not seem to work at all on PASE (unimplemented, will cause SIGILL).
>
> -// Note that just using msem_.. APIs for both PASE and AIX is not an
> option either, as
>
> -// on AIX, msem_..() calls are suspected of causing problems.
>
>
>
> static void local_sem_post() {
>
> -#if !defined(AIX)
>
> sig_semaphore->signal();
>
> -#else
>
> - static bool warn_only_once = false;
>
> - if (os::Aix::on_aix()) {
>
> - int rc = ::sem_post(&sig_semaphore);
>
> - if (rc == -1 && !warn_only_once) {
>
> - trcVerbose("sem_post failed (errno = %d, %s)", errno,
> os::errno_name(errno));
>
> - warn_only_once = true;
>
> - }
>
> - } else {
>
> - guarantee0(p_sig_msem != NULL);
>
> - int rc = ::msem_unlock(p_sig_msem, 0);
>
> - if (rc == -1 && !warn_only_once) {
>
> - trcVerbose("msem_unlock failed (errno = %d, %s)", errno,
> os::errno_name(errno));
>
> - warn_only_once = true;
>
> - }
>
> - }
>
> - #endif
>
> }
>
>
>
> static void local_sem_wait() {
>
> -#if !defined(AIX)
>
> sig_semaphore->wait();
>
> -#else
>
> - static bool warn_only_once = false;
>
> - if (os::Aix::on_aix()) {
>
> - int rc = ::sem_wait(&sig_semaphore);
>
> - if (rc == -1 && !warn_only_once) {
>
> - trcVerbose("sem_wait failed (errno = %d, %s)", errno,
> os::errno_name(errno));
>
> - warn_only_once = true;
>
> - }
>
> - } else {
>
> - guarantee0(p_sig_msem != NULL); // must init before use
>
> - int rc = ::msem_lock(p_sig_msem, 0);
>
> - if (rc == -1 && !warn_only_once) {
>
> - trcVerbose("msem_lock failed (errno = %d, %s)", errno,
> os::errno_name(errno));
>
> - warn_only_once = true;
>
> - }
>
> - }
>
> - #endif
>
> }
>
>
>
> void PosixSignals::jdk_misc_signal_init() {
>
> @@ -1573,7 +1514,6 @@
>
> os::SuspendResume::State current = osthread->sr.state();
>
>
>
> // TODO: reconcile the differences betrween Linux/BSD vs AIX here!
>
> -#if !defined(AIX)
>
> if (current == os::SuspendResume::SR_SUSPEND_REQUEST) {
>
> suspend_save_context(osthread, siginfo, context);
>
>
>
> @@ -1617,45 +1557,6 @@
>
> } else {
>
> // ignore
>
> }
>
> -#else
>
> - if (current == os::SuspendResume::SR_SUSPEND_REQUEST) {
>
> - suspend_save_context(osthread, siginfo, context);
>
> -
>
> - // attempt to switch the state, we assume we had a SUSPEND_REQUEST
>
> - os::SuspendResume::State state = osthread->sr.suspended();
>
> - if (state == os::SuspendResume::SR_SUSPENDED) {
>
> - sigset_t suspend_set; // signals for sigsuspend()
>
> - sigemptyset(&suspend_set);
>
> -
>
> - // get current set of blocked signals and unblock resume signal
>
> - pthread_sigmask(SIG_BLOCK, NULL, &suspend_set);
>
> - sigdelset(&suspend_set, SR_signum);
>
> -
>
> - // wait here until we are resumed
>
> - while (1) {
>
> - sigsuspend(&suspend_set);
>
> -
>
> - os::SuspendResume::State result = osthread->sr.running();
>
> - if (result == os::SuspendResume::SR_RUNNING) {
>
> - break;
>
> - }
>
> - }
>
> -
>
> - } else if (state == os::SuspendResume::SR_RUNNING) {
>
> - // request was cancelled, continue
>
> - } else {
>
> - ShouldNotReachHere();
>
> - }
>
> -
>
> - resume_clear_context(osthread);
>
> - } else if (current == os::SuspendResume::SR_RUNNING) {
>
> - // request was cancelled, continue
>
> - } else if (current == os::SuspendResume::SR_WAKEUP_REQUEST) {
>
> - // ignore
>
> - } else {
>
> - ShouldNotReachHere();
>
> - }
>
> -#endif
>
>
>
> errno = old_errno;
>
> }
>
> @@ -1721,7 +1622,6 @@
>
> }
>
>
>
> // TODO: reconcile the differences betrween Linux/BSD vs AIX here!
>
> -#if !defined(AIX)
>
> if (sr_notify(osthread) != 0) {
>
> ShouldNotReachHere();
>
> }
>
> @@ -1745,44 +1645,6 @@
>
> }
>
> }
>
> }
>
> -#else
>
> - if (sr_notify(osthread) != 0) {
>
> - // try to cancel, switch to running
>
> -
>
> - os::SuspendResume::State result = osthread->sr.cancel_suspend();
>
> - if (result == os::SuspendResume::SR_RUNNING) {
>
> - // cancelled
>
> - return false;
>
> - } else if (result == os::SuspendResume::SR_SUSPENDED) {
>
> - // somehow managed to suspend
>
> - return true;
>
> - } else {
>
> - ShouldNotReachHere();
>
> - return false;
>
> - }
>
> - }
>
> -
>
> - // managed to send the signal and switch to SUSPEND_REQUEST, now wait
> for SUSPENDED
>
> -
>
> - for (int n = 0; !osthread->sr.is_suspended(); n++) {
>
> - for (int i = 0; i < RANDOMLY_LARGE_INTEGER2 &&
> !osthread->sr.is_suspended(); i++) {
>
> - os::naked_yield();
>
> - }
>
> -
>
> - // timeout, try to cancel the request
>
> - if (n >= RANDOMLY_LARGE_INTEGER) {
>
> - os::SuspendResume::State cancelled = osthread->sr.cancel_suspend();
>
> - if (cancelled == os::SuspendResume::SR_RUNNING) {
>
> - return false;
>
> - } else if (cancelled == os::SuspendResume::SR_SUSPENDED) {
>
> - return true;
>
> - } else {
>
> - ShouldNotReachHere();
>
> - return false;
>
> - }
>
> - }
>
> - }
>
> -#endif
>
>
>
> guarantee(osthread->sr.is_suspended(), "Must be suspended");
>
> return true;
>
> @@ -1799,7 +1661,6 @@
>
> }
>
>
>
> // TODO: reconcile the differences betrween Linux/BSD vs AIX here!
>
> -#if !defined(AIX)
>
> while (true) {
>
> if (sr_notify(osthread) == 0) {
>
> if (sr_semaphore.timedwait(2)) {
>
> @@ -1811,19 +1672,6 @@
>
> ShouldNotReachHere();
>
> }
>
> }
>
> -#else
>
> - while (!osthread->sr.is_running()) {
>
> - if (sr_notify(osthread) == 0) {
>
> - for (int n = 0; n < RANDOMLY_LARGE_INTEGER &&
> !osthread->sr.is_running(); n++) {
>
> - for (int i = 0; i < 100 && !osthread->sr.is_running(); i++) {
>
> - os::naked_yield();
>
> - }
>
> - }
>
> - } else {
>
> - ShouldNotReachHere();
>
> - }
>
> - }
>
> -#endif
>
> -
>
> +
>
> guarantee(osthread->sr.is_running(), "Must be running!");
>
> }
>
> diff -r ec42084221a6 src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp
>
> --- a/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp Tue Sep 01 21:54:57 2020
> +0200
>
> +++ b/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp Tue Sep 01 22:05:45 2020
> +0200
>
> @@ -51,6 +51,7 @@
>
> #include "runtime/stubRoutines.hpp"
>
> #include "runtime/thread.inline.hpp"
>
> #include "runtime/timer.hpp"
>
> +#include "signals_posix.hpp"
>
> #include "utilities/events.hpp"
>
> #include "utilities/vmError.hpp"
>
> #ifdef COMPILER1
>
> @@ -225,7 +226,7 @@
>
>
>
> JavaThread* thread = NULL;
>
> VMThread* vmthread = NULL;
>
> - if (os::Aix::signal_handlers_are_installed) {
>
> + if (PosixSignals::are_signal_handlers_installed()) {
>
> if (t != NULL) {
>
> if(t->is_Java_thread()) {
>
> thread = (JavaThread*)t;
>
>
>
>
>
> *From:* gerard ziemski <gerard.ziemski at oracle.com>
> *Sent:* Dienstag, 1. September 2020 18:36
> *To:* Doerr, Martin <martin.doerr at sap.com>; Thomas Stüfe <
> thomas.stuefe at gmail.com>
> *Cc:* hotspot-runtime-dev at openjdk.java.net; Schmidt, Lutz <
> lutz.schmidt at sap.com>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
> Stuefe, Thomas <thomas.stuefe at sap.com>
> *Subject:* Re: Attention AIX developers - factoring out POSIX signal code
> (JDK-8252324)
>
>
>
> Thank you Martin, Thomas for your feedback.
>
> On 8/31/20 12:20 PM, Doerr, Martin wrote:
>
> However, jdk/jdk doesn’t support as400 PASE, so I’d be fine with using
> Gerard’s new POSIX code and removing all AIX specific stuff which was built
> to support as400 PASE.
>
> I believe the semaphore stuff works on AIX, so we don’t need “#if
> !defined(AIX)”, right?
>
> Just to make sure I understand it correctly: we don't need AIX specific
> semaphore code (i.e. local_sem_init(), local_sem_post() and
> local_sem_wait()), so we can remove it and go back to using runtime
> Semaphore for BSD/Linux/AIX?
>
> What other PASE specific code is currently in singlas_posix.cpp that
> should be removed? I see some extra signal unblocking code in os::signal()
> that is for AIX platform (with a comment that it applies to both AIX and
> PASE). That should stay in though, right?
>
> Is this something you want me to do, or will you clean that up yourself as
> appropriate?
>
> @Thomas, I assigned https://bugs.openjdk.java.net/browse/JDK-825253 to
> myself as per your suggestion.
>
>
> cheers
>
More information about the hotspot-runtime-dev
mailing list