locking rework

now spinlocks also disable interrupts (and essentially preemption)
this is especially useful for scheduling/unblocking tasks from anywhere

run.sh

@@ -3,7 +3,7 @@
 
 POSITIONAL_ARGS=()
 
-QEMU_OPTS=""
+QEMU_OPTS=" -no-reboot "
 
 while [[ $# -gt 0 ]]; do
   case $1 in

src/arch/x86/SerialTty.cpp

@@ -24,21 +24,20 @@ static int read() {
 }
 
 void SerialTty::this_pooler() {
+    mutex.lock();
     while (true) {
-        sleep_self(10000);
-        if (intflag != 0) {
-            if (mutex.try_lock()) {
-                intflag = 0;
-                int r = read();
-                while (r != -1) {
-                    buf.push_back((char) r);
-                    r = read();
-                }
-                cv.notify_one();
-                mutex.unlock();
-            }
+        bool read_something = false;
+        int r = read();
+        while (r != -1) {
+            read_something = true;
+            buf.push_back((char) r);
+            r = read();
         }
+        if (read_something)
+            readercv.notify_all();
+        isrcv.wait(mutex);
     }
+    mutex.unlock();
 }
 
 SerialTty::SerialTty() : Tty() {
@@ -58,14 +57,13 @@ void SerialTty::isr(void *tty) {
 
 
 void SerialTty::this_isr() {
-    intflag.fetch_add(1);
+    isrcv.notify_one();
 }
 
-
 char SerialTty::readchar() {
     mutex.lock();
     if (buf.empty()) {
-        cv.wait(mutex);
+        readercv.wait(mutex);
     }
     assert(!buf.empty());
     char ret = buf.pop_back();

src/arch/x86/SerialTty.hpp

@@ -10,13 +10,14 @@
 #include "cv.hpp"
 
 class SerialTty : public Tty {
+    // TODO: Possibly there should be 2 mutexes?
     Mutex mutex;
-    CV cv;
+    CV readercv;
+    CV isrcv;
     static void isr(void *tty);
 
     void this_isr();
     void this_pooler();
-    std::atomic<int> intflag = 0;
     CircularBuffer<char, 512> buf;
 
 public:

src/arch/x86/kmem.cpp

@@ -8,6 +8,7 @@
 #include "paging.hpp"
 #include "task.hpp"
 
+#include "mutex.hpp"
 #include "string.h"
 
 struct HeapEntry *KERN_HeapBegin;
@@ -25,7 +26,7 @@ uint64_t get_heap_used() {
     return used;
 }
 
-static Spinlock kmem_lock;
+static Mutex kmem_lock;
 
 void init_kern_heap() {
     KERN_HeapBegin = static_cast<HeapEntry *>(get4k());

src/arch/x86/memman.cpp

@@ -7,6 +7,7 @@
 #include "Spinlock.hpp"
 #include "asserts.hpp"
 #include "misc.hpp"
+#include "mutex.hpp"
 #include "paging.hpp"
 #include <stddef.h>
 
@@ -17,7 +18,7 @@
 // Expected to be nulled by the bootloader
 static struct FourPages used_bitmap[BITMAP_SIZE];
 
-static Spinlock memman_lock;
+static Mutex memman_lock;
 
 static uint64_t maxPid = 0;// Past the end
 static uint64_t minPid = 0;

src/arch/x86/misc.hpp

@@ -38,6 +38,22 @@ static inline unsigned long save_irqdisable(void) {
     return flags;
 }
 
+static inline unsigned long save_irqenable(void) {
+    unsigned long flags;
+    asm volatile("pushf\n\tsti\n\tpop %0"
+                 : "=r"(flags)
+                 :
+                 : "memory");
+    return flags;
+}
+
+static inline void irqenable() {
+    asm volatile("sti"
+                 :
+                 :
+                 : "memory");
+}
+
 static inline void irqrestore(unsigned long flags) {
     asm("push %0\n\tpopf"
         :

src/arch/x86/task.cpp

@@ -9,6 +9,7 @@
 #include "TtyManager.hpp"
 #include "VMA.hpp"
 #include "asserts.hpp"
+#include "cv.hpp"
 #include "gdt.hpp"
 #include "globals.hpp"
 #include "kmem.hpp"
@@ -34,20 +35,21 @@ void sanity_check_frame(struct task_frame *cur_frame) {
 }
 
 std::atomic<uint64_t> max_pid = 0;
-Spinlock AllTasks_lock;
+Mutex AllTasks_lock;
 SkipList<uint64_t, Task *> AllTasks;
 
-List<Task *>::Node *RunningTask;
+static List<Task *>::Node *RunningTask;
 
-Spinlock NextTasks_lock;
-List<Task *> NextTasks;
+static Spinlock NextTasks_lock;
+static List<Task *> NextTasks;
 
 // Task freer
-Spinlock TasksToFree_lock;
+Mutex TasksToFree_lock;
+CV TasksToFree_cv;
 List<List<Task *>::Node *> TasksToFree;
 
 // Waiting
-Spinlock WaitingTasks_lock;
+Mutex WaitingTasks_mlock;
 SkipList<uint64_t, List<Task *>::Node *> WaitingTasks;
 
 static std::atomic<bool> initialized = false;
@@ -72,21 +74,36 @@ SkipList<uint64_t, std::pair<String, uint64_t>> getTaskTimePerPid() {
 
 static void task_freer() {
     while (true) {
-        sleep_self(10000);
         {
-            LockGuard l(TasksToFree_lock);
-            while (!TasksToFree.empty()) {
-                List<Task *>::Node *t = TasksToFree.back();
-                TasksToFree.pop_back();
-                uint64_t pid = t->val->pid;
+            while (true) {
                 {
-                    LockGuard l(AllTasks_lock);
-                    AllTasks.erase(pid);
+                    LockGuard l(TasksToFree_lock);
+                    if (TasksToFree.empty()) break;
+                }
+                List<Task *>::Node *t;
+                {
+                    LockGuard l(TasksToFree_lock);
+                    t = TasksToFree.back();
+                    if (t->val->state == TS_RUNNING) break;
+                    TasksToFree.pop_back();
+                }
+                {
+                    uint64_t pid = t->val->pid;
+                    {
+                        LockGuard l(AllTasks_lock);
+                        AllTasks.erase(pid);
+                    }
+                    free_task(t->val);
+                    delete t;
                 }
-                free_task(t->val);
-                delete t;
             }
         }
+        {
+            // TODO: this is kinda ugly
+            TasksToFree_lock.lock();
+            TasksToFree_cv.wait(TasksToFree_lock);
+            TasksToFree_lock.unlock();
+        }
     }
 }
 
@@ -120,7 +137,7 @@ struct Task *new_ktask(void (*fn)(), const char *name, bool start) {
         auto new_node = NextTasks.create_node(newt);
 
         {
-            LockGuard l(NextTasks_lock);
+            SpinlockLockNoInt l(NextTasks_lock);
             NextTasks.emplace_front(new_node);
         }
     }
@@ -185,27 +202,31 @@ struct Task *new_utask(void (*entrypoint)(), const char *name) {
     return newt;
 }
 
-void start_task(struct Task *task) {
+List<Task *>::Node *start_task(struct Task *task) {
     assert(task->state != TS_RUNNING);
     task->state = TS_RUNNING;
     auto new_node = NextTasks.create_node(task);
     {
-        LockGuard l(NextTasks_lock);
+        SpinlockLockNoInt l(NextTasks_lock);
         NextTasks.emplace_front(new_node);
     }
+    return new_node;
 }
 
 
 void remove_self() {
+    assert(RunningTask != nullptr);
     {
         LockGuard l(TasksToFree_lock);
-        assert(RunningTask != nullptr);
+        // TasksToFree is expected to do nothing with TS_RUNNING tasks
         TasksToFree.emplace_front(RunningTask);
-        NextTasks_lock.lock();
-        RunningTask->val->state = TS_BLOCKED;
     }
-    NextTasks_lock.unlock();
-    yield_self();
+    // This might not cause freeing of this task, as it might be preempted
+    // and still be running and task freer won't delete it
+    // But eventually it will get cleaned
+    TasksToFree_cv.notify_one();
+
+    self_block();
     assert2(0, "should be removed!");
 }
 
@@ -213,24 +234,22 @@ void sleep_self(uint64_t diff) {
     uint64_t wake_time = micros + diff;
     while (micros <= wake_time) {
         {
-            LockGuard l(WaitingTasks_lock);
+            LockGuard lm(WaitingTasks_mlock);
 
             // TODO this is all ugly
-            uint64_t l1 = 0;
-            for (auto cur = &*WaitingTasks.begin(); !cur->end; cur = cur->next[0]) l1++;
+            // TODO: also maybe it breaks if it wakes before self_block?
+            uint64_t len1 = 0;
+            for (auto cur = &*WaitingTasks.begin(); !cur->end; cur = cur->next[0]) len1++;
 
             assert(RunningTask != nullptr);
             assert(WaitingTasks.add(wake_time, RunningTask) != nullptr);
 
-            uint64_t l2 = 0;
-            for (auto cur = &*WaitingTasks.begin(); !cur->end; cur = cur->next[0]) l2++;
+            uint64_t len2 = 0;
+            for (auto cur = &*WaitingTasks.begin(); !cur->end; cur = cur->next[0]) len2++;
 
-            assert(l2 - l1 == 1);
-            NextTasks_lock.lock();
-            RunningTask->val->state = TS_BLOCKED;
+            assert(len2 - len1 == 1);
         }
-        NextTasks_lock.unlock();
-        yield_self();
+        self_block();
     }
 }
 
@@ -243,7 +262,7 @@ void yield_self() {
 static void task_waker() {
     while (true) {
         {
-            LockGuard l(WaitingTasks_lock);
+            WaitingTasks_mlock.lock();
 
             while (WaitingTasks.begin() != WaitingTasks.end() && WaitingTasks.begin()->key <= micros && WaitingTasks.begin()->data->val->state != TS_RUNNING) {
                 auto *node = &*WaitingTasks.begin();
@@ -258,23 +277,29 @@ static void task_waker() {
                 uint64_t l2 = 0;
                 for (auto *cur = &*WaitingTasks.begin(); !cur->end; cur = cur->next[0]) l2++;
 
+                WaitingTasks_mlock.unlock();
+
                 assert(l1 - l2 == 1);
                 task->val->sleep_until = 0;
                 task->val->state = TS_RUNNING;
 
                 {
-                    LockGuard l(NextTasks_lock);
+                    SpinlockLockNoInt l(NextTasks_lock);
                     NextTasks.emplace_front(task);
                 }
+
+                WaitingTasks_mlock.lock();
             }
+            WaitingTasks_mlock.unlock();
         }
+        yield_self();
     }
 }
 
 void init_tasks() {
     // FIXME: not actually thread-safe, but it probably doesn't matter
     assert2(!atomic_load(&initialized), "Tasks should be initialized once!");
-    new_ktask(task_freer, "freer");
+    start_task(new_ktask(task_freer, "freer", false));
     new_ktask(task_waker, "waker");
     atomic_store(&initialized, true);
 }
@@ -284,31 +309,34 @@ extern "C" void switch_task(struct task_frame *cur_frame) {
     if (!atomic_load(&initialized)) return;
     sanity_check_frame(cur_frame);
 
-    if (!NextTasks_lock.try_lock()) return;
-
-    static uint64_t lastSwitchMicros = 0;
-    uint64_t prevSwitchMicros = lastSwitchMicros;
-    lastSwitchMicros = micros;
+    assert(!NextTasks_lock.test());
 
     AddressSpace *oldspace = nullptr;
+    List<Task *>::Node *next;
 
-    if (RunningTask) {
-        RunningTask->val->frame = *cur_frame;
-        __builtin_memcpy(RunningTask->val->fxsave, temp_fxsave, 512);
-        oldspace = RunningTask->val->addressSpace;
-        RunningTask->val->used_time.fetch_add(lastSwitchMicros - prevSwitchMicros);
-        if (RunningTask->val->state == TS_RUNNING) {
-            NextTasks.emplace_front(RunningTask);
+    {
+        SpinlockLockNoIntAssert ntl(NextTasks_lock);
+
+        static uint64_t lastSwitchMicros = 0;
+        uint64_t prevSwitchMicros = lastSwitchMicros;
+        lastSwitchMicros = micros;
+
+        if (RunningTask) {
+            RunningTask->val->frame = *cur_frame;
+            __builtin_memcpy(RunningTask->val->fxsave, temp_fxsave, 512);
+            oldspace = RunningTask->val->addressSpace;
+            RunningTask->val->used_time.fetch_add(lastSwitchMicros - prevSwitchMicros);
+            if (RunningTask->val->state == TS_RUNNING) {
+                NextTasks.emplace_front(RunningTask);
+            }
         }
+
+        next = NextTasks.extract_back();
+        assert2(next != NULL, "Kernel left with no tasks!");
+        assert2(next->val != NULL, "Kernel left with no tasks!");
+        assert2(next->val->state == TS_RUNNING, "Blocked task in run queue!");
     }
 
-    List<Task *>::Node *next = NextTasks.extract_back();
-    assert2(next != NULL, "Kernel left with no tasks!");
-    assert2(next->val != NULL, "Kernel left with no tasks!");
-    assert2(next->val->state == TS_RUNNING, "Blocked task in run queue!");
-
-    NextTasks_lock.unlock();
-
     RunningTask = next;
     *cur_frame = RunningTask->val->frame;
     __builtin_memcpy(temp_fxsave, RunningTask->val->fxsave, 512);
@@ -327,16 +355,22 @@ extern "C" void switch_task(struct task_frame *cur_frame) {
 }
 
 void self_block() {
-    {
-        LockGuard l(NextTasks_lock);
-        RunningTask->val->state = TS_BLOCKED;
-    }
-    yield_self();
+    // TODO: clarify this function
+    NO_INT(
+            {
+                {
+                    SpinlockLockNoInt l(NextTasks_lock);
+                    RunningTask->val->state = TS_BLOCKED;
+                }
+                yield_self();
+            })
 }
 
 void self_block(Spinlock &to_unlock) {
+    assert2(!are_interrupts_enabled(), "Self blocking with enabled interrupts!");
+
     {
-        LockGuard l(NextTasks_lock);
+        SpinlockLockNoInt l(NextTasks_lock);
         to_unlock.unlock();
         RunningTask->val->state = TS_BLOCKED;
     }
@@ -344,12 +378,13 @@ void self_block(Spinlock &to_unlock) {
 }
 
 void unblock(Task *what) {
+    assert(false);
     assert(what != nullptr);
     assert(what->state != TS_RUNNING);
     sanity_check_frame(&what->frame);
     auto new_node = NextTasks.create_node(what);
     {
-        LockGuard l(NextTasks_lock);
+        SpinlockLockNoInt l(NextTasks_lock);
         what->state = TS_RUNNING;
         NextTasks.emplace_front(new_node);
     }
@@ -360,7 +395,7 @@ void unblock(List<Task *>::Node *what) {
     assert(what->val->state != TS_RUNNING);
     sanity_check_frame(&what->val->frame);
     {
-        LockGuard l(NextTasks_lock);
+        SpinlockLockNoInt l(NextTasks_lock);
         what->val->state = TS_RUNNING;
         NextTasks.emplace_front(what);
     }

src/arch/x86/task.hpp

@@ -55,7 +55,7 @@ List<Task *>::Node *extract_running_task_node();
 void init_tasks();
 struct Task *new_ktask(void (*fn)(), const char *name, bool start = true);
 struct Task *new_utask(void (*entrypoint)(), const char *name);
-void start_task(struct Task *task);
+List<Task *>::Node *start_task(struct Task *task);
 void remove_self();
 void sleep_self(uint64_t diff);
 

src/kernel/Spinlock.hpp

@@ -22,7 +22,8 @@ public:
         return true;
     }
 
-    void lock() {
+    void spinlock() {
+        assert2(!are_interrupts_enabled(), "Assuming all spinlocks are without interrupts");
         while (!try_lock()) { yield_self(); }
     }
 
@@ -49,4 +50,38 @@ private:
 static_assert(std::is_trivially_copyable_v<Spinlock> == true);
 static_assert(std::is_trivially_destructible_v<Spinlock> == true);
 
+class SpinlockLockNoIntAssert {
+public:
+    SpinlockLockNoIntAssert(Spinlock &lock) : lock(&lock) {
+        assert2(!are_interrupts_enabled(), "Interrupts are expected to be disabled here!");
+        this->lock->spinlock();
+    }
+    ~SpinlockLockNoIntAssert() {
+        lock->unlock();
+    }
+
+    SpinlockLockNoIntAssert(SpinlockLockNoIntAssert const &d) = delete;
+
+private:
+    Spinlock *lock;
+};
+
+class SpinlockLockNoInt {
+public:
+    SpinlockLockNoInt(Spinlock &lock) : lock(&lock) {
+        f = save_irqdisable();
+        this->lock->spinlock();
+    }
+    ~SpinlockLockNoInt() {
+        lock->unlock();
+        irqrestore(f);
+    }
+
+    SpinlockLockNoInt(SpinlockLockNoInt const &d) = delete;
+
+private:
+    Spinlock *lock;
+    unsigned long f;
+};
+
 #endif//OS2_SPINLOCK_H

src/kernel/TtyManager.hpp

@@ -10,7 +10,7 @@
 #include "Vector.hpp"
 
 class TtyManager {
-    Spinlock lock;
+    Mutex lock;
     Vector<Tty *> ttys;
 
 public:

src/kernel/VMA.hpp

@@ -7,6 +7,7 @@
 
 #include "SkipList.hpp"
 #include "Spinlock.hpp"
+#include "mutex.hpp"
 #include <cstddef>
 #include <cstdint>
 
@@ -27,7 +28,7 @@ public:
 
 private:
     AddressSpace *space = nullptr;
-    Spinlock space_lock;
+    Mutex space_lock;
 
     struct ListEntry {
         uintptr_t begin;
@@ -36,7 +37,7 @@ private:
     };
 
     SkipList<uintptr_t, ListEntry> regions;
-    Spinlock regions_lock;
+    Mutex regions_lock;
 };
 
 

src/kernel/cv.hpp

@@ -17,39 +17,38 @@ struct Task;
 
 // This is probably broken in some way
 class CV {
-    List<List<Task *>::Node *> waiters;
+    List<Task *> waiters;
     Spinlock waiters_lock;
 
 public:
     template<typename Lockable>
     void wait(Lockable &l) {
-        l.unlock();
-        waiters_lock.lock();
-        waiters.emplace_front(extract_running_task_node());
-        self_block(waiters_lock);
+        NO_INT(
+                l.unlock();
+                // TODO: recheck this is correct
+                waiters_lock.spinlock();
+                waiters.emplace_front(extract_running_task_node());
+                self_block(waiters_lock);)
         l.lock();
     }
     void notify_one() {
         List<Task *>::Node *t = nullptr;
         {
-            LockGuard l(waiters_lock);
+            SpinlockLockNoInt l(waiters_lock);
             if (!waiters.empty()) {
-                t = waiters.back();
-                waiters.pop_back();
+                t = waiters.extract_back();
             }
         }
         if (t) unblock(t);
     }
     void notify_all() {
-        assert(false);
-        List<List<Task *>::Node *> waiters_new;
+        List<Task *> waiters_new;
         {
-            LockGuard l(waiters_lock);
+            SpinlockLockNoInt l(waiters_lock);
             std::swap(waiters_new, waiters);
         }
         while (!waiters_new.empty()) {
-            auto t = waiters_new.back();
-            waiters_new.pop_back();
+            auto t = waiters_new.extract_back();
             unblock(t);
         }
     }

src/kernel/mutex.cpp

@@ -14,7 +14,7 @@ bool Mutex::try_lock() {
     if (!locked.compare_exchange_strong(expected, true)) {
         return false;
     }
-    owner = cur_task();
+    _owner = cur_task();
     return true;
 }
 
@@ -40,6 +40,7 @@ void Mutex::lock() {
                 break;
             }
 
+            // TODO: this isn't really a spinlock, but for now we don't have SMP
             yield_self();
         }
     }
@@ -52,24 +53,29 @@ void Mutex::lock() {
         if (spin_success > 0)
             spin_success--;
 
+        //        for (int i = 0; i < 100000; i++) {
+        //            __builtin_ia32_pause();
+        //        }
+
         while (!Mutex::try_lock()) {
-            waiters_lock.lock();
-            waiters.emplace_front(extract_running_task_node());
-            self_block(waiters_lock);
+            NO_INT(
+                    waiters_lock.spinlock();
+                    waiters.emplace_front(extract_running_task_node());
+                    self_block(waiters_lock););
         }
     }
 }
 
 void Mutex::unlock() {
     bool expected = true;
+    _owner = nullptr;
     if (!locked.compare_exchange_strong(expected, false))
         assert2(false, "Unlocking an unlocked mutex!\n");
     List<Task *>::Node *t = nullptr;
     {
-        LockGuard l(waiters_lock);
+        SpinlockLockNoInt l(waiters_lock);
         if (!waiters.empty()) {
-            t = waiters.back();
-            waiters.pop_back();
+            t = waiters.extract_back();
         }
     }
     if (t) unblock(t);

src/kernel/mutex.hpp

@@ -24,14 +24,15 @@ public:
     bool try_lock();
     void unlock();
     bool test();
+    Task *owner() { return _owner; }
 
 private:
     std::atomic<bool> locked = false;
 
-    List<List<Task *>::Node *> waiters;
+    List<Task *> waiters;
     Spinlock waiters_lock;
 
-    Task *owner = nullptr;
+    Task *_owner = nullptr;
     uint8_t spin_success = 127;
 };
 

src/kernel/templates/List.hpp

@@ -17,12 +17,15 @@ public:
     struct Node {
         T val;
         Node *next;
+        List *list;
     };
 
 private:
     Node *head = nullptr;
     Node *tail = nullptr;
 
+    uint64_t size = 0;
+
 public:
     List() = default;
     ~List() {
@@ -42,26 +45,40 @@ public:
     }
 
     void emplace_front(Node *new_node) {
+        assert(new_node->list == nullptr);
+        new_node->list = this;
         if (head) {
             assert(tail != nullptr);
+            assert(size > 0);
             head->next = new_node;
             head = new_node;
         } else {
+            assert(size == 0);
             head = new_node;
             tail = new_node;
         }
+        size++;
     }
 
     T &back() {
-        if (tail != nullptr) return tail->val;
+        if (tail != nullptr) {
+            assert(size > 0);
+            return tail->val;
+        }
 
         assert(false);
     }
 
     void pop_back() {
-        if (!head) return;
+        if (!head) {
+            assert(size == 0);
+            return;
+        }
 
+        assert(size > 0);
+        size--;
         if (tail == head) {
+            assert(size == 0);
             delete tail;
             tail = nullptr;
             head = nullptr;
@@ -74,22 +91,30 @@ public:
     }
 
     Node *extract_back() {
-        if (!head) return nullptr;
+        if (!head) {
+            assert(size == 0);
+            return nullptr;
+        }
 
+        assert(size > 0);
+        size--;
         if (tail == head) {
+            assert(size == 0);
             auto b = tail;
             tail = nullptr;
             head = nullptr;
+            b->list = nullptr;
             return b;
         }
 
         auto old_tail = tail;
         tail = tail->next;
+        old_tail->list = nullptr;
         return old_tail;
     }
 
     bool empty() {
-        assert((tail == nullptr) == (head == nullptr));
+        assert(((tail == nullptr) == (head == nullptr) && (head == nullptr) == (size == 0)));
         return head == nullptr;
     }
 };