Changeset f3dbe27 in mainline

Timestamp:

2023-04-18T17:33:02Z (18 months ago)

Author:

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

Branches:

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

Children:

117ad5a2

Parents:

06f81c4

git-author:

Jiří Zárevúcky <zarevucky.jiri@…> (2023-04-18 17:27:32)

git-committer:

Jiří Zárevúcky <zarevucky.jiri@…> (2023-04-18 17:33:02)

Message:

Reduce locking further with lazy FPU

It turns out we only need a lock to synchronize between the trap
handler and thread destructor. The atomic operations introduced
are just plain reads and writes, written in an ugly fashion to
appease C11 undefined behavior gods.

In principle we could get rid of that if we made cpu_t::fpu_owner
a strong reference, but that would mean a thread structure could
be held in limbo indefinitely if a new thread is not being
scheduled or doesn't use FPU.

Location:

kernel

Files:

• arch/amd64/src/cpu/cpu.c (modified) (1 diff)
• arch/ia32/src/cpu/cpu.c (modified) (1 diff)
• generic/include/cpu.h (modified) (1 diff)
• generic/src/proc/scheduler.c (modified) (7 diffs)
• generic/src/proc/thread.c (modified) (1 diff)

kernel/arch/amd64/src/cpu/cpu.c

@@ -99 +99 @@
         CPU->arch.tss->iomap_base = &CPU->arch.tss->iomap[0] -
             ((uint8_t *) CPU->arch.tss);
-        CPU->fpu_owner = NULL;
 }
 

kernel/arch/ia32/src/cpu/cpu.c

@@ -87 +87 @@
         CPU->arch.tss->iomap_base = &CPU->arch.tss->iomap[0] - ((uint8_t *) CPU->arch.tss);
 
-        CPU->fpu_owner = NULL;
-
         cpuid(INTEL_CPUID_STANDARD, &info);
 

kernel/generic/include/cpu.h

@@ -98 +98 @@
 
 #ifdef CONFIG_FPU_LAZY
+        /* For synchronization between FPU trap and thread destructor. */
         IRQ_SPINLOCK_DECLARE(fpu_lock);
-        struct thread *fpu_owner;
 #endif
+        _Atomic(struct thread *) fpu_owner;
 
         /**

kernel/generic/src/proc/scheduler.c

@@ -84 +84 @@
 
 #ifdef CONFIG_FPU_LAZY
-        irq_spinlock_lock(&CPU->fpu_lock, true);
-
-        if (THREAD == CPU->fpu_owner)
+        /*
+         * The only concurrent modification possible for fpu_owner here is
+         * another thread changing it from itself to NULL in its destructor.
+         */
+        thread_t *owner = atomic_load_explicit(&CPU->fpu_owner,
+            memory_order_relaxed);
+
+        if (THREAD == owner)
                 fpu_enable();
         else
                 fpu_disable();
-
-        irq_spinlock_unlock(&CPU->fpu_lock, true);
 #elif defined CONFIG_FPU
         fpu_enable();
@@ -133 +136 @@
 {
         fpu_enable();
+
+        /* We need this lock to ensure synchronization with thread destructor. */
         irq_spinlock_lock(&CPU->fpu_lock, false);
 
         /* Save old context */
-        if (CPU->fpu_owner != NULL) {
-                fpu_context_save(&CPU->fpu_owner->fpu_context);
-                CPU->fpu_owner = NULL;
-        }
+        thread_t *owner = atomic_load_explicit(&CPU->fpu_owner, memory_order_relaxed);
+        if (owner != NULL) {
+                fpu_context_save(&owner->fpu_context);
+                atomic_store_explicit(&CPU->fpu_owner, NULL, memory_order_relaxed);
+        }
+
+        irq_spinlock_unlock(&CPU->fpu_lock, false);
 
         if (THREAD->fpu_context_exists) {
@@ -148 +156 @@
         }
 
-        CPU->fpu_owner = THREAD;
-
-        irq_spinlock_unlock(&CPU->fpu_lock, false);
+        atomic_store_explicit(&CPU->fpu_owner, THREAD, memory_order_relaxed);
 }
 #endif /* CONFIG_FPU_LAZY */
@@ -499 +505 @@
 #ifdef CONFIG_SMP
 
-static inline void fpu_owner_lock(cpu_t *cpu)
-{
-#ifdef CONFIG_FPU_LAZY
-        irq_spinlock_lock(&cpu->fpu_lock, false);
-#endif
-}
-
-static inline void fpu_owner_unlock(cpu_t *cpu)
-{
-#ifdef CONFIG_FPU_LAZY
-        irq_spinlock_unlock(&cpu->fpu_lock, false);
-#endif
-}
-
-static inline thread_t *fpu_owner(cpu_t *cpu)
-{
-#ifdef CONFIG_FPU_LAZY
-        assert(irq_spinlock_locked(&cpu->fpu_lock));
-        return cpu->fpu_owner;
-#else
-        return NULL;
-#endif
-}
-
 static thread_t *steal_thread_from(cpu_t *old_cpu, int i)
 {
@@ -530 +512 @@
         ipl_t ipl = interrupts_disable();
 
-        fpu_owner_lock(old_cpu);
         irq_spinlock_lock(&old_rq->lock, false);
+
+        /*
+         * If fpu_owner is any thread in the list, its store is seen here thanks to
+         * the runqueue lock.
+         */
+        thread_t *fpu_owner = atomic_load_explicit(&old_cpu->fpu_owner,
+            memory_order_relaxed);
 
         /* Search rq from the back */
@@ -545 +533 @@
                  */
                 if (thread->stolen || thread->nomigrate ||
-                    thread == fpu_owner(old_cpu)) {
+                    thread == fpu_owner) {
                         irq_spinlock_unlock(&thread->lock, false);
                         continue;
                 }
-
-                fpu_owner_unlock(old_cpu);
 
                 thread->stolen = true;
@@ -587 +573 @@
 
         irq_spinlock_unlock(&old_rq->lock, false);
-        fpu_owner_unlock(old_cpu);
         interrupts_restore(ipl);
         return NULL;

kernel/generic/src/proc/thread.c

@@ -415 +415 @@
 #ifdef CONFIG_FPU_LAZY
         if (thread->cpu) {
+                /*
+                 * We need to lock for this because the old CPU can concurrently try
+                 * to dump this thread's FPU state, in which case we need to wait for
+                 * it to finish. An atomic compare-and-swap wouldn't be enough.
+                 */
                 irq_spinlock_lock(&thread->cpu->fpu_lock, false);
-                if (thread->cpu->fpu_owner == thread)
-                        thread->cpu->fpu_owner = NULL;
+
+                thread_t *owner = atomic_load_explicit(&thread->cpu->fpu_owner,
+                    memory_order_relaxed);
+
+                if (owner == thread) {
+                        atomic_store_explicit(&thread->cpu->fpu_owner, NULL,
+                            memory_order_relaxed);
+                }
+
                 irq_spinlock_unlock(&thread->cpu->fpu_lock, false);
         }