diff --git a/src/hal/hal_lib.c b/src/hal/hal_lib.c index 0839355cf52..3ebd23658b2 100644 --- a/src/hal/hal_lib.c +++ b/src/hal/hal_lib.c @@ -162,6 +162,8 @@ static void free_oldname_struct(hal_oldname_t * oldname); static void free_funct_struct(hal_funct_t * funct); #endif /* RTAPI */ 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 thread_wait_quiescent(hal_thread_t * thread); #ifdef RTAPI static void free_thread_struct(hal_thread_t * thread); #endif /* RTAPI */ @@ -2642,7 +2644,18 @@ int hal_del_funct_from_thread(const char *funct_name, const char *thread_name) funct_entry = (hal_funct_entry_t *) list_entry; if (SHMPTR(funct_entry->funct_ptr) == funct) { /* this funct entry points to our funct, unlink */ - list_remove_entry(list_entry); + funct_entry_unlink(list_entry); + /* the realtime thread walks the list without the mutex; it may + be standing on this entry right now. Don't recycle the entry + (or let the caller unload the code) until the thread has + finished the pass that could still see it. */ + if (thread_wait_quiescent(thread) < 0) { + /* thread is stalled: leak the entry rather than hand the + thread a recycled one */ + funct->users--; + halpr_mutex_release(); + return -ETIMEDOUT; + } /* and delete it */ free_funct_entry_struct(funct_entry); /* done */ @@ -3253,7 +3266,11 @@ static void thread_task(void *arg) if ( runtime > hal_get_si32(thread->maxtime)) { hal_set_si32(thread->maxtime, runtime); } - hal_set_sint(thread->threadbeat, ++thread->beatcnt); + /* publish the completed pass; release orders every read of + funct_list above before the new count is visible (see + thread_wait_quiescent()) */ + __atomic_store_n(&thread->beatcnt, thread->beatcnt + 1, __ATOMIC_RELEASE); + hal_set_sint(thread->threadbeat, thread->beatcnt); } /* wait until next period */ rtapi_wait(); @@ -3850,7 +3867,13 @@ static void free_funct_struct(hal_funct_t * funct) /* test it */ if (SHMPTR(funct_entry->funct_ptr) == funct) { /* this funct entry points to our funct, unlink */ - list_entry = list_remove_entry(list_entry); + list_entry = funct_entry_unlink(list_entry); + /* let the thread leave it before it is recycled and the + code behind it is unloaded */ + if (thread_wait_quiescent(thread) < 0) { + /* stalled thread: leak the entry */ + continue; + } /* and delete it */ free_funct_entry_struct(funct_entry); } else { @@ -3890,6 +3913,66 @@ static void free_funct_struct(hal_funct_t * funct) } #endif /* RTAPI */ +/* Unlink a funct entry from a list the realtime thread may be walking. + Unlike list_remove_entry() the entry keeps its own links, so a thread + standing on it still reaches the rest of the list instead of looping on + the entry. The entry must not be reused before thread_wait_quiescent(). + Returns the next entry. */ +static hal_list_t *funct_entry_unlink(hal_list_t * entry) +{ + hal_list_t *prev, *next; + + prev = SHMPTR(entry->prev); + next = SHMPTR(entry->next); + prev->next = entry->next; + next->prev = entry->prev; + return next; +} + +/* Number of thread periods to wait for a running thread to finish the + pass that may still reference an unlinked funct entry. */ +#define HAL_QUIESCE_PERIODS 1000 + +/* 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 hold a pointer to the unlinked entry. Wait until the + thread completed two more passes (any pass that started before the + unlink has ended) before the entry may be freed. Returns 0 when it is + safe (or threads are not running), -ETIMEDOUT if the thread stalled. */ +static int thread_wait_quiescent(hal_thread_t * thread) +{ + rtapi_sint start, now; + long step; + long long waited, limit; + + if (hal_data->threads_running == 0) { + /* thread is not walking the list */ + return 0; + } + /* order the unlink stores before reading the pass counter */ + __atomic_thread_fence(__ATOMIC_SEQ_CST); + start = __atomic_load_n(&thread->beatcnt, __ATOMIC_ACQUIRE); + step = thread->period / 4; + if (step > rtapi_delay_max()) { + step = rtapi_delay_max(); + } + if (step < 1) { + step = 1; + } + limit = (long long)thread->period * HAL_QUIESCE_PERIODS; + for (waited = 0; waited < limit; waited += step) { + now = __atomic_load_n(&thread->beatcnt, __ATOMIC_ACQUIRE); + if (now - start >= 2) { + return 0; + } + rtapi_delay(step); + } + rtapi_print_msg(RTAPI_MSG_ERR, + "HAL: ERROR: thread '%s' did not complete a pass within %d periods," + " funct entry not freed\n", thread->name, HAL_QUIESCE_PERIODS); + return -ETIMEDOUT; +} + static void free_funct_entry_struct(hal_funct_entry_t * funct_entry) { hal_funct_t *funct; diff --git a/tests/hal-delf-live/README b/tests/hal-delf-live/README new file mode 100644 index 00000000000..79367e8d772 --- /dev/null +++ b/tests/hal-delf-live/README @@ -0,0 +1,14 @@ +delf / unloadrt of a function while its thread is running. + +The realtime thread walks its funct_list without the HAL mutex. Without +quiescence, hal_del_funct_from_thread() unlinks the entry (its links then +point to itself) and recycles it at once; a thread that is inside the +entry keeps calling it forever or follows a recycled link, and the +following unloadrt dlclose()s code the thread may still execute. The +result is a hung thread or a crashed rtapi_app. + +The test loads a component whose function busy-waits a fixed time, so the +thread spends a large share of each period on the entry being removed, and +repeats loadrt -> addf -> delf -> unloadrt DELF_ITER times (default 200) +with threads started. It passes when halrun finishes in time and the +thread still beats afterwards. diff --git a/tests/hal-delf-live/beat-check.sh b/tests/hal-delf-live/beat-check.sh new file mode 100755 index 00000000000..a420ec2312b --- /dev/null +++ b/tests/hal-delf-live/beat-check.sh @@ -0,0 +1,7 @@ +#!/bin/sh +# exit 0 when thread 't' is still executing passes +a=$(halcmd getp t.threadbeat) +sleep 0.1 +b=$(halcmd getp t.threadbeat) +echo "threadbeat $a -> $b" +[ "$b" -gt "$a" ] diff --git a/tests/hal-delf-live/checkresult b/tests/hal-delf-live/checkresult new file mode 100755 index 00000000000..82e7de12566 --- /dev/null +++ b/tests/hal-delf-live/checkresult @@ -0,0 +1,3 @@ +#!/bin/sh +# success or failure is determined by the return value of the test.sh script +exit 0 diff --git a/tests/hal-delf-live/control b/tests/hal-delf-live/control new file mode 100644 index 00000000000..5cc4825bdc1 --- /dev/null +++ b/tests/hal-delf-live/control @@ -0,0 +1 @@ +Restrictions: sudo diff --git a/tests/hal-delf-live/delfvictim.comp b/tests/hal-delf-live/delfvictim.comp new file mode 100644 index 00000000000..10afd128642 --- /dev/null +++ b/tests/hal-delf-live/delfvictim.comp @@ -0,0 +1,12 @@ +component delfvictim "Test component for tests/hal-delf-live: burns spin_ns in its function"; + +pin in bit dummy; +param rw uint spin_ns = 20000; + +function _; +license "GPL"; +author "SyncTwin"; +;; +long long t0 = rtapi_get_time(); +(void)period; +while (rtapi_get_time() - t0 < (long long)spin_ns) { } diff --git a/tests/hal-delf-live/test.sh b/tests/hal-delf-live/test.sh new file mode 100755 index 00000000000..47e7e9ea526 --- /dev/null +++ b/tests/hal-delf-live/test.sh @@ -0,0 +1,36 @@ +#!/bin/bash +# Repeatedly remove a function from a running thread and unload its +# component. Without thread quiescence in hal_del_funct_from_thread() this +# hangs or crashes rtapi_app (probabilistically, usually in the first tens +# of iterations). +set -e +${SUDO} halcompile --install delfvictim.comp + +N=${DELF_ITER:-200} +TMPDIR=$(mktemp -d /tmp/hal-delf-live.XXXXXX) +trap 'rm -rf "$TMPDIR"' 0 1 2 3 15 +HAL="$TMPDIR/loop.hal" + +{ + echo "loadrt threads name1=t period1=100000" + echo "start" + for _ in $(seq "$N"); do + echo "loadrt delfvictim names=v" + echo "addf v t" + echo "loadusr -w sleep 0.002" + echo "delf v t" + echo "unloadrt delfvictim" + done + # the thread must still be beating after the loop + echo "loadusr -w $(pwd)/beat-check.sh" +} > "$HAL" + +set +e +timeout -k 5 $((N / 2 + 60)) halrun -f "$HAL" +RES=$? +set -e +if [ $RES -ne 0 ]; then + echo "FAIL: halrun exited with $RES after delf/unloadrt in a running thread" + exit 1 +fi +echo "PASS: $N x loadrt/addf/delf/unloadrt with running thread"