Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8072377

WhiteBox sweeper thread management bugs

    Details

    • Type: Bug
    • Status: Closed
    • Priority: P4
    • Resolution: Not an Issue
    • Affects Version/s: 9
    • Fix Version/s: 9
    • Component/s: hotspot
    • Labels:
      None

      Description

      The WhiteBox API allows for the creation and then start of a CodeCacheSweeperThread.

      There are two bugs in the current code:

      JavaThread* WhiteBox::create_sweeper_thread(TRAPS) {
        // create sweeper thread w/ custom entry -- one iteration instead of loop
        CodeCacheSweeperThread* sweeper_thread = new CodeCacheSweeperThread();
        sweeper_thread->set_entry_point(&WhiteBox::sweeper_thread_entry);

      First it is possible that the osThread could not be created due to resource/memory issues, so we need to add a check and bail out:

      JavaThread* WhiteBox::create_sweeper_thread(TRAPS) {
        // create sweeper thread w/ custom entry -- one iteration instead of loop
        CodeCacheSweeperThread* sweeper_thread = new CodeCacheSweeperThread();
        if (sweeper_thread->osthread() == NULL) {
          return NULL;
        }
        sweeper_thread->set_entry_point(&WhiteBox::sweeper_thread_entry);

      The second bug is that starting a JavaThread requires careful synchronization to ensure that the newly created thread can't run, terminate and be deallocated while the original starting thread is still trying to interact with it. For regular JavaThreads this is handled by the fact that java.lang.Thread.start() is a synchronized method, and before the new thread can terminate it has to acquire that same monitor (and do a notifyAll() as part of the Thread.join() implementation). This is documented in thread.cpp:

      void Thread::start(Thread* thread) {
        // Start is different from resume in that its safety is guaranteed by context or
        // being called from a Java method synchronized on the Thread object.

      and os.cpp:

      // The INITIALIZED state is distinguished from the SUSPENDED state because the
      // conditions in which a thread is first started are different from those in which
      // a suspension is resumed. These differences make it hard for us to apply the
      // tougher checks when starting threads that we want to do when resuming them.
      // However, when start_thread is called as a result of Thread.start, on a Java
      // thread, the operation is synchronized on the Java Thread object. So there
      // cannot be a race to start the thread and hence for the thread to exit while
      // we are working on it. Non-Java threads that start Java threads either have
      // to do so in a context in which races are impossible, or should do appropriate
      // locking.

      void os::start_thread(Thread* thread) {

      In the WhiteBox case there is no locking:

      WB_ENTRY(jobject, WB_ForceNMethodSweep(JNIEnv* env, jobject o))
        JavaThread* sweeper_thread = WhiteBox::create_sweeper_thread(Thread::current());
        if (sweeper_thread == NULL) {
          return NULL;
        }
        jobject result = JNIHandles::make_local(env, sweeper_thread->threadObj());
        Thread::start(sweeper_thread);
        return result;
      WB_END

      so deep inside Thread::start -> os::start_thread -> pd_start_thread we can logically release the start_lock monitor and as soon as we have done the low-level platform mutex unlock the new thread can run, terminate and delete all its data structures - including the start_lock monitor the original thread is still finishing "unlocking".

      This latter bug is an unlikely race due to what the thread may do:

      void WhiteBox::sweeper_thread_entry(JavaThread* thread, TRAPS) {
        guarantee(WhiteBoxAPI, "internal testing API :: WhiteBox has to be enabled");
        {
          MutexLockerEx mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
          NMethodSweeper::_should_sweep = true;
        }
        NMethodSweeper::possibly_sweep();
      }

      but it still may be possible.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                dholmes David Holmes
                Reporter:
                dholmes David Holmes
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: