Changeset 1871118 in mainline

Timestamp:

2023-02-10T22:59:11Z (22 months ago)

Author:

Jiří Zárevúcky <zarevucky.jiri@…>

Branches:

master, ticket/834-toolchain-update, topic/msim-upgrade, topic/simplify-dev-export

Children:

11d2c983

Parents:

daadfa6

git-author:

Jiří Zárevúcky <zarevucky.jiri@…> (2023-02-10 22:53:12)

git-committer:

Jiří Zárevúcky <zarevucky.jiri@…> (2023-02-10 22:59:11)

Message:

Make thread_t reference counted

This simplifies interaction between various locks and thread
lifespan, which simplifies things. For example, threads_lock can
now simply be a mutex protecting the global it was made for, and
nothing more.

Location:

kernel

Files:

• generic/include/lib/refcount.h (modified) (1 diff)
• generic/include/proc/thread.h (modified) (6 diffs)
• generic/include/synch/waitq.h (modified) (1 diff)
• generic/src/console/cmd.c (modified) (1 diff)
• generic/src/ipc/kbox.c (modified) (3 diffs)
• generic/src/main/kinit.c (modified) (2 diffs)
• generic/src/main/uinit.c (modified) (1 diff)
• generic/src/proc/program.c (modified) (1 diff)
• generic/src/proc/scheduler.c (modified) (3 diffs)
• generic/src/proc/task.c (modified) (3 diffs)
• generic/src/proc/thread.c (modified) (19 diffs)
• generic/src/synch/waitq.c (modified) (1 diff)
• generic/src/sysinfo/stats.c (modified) (4 diffs)
• generic/src/udebug/udebug_ops.c (modified) (3 diffs)
• test/mm/falloc2.c (modified) (1 diff)
• test/mm/slab1.c (modified) (1 diff)
• test/mm/slab2.c (modified) (1 diff)
• test/synch/semaphore1.c (modified) (2 diffs)
• test/synch/semaphore2.c (modified) (1 diff)
• test/thread/thread1.c (modified) (1 diff)

kernel/generic/include/lib/refcount.h

@@ -76 +76 @@
 }
 
+/**
+ * Try to upgrade a weak reference.
+ * Naturally, this is contingent on another form of synchronization being used
+ * to ensure that the object continues to exist while the weak reference is in
+ * use.
+ */
+static inline bool refcount_try_up(atomic_refcount_t *rc)
+{
+        int cnt = atomic_load_explicit(&rc->__cnt, memory_order_relaxed);
+
+        while (cnt >= 0) {
+                if (atomic_compare_exchange_weak_explicit(&rc->__cnt, &cnt, cnt + 1,
+                    memory_order_relaxed, memory_order_relaxed)) {
+                        return true;
+                }
+        }
+
+        return false;
+}
+
 static inline bool refcount_unique(atomic_refcount_t *rc)
 {

kernel/generic/include/proc/thread.h

@@ -70 +70 @@
 /** Thread structure. There is one per thread. */
 typedef struct thread {
+        atomic_refcount_t refcount;
+
         link_t rq_link;  /**< Run queue link. */
         link_t wq_link;  /**< Wait queue link. */
@@ -77 +79 @@
         odlink_t lthreads;
 
+        /**
+         * If true, the thread is terminating.
+         * It will not go to sleep in interruptible synchronization functions
+         * and will call thread_exit() before returning to userspace.
+         */
+        volatile bool interrupted;
+
+        /** Waitq for thread_join_timeout(). */
+        waitq_t join_wq;
+
         /** Lock protecting thread structure.
          *
-         * Protects the whole thread structure except list links above.
+         * Protects the whole thread structure except fields listed above.
          */
         IRQ_SPINLOCK_DECLARE(lock);
@@ -133 +145 @@
          */
         bool in_copy_to_uspace;
-
-        /**
-         * If true, the thread will not go to sleep at all and will call
-         * thread_exit() before returning to userspace.
-         */
-        bool interrupted;
-
-        /** If true, thread_join_timeout() cannot be used on this thread. */
-        bool detached;
-        /** Waitq for thread_join_timeout(). */
-        waitq_t join_wq;
-        /** Link used in the joiner_head list. */
-        link_t joiner_link;
 
 #ifdef CONFIG_FPU
@@ -219 +218 @@
 extern void thread_interrupt(thread_t *, bool);
 
+static inline thread_t *thread_ref(thread_t *thread)
+{
+        refcount_up(&thread->refcount);
+        return thread;
+}
+
+static inline thread_t *thread_try_ref(thread_t *thread)
+{
+        if (refcount_try_up(&thread->refcount))
+                return thread;
+        else
+                return NULL;
+}
+
+extern void thread_put(thread_t *);
+
 #ifndef thread_create_arch
 extern errno_t thread_create_arch(thread_t *, thread_flags_t);
@@ -236 +251 @@
 extern errno_t thread_join(thread_t *);
 extern errno_t thread_join_timeout(thread_t *, uint32_t, unsigned int);
-extern void thread_detach(thread_t *);
 
 extern void thread_print_list(bool);
-extern void thread_destroy(thread_t *, bool);
 extern thread_t *thread_find_by_id(thread_id_t);
 extern size_t thread_count(void);
@@ -245 +258 @@
 extern thread_t *thread_next(thread_t *);
 extern void thread_update_accounting(bool);
-extern bool thread_exists(thread_t *);
+extern thread_t *thread_try_get(thread_t *);
 
 extern void thread_migration_disable(void);

kernel/generic/include/synch/waitq.h

@@ -43 +43 @@
 typedef enum {
         WAKEUP_FIRST = 0,
-        WAKEUP_ALL
+        WAKEUP_ALL,
+        WAKEUP_CLOSE,
 } wakeup_mode_t;
 

kernel/generic/src/console/cmd.c

@@ -993 +993 @@
                         printf("cpu%u: ", i);
                         thread_wire(thread, &cpus[i]);
-                        thread_ready(thread);
+                        thread_ready(thread_ref(thread));
                         thread_join(thread);
-                        thread_detach(thread);
+                        thread_put(thread);
                 } else
                         printf("Unable to create thread for cpu%u\n", i);

kernel/generic/src/ipc/kbox.c

@@ -90 +90 @@
                 LOG("Join kb.thread.");
                 thread_join(TASK->kb.thread);
-                thread_detach(TASK->kb.thread);
+                thread_put(TASK->kb.thread);
                 LOG("...join done.");
                 TASK->kb.thread = NULL;
@@ -136 +136 @@
                 /* Only detach kbox thread unless already terminating. */
                 if (TASK->kb.finished == false) {
-                        /* Detach kbox thread so it gets freed from memory. */
-                        thread_detach(TASK->kb.thread);
+                        /* Release kbox thread so it gets freed from memory. */
+                        thread_put(TASK->kb.thread);
                         TASK->kb.thread = NULL;
                 }
@@ -247 +247 @@
                 }
 
-                task->kb.thread = kb_thread;
+                task->kb.thread = thread_ref(kb_thread);
                 thread_ready(kb_thread);
         }

kernel/generic/src/main/kinit.c

@@ -102 +102 @@
         thread_t *thread;
 
-        /*
-         * Detach kinit as nobody will call thread_join_timeout() on it.
-         */
-        thread_detach(THREAD);
-
         interrupts_disable();
 
@@ -125 +120 @@
 
                 thread_wire(thread, &cpus[0]);
-                thread_ready(thread);
+                thread_ready(thread_ref(thread));
                 thread_join(thread);
-                thread_detach(thread);
+                thread_put(thread);
 
                 /*

kernel/generic/src/main/uinit.c

@@ -56 +56 @@
 void uinit(void *arg)
 {
-        /*
-         * So far, we don't have a use for joining userspace threads so we
-         * immediately detach each uinit thread. If joining of userspace threads
-         * is required, some userspace API based on the kernel mechanism will
-         * have to be implemented. Moreover, garbage collecting of threads that
-         * didn't detach themselves and nobody else joined them will have to be
-         * deployed for the event of forceful task termination.
-         */
-        thread_detach(THREAD);
-
 #ifdef CONFIG_UDEBUG
         udebug_stoppable_end();

kernel/generic/src/proc/program.c

@@ -204 +204 @@
 {
         thread_ready(prg->main_thread);
+        prg->main_thread = NULL;
 }
 

kernel/generic/src/proc/scheduler.c

@@ -389 +389 @@
 void scheduler_separated_stack(void)
 {
-        DEADLOCK_PROBE_INIT(p_joinwq);
         task_t *old_task = TASK;
         as_t *old_as = AS;
@@ -419 +418 @@
 
                 case Exiting:
-                repeat:
-                        if (THREAD->detached) {
-                                thread_destroy(THREAD, false);
-                        } else {
-                                /*
-                                 * The thread structure is kept allocated until
-                                 * somebody calls thread_detach() on it.
-                                 */
-                                if (!irq_spinlock_trylock(&THREAD->join_wq.lock)) {
-                                        /*
-                                         * Avoid deadlock.
-                                         */
-                                        irq_spinlock_unlock(&THREAD->lock, false);
-                                        delay(HZ);
-                                        irq_spinlock_lock(&THREAD->lock, false);
-                                        DEADLOCK_PROBE(p_joinwq,
-                                            DEADLOCK_THRESHOLD);
-                                        goto repeat;
-                                }
-                                _waitq_wakeup_unsafe(&THREAD->join_wq,
-                                    WAKEUP_FIRST);
-                                irq_spinlock_unlock(&THREAD->join_wq.lock, false);
-
-                                THREAD->state = Lingering;
-                                irq_spinlock_unlock(&THREAD->lock, false);
-                        }
+                        irq_spinlock_unlock(&THREAD->lock, false);
+                        waitq_wakeup(&THREAD->join_wq, WAKEUP_CLOSE);
+
+                        /*
+                         * Release the reference CPU has for the thread.
+                         * If there are no other references (e.g. threads calling join),
+                         * the thread structure is deallocated.
+                         */
+                        thread_put(THREAD);
                         break;
 
@@ -556 +538 @@
         size_t average;
         size_t rdy;
-
-        /*
-         * Detach kcpulb as nobody will call thread_join_timeout() on it.
-         */
-        thread_detach(THREAD);
 
 loop:

kernel/generic/src/proc/task.c

@@ -385 +385 @@
         irq_spinlock_lock(&tasks_lock, true);
         irq_spinlock_lock(&TASK->lock, false);
-        irq_spinlock_lock(&threads_lock, false);
 
         /* Set task name */
         str_cpy(TASK->name, TASK_NAME_BUFLEN, namebuf);
 
-        irq_spinlock_unlock(&threads_lock, false);
         irq_spinlock_unlock(&TASK->lock, false);
         irq_spinlock_unlock(&tasks_lock, true);
@@ -529 +527 @@
 {
         irq_spinlock_lock(&task->lock, false);
-        irq_spinlock_lock(&threads_lock, false);
 
         /*
@@ -536 +533 @@
 
         list_foreach(task->threads, th_link, thread_t, thread) {
-                thread_interrupt(thread, false);
+                thread_t *thr = thread_try_ref(thread);
+                if (thr)
+                        thread_interrupt(thr, false);
+
+                // If NULL, the thread is already getting destroyed concurrently with this.
         }
 
-        irq_spinlock_unlock(&threads_lock, false);
         irq_spinlock_unlock(&task->lock, false);
 }

kernel/generic/src/proc/thread.c

@@ -94 +94 @@
  *
  * Members are of type thread_t.
+ *
+ * This structure contains weak references. Any reference from it must not leave
+ * threads_lock critical section unless strengthened via thread_try_ref().
  */
 odict_t threads;
@@ -249 +252 @@
 /** Make thread ready
  *
- * Switch thread to the ready state.
+ * Switch thread to the ready state. Consumes reference passed by the caller.
  *
  * @param thread Thread to make ready.
@@ -320 +323 @@
                 return NULL;
 
+        refcount_init(&thread->refcount);
+
         if (thread_create_arch(thread, flags) != EOK) {
                 slab_free(thread_cache, thread);
@@ -368 +373 @@
 
         thread->interrupted = false;
-        thread->detached = false;
         waitq_initialize(&thread->join_wq);
 
@@ -399 +403 @@
  *
  */
-void thread_destroy(thread_t *thread, bool irq_res)
-{
-        assert(irq_spinlock_locked(&thread->lock));
+static void thread_destroy(void *obj)
+{
+        thread_t *thread = (thread_t *) obj;
+
+        irq_spinlock_lock(&thread->lock, true);
         assert((thread->state == Exiting) || (thread->state == Lingering));
         assert(thread->task);
@@ -421 +427 @@
          */
         list_remove(&thread->th_link);
-        irq_spinlock_unlock(&thread->task->lock, irq_res);
+        irq_spinlock_unlock(&thread->task->lock, true);
 
         /*
@@ -430 +436 @@
 }
 
+void thread_put(thread_t *thread)
+{
+        if (refcount_down(&thread->refcount)) {
+                thread_destroy(thread);
+        }
+}
+
 /** Make the thread visible to the system.
  *
@@ -441 +454 @@
 void thread_attach(thread_t *thread, task_t *task)
 {
+        ipl_t ipl = interrupts_disable();
+
         /*
          * Attach to the specified task.
          */
-        irq_spinlock_lock(&task->lock, true);
+        irq_spinlock_lock(&task->lock, false);
 
         /* Hold a reference to the task. */
@@ -455 +470 @@
         list_append(&thread->th_link, &task->threads);
 
-        irq_spinlock_pass(&task->lock, &threads_lock);
+        irq_spinlock_unlock(&task->lock, false);
 
         /*
          * Register this thread in the system-wide dictionary.
          */
+        irq_spinlock_lock(&threads_lock, false);
         odict_insert(&thread->lthreads, &threads, NULL);
-        irq_spinlock_unlock(&threads_lock, true);
+        irq_spinlock_unlock(&threads_lock, false);
+
+        interrupts_restore(ipl);
 }
 
@@ -513 +531 @@
  * blocking call was interruptable. See waitq_sleep_timeout().
  *
- * The caller must guarantee the thread object is valid during the entire
- * function, eg by holding the threads_lock lock.
- *
  * Interrupted threads automatically exit when returning back to user space.
  *
- * @param thread A valid thread object. The caller must guarantee it
- *               will remain valid until thread_interrupt() exits.
+ * @param thread A valid thread object.
  */
 void thread_interrupt(thread_t *thread, bool irq_dis)
@@ -534 +548 @@
         if (sleeping)
                 waitq_interrupt_sleep(thread);
+
+        thread_put(thread);
 }
 
@@ -581 +597 @@
 
 /** Wait for another thread to exit.
+ * This function does not destroy the thread. Reference counting handles that.
  *
  * @param thread Thread to join on exit.
@@ -594 +611 @@
                 return EINVAL;
 
-        /*
-         * Since thread join can only be called once on an undetached thread,
-         * the thread pointer is guaranteed to be still valid.
-         */
-
         irq_spinlock_lock(&thread->lock, true);
-        assert(!thread->detached);
+        state_t state = thread->state;
         irq_spinlock_unlock(&thread->lock, true);
 
-        return waitq_sleep_timeout(&thread->join_wq, usec, flags, NULL);
-
-        // FIXME: join should deallocate the thread.
-        //        Current code calls detach after join, that's contrary to how
-        //        join is used in other threading APIs.
-}
-
-/** Detach thread.
- *
- * Mark the thread as detached. If the thread is already
- * in the Lingering state, deallocate its resources.
- *
- * @param thread Thread to be detached.
- *
- */
-void thread_detach(thread_t *thread)
-{
-        /*
-         * Since the thread is expected not to be already detached,
-         * pointer to it must be still valid.
-         */
-        irq_spinlock_lock(&thread->lock, true);
-        assert(!thread->detached);
-
-        if (thread->state == Lingering) {
-                /*
-                 * Unlock &thread->lock and restore
-                 * interrupts in thread_destroy().
-                 */
-                thread_destroy(thread, true);
-                return;
+        if (state == Exiting) {
+                return EOK;
         } else {
-                thread->detached = true;
-        }
-
-        irq_spinlock_unlock(&thread->lock, true);
+                return waitq_sleep_timeout(&thread->join_wq, usec,
+                    SYNCH_FLAGS_NON_BLOCKING, NULL);
+        }
 }
 
@@ -702 +684 @@
         thread_t *thread;
 
-        /* Messing with thread structures, avoid deadlock */
+        /* Accessing system-wide threads list through thread_first()/thread_next(). */
         irq_spinlock_lock(&threads_lock, true);
 
@@ -730 +712 @@
 }
 
-/** Check whether thread exists.
- *
- * Note that threads_lock must be already held and
- * interrupts must be already disabled.
- *
- * @param thread Pointer to thread.
- *
- * @return True if thread t is known to the system, false otherwise.
- *
- */
-bool thread_exists(thread_t *thread)
-{
-        assert(interrupts_disabled());
-        assert(irq_spinlock_locked(&threads_lock));
-
+static bool thread_exists(thread_t *thread)
+{
         odlink_t *odlink = odict_find_eq(&threads, thread, NULL);
         return odlink != NULL;
+}
+
+/** Check whether the thread exists, and if so, return a reference to it.
+ */
+thread_t *thread_try_get(thread_t *thread)
+{
+        irq_spinlock_lock(&threads_lock, true);
+
+        if (thread_exists(thread)) {
+                /* Try to strengthen the reference. */
+                thread = thread_try_ref(thread);
+        } else {
+                thread = NULL;
+        }
+
+        irq_spinlock_unlock(&threads_lock, true);
+
+        return thread;
 }
 
@@ -777 +764 @@
  * interrupts must be disabled.
  *
+ * The returned reference is weak.
+ * If the caller needs to keep it, thread_try_ref() must be used to upgrade
+ * to a strong reference _before_ threads_lock is released.
+ *
  * @param id Thread ID.
  *
@@ -854 +845 @@
 {
         irq_spinlock_lock(&threads_lock, true);
-
-        thread_t *thread = thread_find_by_id(thread_id);
+        thread_t *thread = thread_try_ref(thread_find_by_id(thread_id));
+        irq_spinlock_unlock(&threads_lock, true);
+
         if (thread == NULL) {
                 printf("No such thread.\n");
-                irq_spinlock_unlock(&threads_lock, true);
                 return;
         }
-
-        irq_spinlock_lock(&thread->lock, false);
 
         /*
@@ -876 +865 @@
          */
 
+        irq_spinlock_lock(&thread->lock, true);
+
         bool sleeping = false;
         istate_t *istate = thread->udebug.uspace_state;
@@ -886 +877 @@
                 printf("Thread interrupt state not available.\n");
 
-        irq_spinlock_unlock(&thread->lock, false);
+        irq_spinlock_unlock(&thread->lock, true);
 
         if (sleeping)
                 waitq_interrupt_sleep(thread);
 
-        irq_spinlock_unlock(&threads_lock, true);
+        thread_put(thread);
 }
 

kernel/generic/src/synch/waitq.c

@@ -513 +513 @@
 loop:
         if (list_empty(&wq->sleepers)) {
-                if (mode != WAKEUP_ALL) {
+                if (mode == WAKEUP_CLOSE) {
+                        // FIXME: this can technically fail if we get two billion sleeps after the wakeup call.
+                        wq->missed_wakeups = INT_MAX;
+                } else if (mode != WAKEUP_ALL) {
                         wq->missed_wakeups++;
                 }

kernel/generic/src/sysinfo/stats.c

@@ -332 +332 @@
     bool dry_run, void *data)
 {
-        /* Messing with threads structures, avoid deadlock */
+        /* Messing with threads structures */
         irq_spinlock_lock(&threads_lock, true);
 
@@ -597 +597 @@
                 return ret;
 
-        /* Messing with threads structures, avoid deadlock */
+        /* Messing with threads structures */
         irq_spinlock_lock(&threads_lock, true);
 
@@ -627 +627 @@
                 ret.data.size = sizeof(stats_thread_t);
 
-                /* Hand-over-hand locking */
-                irq_spinlock_exchange(&threads_lock, &thread->lock);
-
+                /*
+                 * Replaced hand-over-hand locking with regular nested sections
+                 * to avoid weak reference leak issues.
+                 */
+                irq_spinlock_lock(&thread->lock, false);
                 produce_stats_thread(thread, stats_thread);
-
-                irq_spinlock_unlock(&thread->lock, true);
+                irq_spinlock_unlock(&thread->lock, false);
+
+                irq_spinlock_unlock(&threads_lock, true);
         }
 
@@ -847 +850 @@
 void kload(void *arg)
 {
-        thread_detach(THREAD);
-
         while (true) {
                 size_t ready = atomic_load(&nrdy);

kernel/generic/src/udebug/udebug_ops.c

@@ -83 +83 @@
         mutex_lock(&TASK->udebug.lock);
 
-        /* thread_exists() must be called with threads_lock held */
-        irq_spinlock_lock(&threads_lock, true);
-
-        if (!thread_exists(thread)) {
-                irq_spinlock_unlock(&threads_lock, true);
+        thread = thread_try_get(thread);
+
+        if (!thread) {
                 mutex_unlock(&TASK->udebug.lock);
                 return ENOENT;
         }
 
-        /* thread->lock is enough to ensure the thread's existence */
-        irq_spinlock_exchange(&threads_lock, &thread->lock);
+        irq_spinlock_lock(&thread->lock, true);
 
         /* Verify that 'thread' is a userspace thread. */
@@ -111 +108 @@
         }
 
-        /*
-         * Since the thread has active == true, TASK->udebug.lock
-         * is enough to ensure its existence and that active remains
-         * true.
-         *
-         */
-        irq_spinlock_unlock(&thread->lock, true);
-
-        /* Only mutex TASK->udebug.lock left. */
-
         /* Now verify that the thread belongs to the current task. */
         if (thread->task != TASK) {
                 /* No such thread belonging this task */
+                irq_spinlock_unlock(&thread->lock, true);
                 mutex_unlock(&TASK->udebug.lock);
                 return ENOENT;
         }
+
+        irq_spinlock_unlock(&thread->lock, true);
+
+        /* Only mutex TASK->udebug.lock left. */
 
         /*
@@ -153 +145 @@
 {
         mutex_unlock(&thread->udebug.lock);
+
+        /* Drop reference from _thread_op_begin() */
+        thread_put(thread);
 }
 

kernel/test/mm/falloc2.c

@@ -59 +59 @@
                 return;
         }
-
-        thread_detach(THREAD);
 
         for (unsigned int run = 0; run < THREAD_RUNS; run++) {

kernel/test/mm/slab1.c

@@ -128 +128 @@
         int i, j;
 
-        thread_detach(THREAD);
-
         TPRINTF("Starting thread #%" PRIu64 "...\n", THREAD->tid);
 

kernel/test/mm/slab2.c

@@ -137 +137 @@
         void *data = NULL, *new;
 
-        thread_detach(THREAD);
-
         mutex_lock(&starter_mutex);
         condvar_wait(&thread_starter, &starter_mutex);

kernel/test/synch/semaphore1.c

@@ -46 +46 @@
 static void producer(void *arg)
 {
-        thread_detach(THREAD);
-
         waitq_sleep(&can_start);
 
@@ -58 +56 @@
 static void consumer(void *arg)
 {
-        thread_detach(THREAD);
-
         waitq_sleep(&can_start);
 

kernel/test/synch/semaphore2.c

@@ -61 +61 @@
         int to;
 
-        thread_detach(THREAD);
-
         waitq_sleep(&can_start);
 

kernel/test/thread/thread1.c

@@ -42 +42 @@
 static void threadtest(void *data)
 {
-        thread_detach(THREAD);
-
         while (atomic_load(&finish)) {
                 TPRINTF("%" PRIu64 " ", THREAD->tid);