hal: stop returns once no thread runs a function

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*"

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
 

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;
 }

src/hal/hal_priv.h

@@ -100,6 +100,7 @@
 
 #include <rtapi.h>
 #include <rtapi_mutex.h>
+#include <rtapi_atomic.h>
 
 /* 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;
 };

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.

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)