From ed94a88925d5a08dd98dab5338f8a3685674f35c Mon Sep 17 00:00:00 2001 From: "B. Scott Michel" Date: Thu, 19 Jun 2025 14:06:26 -0700 Subject: [PATCH] SCP: Acqire mutexes for both cond signals and waits Acquire enclosing mutex used with pthread_cond_wait() when calling pthread_cond_signal() or pthread_cond_broadcast(). This eliminates the possibility that pthread_cond_wait() misses the corresponding pthread_- cond_signal(). See https://stackoverflow.com/questions/4544234/ for a good diagram that explains the thread interleaving. Also look for premature mutex unlocks. As the Linux pthread_cond_- signal() man page states, "... if predictable scheduling behavior is required, then that mutex shall be locked by the thread calling pthread_cond_broadcast() or pthread_cond_signal()." ETH: - Add startup condvar and mutex, have _eth_reader and _eth_writer signal successful thread startup (as other SIMH threads do.) - Set thread names for reader and writer threads. --- scp.c | 3 ++- sim_console.c | 1 + sim_ether.c | 40 ++++++++++++++++++++++++++++++++++----- sim_ether.h | 4 ++++ sim_frontpanel.c | 6 +++--- sim_tape.c | 6 ++++-- sim_timer.c | 49 +++++++++++++++++++++++++++++------------------- sim_tmxr.c | 8 +++++--- 8 files changed, 84 insertions(+), 33 deletions(-) diff --git a/scp.c b/scp.c index 7d9ea9cf1..60b6a3c56 100644 --- a/scp.c +++ b/scp.c @@ -437,12 +437,13 @@ else { uptr->a_next = q; /* Mark as on list */ } while (q != AIO_QUEUE_SET(uptr, q)); } -AIO_IUNLOCK; sim_asynch_check = 0; /* try to force check */ if (sim_idle_wait) { sim_debug (TIMER_DBG_IDLE, &sim_timer_dev, "waking due to event on %s after %d %s\n", sim_uname(uptr), event_time, sim_vm_interval_units); + /* sim_asynch_lock must remain acquired during the condition signal. */ pthread_cond_signal (&sim_asynch_wake); } +AIO_IUNLOCK; } #else t_bool sim_asynch_enabled = FALSE; diff --git a/sim_console.c b/sim_console.c index 1c0e82d4b..d46c69cc0 100644 --- a/sim_console.c +++ b/sim_console.c @@ -3162,6 +3162,7 @@ sim_debug (DBG_ASY, &sim_con_telnet, "_console_poll() - starting\n"); pthread_mutex_lock (&sim_tmxr_poll_lock); pthread_cond_signal (&sim_console_startup_cond); /* Signal we're ready to go */ +/* We still have the poll lock acquired entering the loop. */ while (sim_asynch_enabled) { if (!sim_is_running) { diff --git a/sim_ether.c b/sim_ether.c index 6e6121843..5ad2155d9 100644 --- a/sim_ether.c +++ b/sim_ether.c @@ -1994,7 +1994,7 @@ _eth_callback((u_char *)opaque, &header, buf); static void * _eth_reader(void *arg) { -ETH_DEV* volatile dev = (ETH_DEV*)arg; +ETH_DEV* volatile dev = (ETH_DEV *) arg; int status = 0; int sel_ret = 0; int do_select = 0; @@ -2028,6 +2028,11 @@ sim_debug(dev->dbit, dev->dptr, "Reader Thread Starting\n"); when this thread needs to run */ sim_os_set_thread_priority (PRIORITY_ABOVE_NORMAL); +/* Signal that we've started... */ +pthread_mutex_lock(&dev->startup_mtx); +pthread_cond_signal(&dev->startup_cond); +pthread_mutex_unlock(&dev->startup_mtx); + while (dev->handle) { #if defined (_WIN32) if (dev->eth_api == ETH_API_PCAP) { @@ -2212,7 +2217,7 @@ return NULL; static void * _eth_writer(void *arg) { -ETH_DEV* volatile dev = (ETH_DEV*)arg; +ETH_DEV* volatile dev = (ETH_DEV *) arg; ETH_WRITE_REQUEST *request = NULL; /* Boost Priority for this I/O thread vs the CPU instruction execution @@ -2222,6 +2227,11 @@ sim_os_set_thread_priority (PRIORITY_ABOVE_NORMAL); sim_debug(dev->dbit, dev->dptr, "Writer Thread Starting\n"); +/* Signal that we've started... */ +pthread_mutex_lock(&dev->startup_mtx); +pthread_cond_signal(&dev->startup_cond); +pthread_mutex_unlock(&dev->startup_mtx); + pthread_mutex_lock (&dev->writer_lock); while (dev->handle) { pthread_cond_wait (&dev->writer_cond, &dev->writer_lock); @@ -2760,6 +2770,8 @@ dev->dbit = dbit; #if defined (USE_READER_THREAD) if (1) { pthread_attr_t attr; + char thr_name[16]; + const size_t n_thr_name = sizeof(thr_name) / sizeof(char); ethq_init (&dev->read_queue, 200); /* initialize FIFO queue */ pthread_mutex_init (&dev->lock, NULL); @@ -2778,8 +2790,21 @@ if (1) { } } #endif /* defined(__hpux) */ - pthread_create (&dev->reader_thread, &attr, _eth_reader, (void *)dev); - pthread_create (&dev->writer_thread, &attr, _eth_writer, (void *)dev); + pthread_cond_init (&dev->startup_cond, NULL); + pthread_mutex_init (&dev->startup_mtx, NULL); + pthread_mutex_lock (&dev->startup_mtx); + pthread_create (&dev->reader_thread, &attr, _eth_reader, dev); + pthread_cond_wait(&dev->startup_cond, &dev->startup_mtx); + snprintf(thr_name, n_thr_name - 1, "r: %s", dev->name); + thr_name[n_thr_name - 1] = '\0'; + pthread_setname_np(dev->reader_thread, thr_name); + /* pthread_cond_wait() returns with startup_mtx locked. */ + pthread_create (&dev->writer_thread, &attr, _eth_writer, dev); + pthread_cond_wait(&dev->startup_cond, &dev->startup_mtx); + snprintf(thr_name, n_thr_name - 1, "w: %s", dev->name); + thr_name[n_thr_name - 1] = '\0'; + pthread_setname_np(dev->writer_thread, thr_name); + pthread_mutex_unlock (&dev->startup_mtx); pthread_attr_destroy(&attr); } #endif /* defined (USE_READER_THREAD */ @@ -2849,11 +2874,15 @@ dev->have_host_nic_phy_addr = 0; #if defined (USE_READER_THREAD) pthread_join (dev->reader_thread, NULL); pthread_mutex_destroy (&dev->lock); +pthread_mutex_lock(&dev->writer_lock); pthread_cond_signal (&dev->writer_cond); +pthread_mutex_unlock(&dev->writer_lock); pthread_join (dev->writer_thread, NULL); pthread_mutex_destroy (&dev->self_lock); pthread_mutex_destroy (&dev->writer_lock); pthread_cond_destroy (&dev->writer_cond); +pthread_mutex_destroy(&dev->startup_mtx); +pthread_cond_destroy(&dev->startup_cond); if (1) { ETH_WRITE_REQUEST *buffer; while (NULL != (buffer = dev->write_buffers)) { @@ -3373,8 +3402,9 @@ memcpy(request->packet.msg, packet->msg, packet->len); } /* Awaken writer thread to perform actual write */ -pthread_mutex_unlock (&dev->writer_lock); +pthread_mutex_lock (&dev->writer_lock); pthread_cond_signal (&dev->writer_cond); +pthread_mutex_unlock (&dev->writer_lock); /* Return with a status from some prior write */ if (routine) diff --git a/sim_ether.h b/sim_ether.h index f0ecb292d..d02c6aa6f 100644 --- a/sim_ether.h +++ b/sim_ether.h @@ -325,6 +325,10 @@ struct eth_device { int write_queue_peak; ETH_WRITE_REQUEST *write_buffers; t_stat write_status; + + /* Thread startup state. */ + pthread_mutex_t startup_mtx; + pthread_cond_t startup_cond; #endif }; diff --git a/sim_frontpanel.c b/sim_frontpanel.c index c390ef9f4..99ee0bc2a 100644 --- a/sim_frontpanel.c +++ b/sim_frontpanel.c @@ -403,8 +403,8 @@ pthread_setspecific (panel_thread_id, "debugflush"); pthread_mutex_lock (&p->io_lock); p->debugflush_thread_running = 1; -pthread_mutex_unlock (&p->io_lock); pthread_cond_signal (&p->startup_done); /* Signal we're ready to go */ +pthread_mutex_unlock (&p->io_lock); msleep (100); pthread_mutex_lock (&p->io_lock); while (p->sock != INVALID_SOCKET) { @@ -2157,8 +2157,8 @@ if (!p->parent) { } } p->io_thread_running = 1; -pthread_mutex_unlock (&p->io_lock); pthread_cond_signal (&p->startup_done); /* Signal we're ready to go */ +pthread_mutex_unlock (&p->io_lock); msleep (100); pthread_mutex_lock (&p->io_lock); while ((p->sock != INVALID_SOCKET) && @@ -2460,8 +2460,8 @@ _panel_debug (p, DBG_THR, "Starting", NULL, 0); pthread_mutex_lock (&p->io_lock); p->callback_thread_running = 1; -pthread_mutex_unlock (&p->io_lock); pthread_cond_signal (&p->startup_done); /* Signal we're ready to go */ +pthread_mutex_unlock (&p->io_lock); msleep (100); pthread_mutex_lock (&p->io_lock); while ((p->sock != INVALID_SOCKET) && diff --git a/sim_tape.c b/sim_tape.c index 450b98a84..8469d00d4 100644 --- a/sim_tape.c +++ b/sim_tape.c @@ -239,10 +239,11 @@ struct tape_context *ctx = (struct tape_context *)uptr->tape_ctx; pthread_mutex_lock (&ctx->io_lock); pthread_cond_signal (&ctx->startup_cond); /* Signal we're ready to go */ while (1) { + /* Entering the loop, io_lock is acquired. */ pthread_cond_wait (&ctx->io_cond, &ctx->io_lock); + pthread_mutex_unlock (&ctx->io_lock); if (ctx->io_top == TOP_DONE) break; - pthread_mutex_unlock (&ctx->io_lock); switch (ctx->io_top) { case TOP_RDRF: ctx->io_status = sim_tape_rdrecf (uptr, ctx->buf, ctx->bc, ctx->max); @@ -299,9 +300,10 @@ struct tape_context *ctx = (struct tape_context *)uptr->tape_ctx; pthread_mutex_lock (&ctx->io_lock); ctx->io_top = TOP_DONE; pthread_cond_signal (&ctx->io_done); + pthread_mutex_unlock(&ctx->io_lock); /* Allow waiters to wake up. */ sim_activate (uptr, ctx->asynch_io_latency); + pthread_mutex_lock(&ctx->io_lock); /* Re-acquire for the condition wait at loop's top. */ } - pthread_mutex_unlock (&ctx->io_lock); sim_debug_unit (ctx->dbit, uptr, "_tape_io(unit=%d) exiting\n", (int)(uptr-ctx->dptr->units)); diff --git a/sim_timer.c b/sim_timer.c index 333d26a97..b05ff021b 100644 --- a/sim_timer.c +++ b/sim_timer.c @@ -2306,6 +2306,7 @@ sim_debug (DBG_TIM, &sim_timer_dev, "_timer_thread() - starting\n"); pthread_mutex_lock (&sim_timer_lock); sim_timer_thread_running = TRUE; pthread_cond_signal (&sim_timer_startup_cond); /* Signal we're ready to go */ +pthread_mutex_unlock (&sim_timer_lock); /* Wake up waiters. */ while (sim_asynch_timer && sim_is_running) { struct timespec start_time, stop_time; struct timespec due_time; @@ -2314,6 +2315,12 @@ while (sim_asynch_timer && sim_is_running) { double inst_per_sec; UNIT *uptr, *cptr, *prvptr; + /* Acquire sim_timer_lock and guard the simulator's wall clock queue's internals. + * It will be temporarily released by pthread_cond_timedwait() if the timer thread + * has to wait for work to do. sim_timer_lock is released at the bottom of the + * loop. + */ + pthread_mutex_lock(&sim_timer_lock); if (sim_wallclock_entry) { /* something to insert in queue? */ sim_debug (DBG_TIM, &sim_timer_dev, "_timer_thread() - timing %s for %s\n", @@ -2354,33 +2361,35 @@ while (sim_asynch_timer && sim_is_running) { if (sim_wallclock_queue == QUEUE_LIST_END) sim_debug (DBG_TIM, &sim_timer_dev, "_timer_thread() - waiting forever\n"); else - sim_debug (DBG_TIM, &sim_timer_dev, "_timer_thread() - waiting for %.0f usecs until %.6f for %s\n", wait_usec, sim_wallclock_queue->a_due_time, sim_uname(sim_wallclock_queue)); + sim_debug (DBG_TIM, &sim_timer_dev, "_timer_thread() - waiting for %.0f usecs until %.6f for %s\n", + wait_usec, sim_wallclock_queue->a_due_time, sim_uname(sim_wallclock_queue)); if ((wait_usec <= 0.0) || - (0 != pthread_cond_timedwait (&sim_timer_wake, &sim_timer_lock, &due_time))) { + (0 != pthread_cond_timedwait (&sim_timer_wake, &sim_timer_lock, &due_time))) { - if (sim_wallclock_queue == QUEUE_LIST_END) /* queue empty? */ - continue; /* wait again */ - inst_per_sec = sim_timer_inst_per_sec (); + if (sim_wallclock_queue != QUEUE_LIST_END) { /* Got work? */ + inst_per_sec = sim_timer_inst_per_sec (); - uptr = sim_wallclock_queue; - sim_wallclock_queue = uptr->a_next; - uptr->a_next = NULL; /* hygiene */ + uptr = sim_wallclock_queue; + sim_wallclock_queue = uptr->a_next; + uptr->a_next = NULL; /* hygiene */ - clock_gettime(CLOCK_REALTIME, &stop_time); - if (1 != sim_timespec_compare (&due_time, &stop_time)) - inst_delay = 0; - else - inst_delay = (int32)(inst_per_sec*(_timespec_to_double(&due_time)-_timespec_to_double(&stop_time))); - sim_debug (DBG_TIM, &sim_timer_dev, "_timer_thread() - slept %.0fms - activating(%s,%d)\n", - 1000.0*(_timespec_to_double (&stop_time)-_timespec_to_double (&start_time)), sim_uname(uptr), inst_delay); - sim_activate (uptr, inst_delay); + clock_gettime(CLOCK_REALTIME, &stop_time); + if (1 != sim_timespec_compare (&due_time, &stop_time)) + inst_delay = 0; + else + inst_delay = (int32)(inst_per_sec*(_timespec_to_double(&due_time)-_timespec_to_double(&stop_time))); + sim_debug (DBG_TIM, &sim_timer_dev, "_timer_thread() - slept %.0fms - activating(%s,%d)\n", + 1000.0*(_timespec_to_double (&stop_time)-_timespec_to_double (&start_time)), sim_uname(uptr), inst_delay); + sim_activate (uptr, inst_delay); + } } else {/* Something wants to adjust the queue since the wait condition was signaled */ } + + /* Let go of the mutex. */ + pthread_mutex_unlock(&sim_timer_lock); } sim_timer_thread_running = FALSE; -pthread_mutex_unlock (&sim_timer_lock); - sim_debug (DBG_TIM, &sim_timer_dev, "_timer_thread() - exiting\n"); return NULL; @@ -2584,7 +2593,9 @@ if (sim_asynch_timer) { pthread_attr_setscope (&attr, PTHREAD_SCOPE_SYSTEM); pthread_create (&sim_timer_thread, &attr, _timer_thread, NULL); pthread_attr_destroy( &attr); + pthread_mutex_lock(&sim_timer_lock); pthread_cond_wait (&sim_timer_startup_cond, &sim_timer_lock); /* Wait for thread to stabilize */ + pthread_mutex_unlock(&sim_timer_lock); pthread_cond_destroy (&sim_timer_startup_cond); } pthread_mutex_unlock (&sim_timer_lock); @@ -2843,8 +2854,8 @@ if ((sim_asynch_timer) && } } sim_wallclock_entry = uptr; - pthread_mutex_unlock (&sim_timer_lock); pthread_cond_signal (&sim_timer_wake); /* wake the timer thread to deal with it */ + pthread_mutex_unlock (&sim_timer_lock); /* allow the time thread to wake up. */ return SCPE_OK; } else { /* inserting at prvptr */ diff --git a/sim_tmxr.c b/sim_tmxr.c index 2ae579148..c6f0ff90b 100644 --- a/sim_tmxr.c +++ b/sim_tmxr.c @@ -3755,6 +3755,7 @@ sockets = (SOCKET *)calloc(FD_SETSIZE, sizeof(*sockets)); timeout_usec = 1000000; pthread_mutex_lock (&sim_tmxr_poll_lock); pthread_cond_signal (&sim_tmxr_startup_cond); /* Signal we're ready to go */ +/* We still have sim_tmxr_poll_lock acquired entering the loop. */ while (sim_asynch_enabled) { int i, j, status, select_errno; fd_set readfds, errorfds; @@ -4175,6 +4176,7 @@ lines = (TMLN **)calloc(MAXIMUM_WAIT_OBJECTS, sizeof(*lines)); threads = (pthread_t *)calloc(MAXIMUM_WAIT_OBJECTS, sizeof(*threads)); pthread_mutex_lock (&sim_tmxr_poll_lock); pthread_cond_signal (&sim_tmxr_serial_startup_cond); /* Signal we're ready to go */ +pthread_mutex_unlock (&sim_tmxr_poll_lock); /* Let waiters wake up. */ pthread_cond_init (&sim_serial_line_startup_cond, NULL); while (sim_asynch_enabled) { pthread_attr_t attr; @@ -4192,8 +4194,10 @@ while (sim_asynch_enabled) { for (j=0; jlines; ++j) { if (mp->ldsc[j].serport) { lines[serport_count] = &mp->ldsc[j]; + pthread_mutex_lock (&sim_tmxr_poll_lock); pthread_create (&threads[serport_count], &attr, _tmxr_serial_line_poll, (void *)&mp->ldsc[j]); pthread_cond_wait (&sim_serial_line_startup_cond, &sim_tmxr_poll_lock); /* Wait for thread to stabilize */ + pthread_mutex_unlock (&sim_tmxr_poll_lock); ++serport_count; } } @@ -4201,12 +4205,10 @@ while (sim_asynch_enabled) { pthread_attr_destroy( &attr); if (serport_count == 0) /* No open serial ports? */ break; /* We're done */ - pthread_mutex_unlock (&sim_tmxr_poll_lock); + /* Wait for serial line threads to exit. */ for (i=0; i