From cc15b0df7f0700cd5fd5158cf9f882e4ca34b628 Mon Sep 17 00:00:00 2001 From: Luca Toniolo <10792599+grandixximo@users.noreply.github.com> Date: Sat, 17 Oct 2026 15:40:13 +1100 Subject: [PATCH] hal: stop returns once no thread runs a function hal_stop_threads() only cleared threads_running. A thread in the middle of its pass went on running the rest of its functions, so a halcmd stop followed by unloadrt, which is how linuxcnc shuts down, could unload a component while the thread was still executing its function, and rtapi_app crashed in thread_task(). Hannes Diethelm found it in tests whose functions keep the servo thread busy for most of the period (#4669). Each thread now marks itself busy before it reads threads_running and clears the mark after its pass. In a user process hal_stop_threads() clears threads_running, then waits until no thread is busy, at most 1000 of the thread's periods; a stuck thread makes it fail with -ETIMEDOUT. With both flags sequentially consistent atomics, either the thread sees the stop or the stop sees the pass. Called from realtime code it does not wait, as before. delf with the threads stopped trusted threads_running alone. It now also waits until the thread has finished its pass, which matters for a stop from realtime code and for a stop that runs while delf waits. tests/hal-delf-live holds the thread inside a function across a stop; the stop must not return before the thread has left it. --- docs/src/man/man1/halcmd.1.adoc | 4 +- docs/src/man/man3/hal_start_threads.3.adoc | 3 + src/hal/hal_lib.c | 99 ++++++++++++++++++---- src/hal/hal_priv.h | 6 +- tests/hal-delf-live/README | 7 +- tests/hal-delf-live/delftest.py | 14 ++- 6 files changed, 110 insertions(+), 23 deletions(-) diff --git a/docs/src/man/man1/halcmd.1.adoc b/docs/src/man/man1/halcmd.1.adoc index 7b527b87066..82a8b2369f5 100644 --- a/docs/src/man/man1/halcmd.1.adoc +++ b/docs/src/man/man1/halcmd.1.adoc @@ -203,7 +203,9 @@ _pinname_ *=* _value_:: the order in which they were added. *stop*:: Stops execution of realtime threads. The threads will no longer call - their functions. + their functions. *stop* returns once every thread has finished the + functions it was running, so they can be unloaded. It fails if a + thread has not finished them within 1000 of its periods. *show* [_item_]:: Prints HAL items to _stdout_ in human readable format. _item_ can be one of "*comp*" (components), "*pin*", "*sig*" (signals), "*param*" diff --git a/docs/src/man/man3/hal_start_threads.3.adoc b/docs/src/man/man3/hal_start_threads.3.adoc index c13ecab07a4..e150fb3ce0c 100644 --- a/docs/src/man/man3/hal_start_threads.3.adoc +++ b/docs/src/man/man3/hal_start_threads.3.adoc @@ -23,10 +23,13 @@ This is the point at which realtime functions start being called. The *hal_stop_threads()* function stops all threads that were previously started by *hal_start_threads()*. It should be called before any component that is part of a system exits. +Called from a non-realtime process, it returns once every thread has finished the functions it was running, so their code can be unloaded. +Called from realtime code, it does not wait. == RETURN VALUE Returns a HAL status code. +*hal_stop_threads()* returns -ETIMEDOUT when a thread has not finished its functions within 1000 of its periods. == SEE ALSO diff --git a/src/hal/hal_lib.c b/src/hal/hal_lib.c index 227cf6ad629..d5d84107e8c 100644 --- a/src/hal/hal_lib.c +++ b/src/hal/hal_lib.c @@ -165,6 +165,9 @@ static void free_funct_entry_struct(hal_funct_entry_t * funct_entry); static hal_list_t *funct_entry_unlink(hal_list_t * entry); static int funct_entry_release(hal_thread_t * thread, hal_funct_entry_t * funct_entry); +#ifdef ULAPI +static int threads_wait_idle(void); +#endif /* ULAPI */ #ifdef RTAPI static void free_thread_struct(hal_thread_t * thread); #endif /* RTAPI */ @@ -2776,6 +2779,12 @@ int hal_stop_threads(void) } hal_data->threads_running = 0; +#ifdef ULAPI + /* a thread may still be in the pass it started before */ + if (threads_wait_idle() != 0) { + return -ETIMEDOUT; + } +#endif /* ULAPI */ rtapi_print_msg(RTAPI_MSG_DBG, "HAL: threads stopped\n"); return 0; } @@ -3275,6 +3284,11 @@ static void thread_task(void *arg) thread = arg; while (1) { + /* busy is set before threads_running is read, and + hal_stop_threads() clears threads_running before it reads + busy: either this pass sees the stop or the stop sees the + pass and waits for it */ + atomic_store_explicit(&thread->busy, 1, memory_order_seq_cst); if (hal_data->threads_running > 0 && !thread->init_done) { /* special init cycle: run init_funct_list once with no timing measurement, re-anchor the period so the long @@ -3343,6 +3357,8 @@ static void thread_task(void *arg) } hal_set_sint(thread->threadbeat, ++thread->beatcnt); } + /* the functions are no longer used until the next pass */ + atomic_store_explicit(&thread->busy, 0, memory_order_release); /* wait until next period */ rtapi_wait(); } @@ -3708,6 +3724,7 @@ static hal_thread_t *alloc_thread_struct(void) list_init_entry(&(p->funct_list)); list_init_entry(&(p->init_funct_list)); p->init_done = 0; + atomic_init(&p->busy, 0); p->name[0] = '\0'; } return p; @@ -4002,11 +4019,54 @@ static hal_list_t *funct_entry_unlink(hal_list_t * entry) starved; delf then fails instead of waiting forever. */ #define QUIESCENT_TIMEOUT_PERIODS 1000 +/* Wait until 'thread' is between two passes. With the threads stopped + it then does not start another one. Returns 0, or -ETIMEDOUT. */ +static int thread_wait_idle(hal_thread_t * thread) +{ + int n; + + for (n = 0; atomic_load_explicit(&thread->busy, memory_order_seq_cst); n++) { + if (n == QUIESCENT_TIMEOUT_PERIODS) { + return -ETIMEDOUT; + } + rtapi_delay(thread->period); + } + return 0; +} + +/* Called by hal_stop_threads() after it cleared threads_running. A + thread may still be in the pass it started before. Wait until every + thread has finished it, so that once the threads are stopped no + function runs and functions can be removed and unloaded. Returns 0, + or -ETIMEDOUT. */ +static int threads_wait_idle(void) +{ + int next; + hal_thread_t *thread; + int retval = 0; + + halpr_mutex_acquire(); + next = hal_data->thread_list_ptr; + while (next != 0) { + thread = SHMPTR(next); + if (thread_wait_idle(thread) != 0) { + rtapi_print_msg(RTAPI_MSG_ERR, + "HAL: ERROR: thread '%s' did not finish its functions within" + " %d periods\n", thread->name, QUIESCENT_TIMEOUT_PERIODS); + retval = -ETIMEDOUT; + } + next = thread->next_ptr; + } + halpr_mutex_release(); + return retval; +} + /* Called with the HAL mutex held, after a funct entry was unlinked from 'thread'. The realtime thread walks its funct_list without the mutex, so it may still be on the unlinked entry. Wait until the thread's threadbeat advanced by two (the pass that could see the entry has - ended) or the threads are stopped. Returns 0, or -ETIMEDOUT. + ended) or, once the threads are stopped, until the thread has + finished its pass. Returns 0, or -ETIMEDOUT. thread->threadbeat is only valid in the context that created the thread (pin addressing depends on the context), so the pin is looked @@ -4018,6 +4078,9 @@ static int thread_wait_quiescent(hal_thread_t * thread) rtapi_sint start; int n, rv; + if (hal_data->threads_running == 0) { + return thread_wait_idle(thread); + } rtapi_snprintf(name, sizeof(name), "%s.threadbeat", thread->name); q.name = name; q.qtype = HAL_QTYPE_PIN; @@ -4035,43 +4098,47 @@ static int thread_wait_quiescent(hal_thread_t * thread) start = hal_get_sint(q.pp.ref.s); for (n = 0; n < QUIESCENT_TIMEOUT_PERIODS; n++) { rtapi_delay(thread->period); - if (hal_data->threads_running == 0 - || hal_get_sint(q.pp.ref.s) - start >= 2) { + if (hal_get_sint(q.pp.ref.s) - start >= 2) { return 0; } + if (hal_data->threads_running == 0) { + return thread_wait_idle(thread); + } } return -ETIMEDOUT; } #endif /* ULAPI */ /* Free a funct entry that was unlinked from 'thread' with - funct_entry_unlink(). While the threads run, the thread may still be - on the entry. delf (ULAPI) waits until it has left. Code that runs in + funct_entry_unlink(). The thread may still be on the entry, while + the threads run or, after they were stopped, until it has finished + its pass. delf (ULAPI) waits until it has left. Code that runs in RTAPI (a component's exit, unloadrt) must not wait; there the entry - is not recycled and an error is reported, the threads should be - stopped or the function removed with delf before unloading. */ + is not recycled and an error is reported if the threads run, they + should be stopped or the function removed with delf before + unloading. */ static int funct_entry_release(hal_thread_t * thread, hal_funct_entry_t * funct_entry) { hal_funct_t *funct = SHMPTR(funct_entry->funct_ptr); - if (hal_data->threads_running > 0) { #ifdef ULAPI - if (thread_wait_quiescent(thread) != 0) { - rtapi_print_msg(RTAPI_MSG_ERR, - "HAL: ERROR: thread '%s' did not leave function '%s' within" - " %d periods; the function must not be unloaded\n", - thread->name, funct->name, QUIESCENT_TIMEOUT_PERIODS); - return -ETIMEDOUT; - } + if (thread_wait_quiescent(thread) != 0) { + rtapi_print_msg(RTAPI_MSG_ERR, + "HAL: ERROR: thread '%s' did not leave function '%s' within" + " %d periods; the function must not be unloaded\n", + thread->name, funct->name, QUIESCENT_TIMEOUT_PERIODS); + return -ETIMEDOUT; + } #else + if (hal_data->threads_running > 0) { rtapi_print_msg(RTAPI_MSG_ERR, "HAL: ERROR: function '%s' removed from running thread '%s';" " stop the threads or delf the function before unloading\n", funct->name, thread->name); return -EBUSY; -#endif } +#endif free_funct_entry_struct(funct_entry); return 0; } diff --git a/src/hal/hal_priv.h b/src/hal/hal_priv.h index 8692aae9463..78247c78699 100644 --- a/src/hal/hal_priv.h +++ b/src/hal/hal_priv.h @@ -100,6 +100,7 @@ #include #include +#include /* IMPORTANT: If any of the structures in this file are changed, the version code (HAL_VER) must be incremented, to ensure that @@ -124,7 +125,7 @@ */ #define HAL_KEY 0x48414C32 /* key used to open HAL shared memory */ -#define HAL_VER 0x00000015 /* version code */ +#define HAL_VER 0x00000016 /* version code */ #define HAL_SIZE (2*256*4096) /* These pointers are set by hal_init() to point to the shmem block @@ -286,7 +287,7 @@ typedef struct hal_data_t { SHMFIELD(hal_funct_t) funct_list_ptr; // root of linked list of functions SHMFIELD(hal_thread_t) thread_list_ptr; // root of linked list of threads long base_period; // timer period for realtime tasks - int threads_running; // non-zero if threads are started + rtapi_atomic_int threads_running; // non-zero if threads are started SHMFIELD(hal_oldname_t) oldname_free_ptr; // list of free oldname structs SHMFIELD(hal_comp_t) comp_free_ptr; // list of free component structs SHMFIELD(hal_pin_t) pin_free_ptr; // list of free pin structs @@ -430,6 +431,7 @@ struct hal_thread_t { hal_list_t funct_list; // list of functions to run hal_list_t init_funct_list; // list of init functions, run once before first cyclic cycle int init_done; // 0 = init pending, 1 = init cycle has executed + rtapi_atomic_int busy; // non-zero while the thread may run functions char name[HAL_NAME_LEN + 1]; // thread name int comp_id; }; diff --git a/tests/hal-delf-live/README b/tests/hal-delf-live/README index 281b5465749..9e1e8c3f473 100644 --- a/tests/hal-delf-live/README +++ b/tests/hal-delf-live/README @@ -26,7 +26,10 @@ waits there in rtapi_delay(), which sleeps in uspace, so delftest.py and halcmd also run on the CPU of the thread. When delf returns, pin inside must be false; after unloadrt t.threadbeat must keep advancing. Then a second instance is held inside its function and -unloadrt is run without delf; the thread must keep running. Last, the -usual stop, unloadrt, start sequence is checked. Pins are +unloadrt is run without delf; the thread must keep running. Last, a +third instance is held inside its function and the usual stop, +unloadrt, start sequence is run: when stop returns, pin inside must be +false, since with the threads stopped unloadrt removes the function at +once and unloads its code. Pins are accessed with hal.set_p() and hal.get_p(); there are no nets. A wait that makes no progress for 10 s aborts the test. diff --git a/tests/hal-delf-live/delftest.py b/tests/hal-delf-live/delftest.py index 681f598268e..df1d677ab7d 100755 --- a/tests/hal-delf-live/delftest.py +++ b/tests/hal-delf-live/delftest.py @@ -2,7 +2,9 @@ # Hold the realtime thread inside a function and delf the function from # another process. delf must not return before the thread has left the # function. unloadrt of a component whose function is in the running -# thread removes the function the same way first; it works after stop too. +# thread removes the function the same way first. stop must not return +# before the thread has left the function either; unloadrt after stop +# then works too. # # Every wait is on a HAL state. A wait that takes more than 10 s of wall # time means the machine is dying; the test then aborts. @@ -62,11 +64,19 @@ def wait_beat(n): wait_beat(2) print("ok: unloadrt waits for the thread to leave the function") -# usual path: stop, then unload with the function still in the thread +# usual path: stop, then unload with the function still in the thread. +# stop must not return while the thread is inside the function: with +# the threads stopped, unloadrt removes the function at once and +# unloads its code halcmd("loadrt", "delfvictim", "names=w") halcmd("addf", "w", "t") wait_beat(2) +hal.set_p("w.hold", True) +wait(lambda: hal.get_p("w.inside")) halcmd("stop") +if hal.get_p("w.inside"): + sys.exit("FAIL: stop returned while the thread was inside w") +print("ok: stop waits for the thread to leave the function") halcmd("unloadrt", "delfvictim") halcmd("start") wait_beat(2)