瀏覽代碼

Kernel: Protect Thread::m_name with a spinlock

This replaces manually grabbing the thread's main lock.

This lets us remove the `get_thread_name` and `set_thread_name` syscalls
from the big lock. :^)
Sam Atkins 2 年之前
父節點
當前提交
1014aefe64
共有 5 個文件被更改,包括 27 次插入27 次删除
  1. 2 2
      Kernel/API/Syscall.h
  2. 1 1
      Kernel/FileSystem/SysFS/Subsystems/Kernel/Processes.cpp
  3. 12 12
      Kernel/Syscalls/thread.cpp
  4. 8 0
      Kernel/Thread.cpp
  5. 4 12
      Kernel/Thread.h

+ 2 - 2
Kernel/API/Syscall.h

@@ -85,7 +85,7 @@ enum class NeedsBigProcessLock {
     S(get_process_name, NeedsBigProcessLock::No)            \
     S(get_root_session_id, NeedsBigProcessLock::No)         \
     S(get_stack_bounds, NeedsBigProcessLock::No)            \
-    S(get_thread_name, NeedsBigProcessLock::Yes)            \
+    S(get_thread_name, NeedsBigProcessLock::No)             \
     S(getcwd, NeedsBigProcessLock::No)                      \
     S(getegid, NeedsBigProcessLock::No)                     \
     S(geteuid, NeedsBigProcessLock::No)                     \
@@ -158,7 +158,7 @@ enum class NeedsBigProcessLock {
     S(set_coredump_metadata, NeedsBigProcessLock::No)       \
     S(set_mmap_name, NeedsBigProcessLock::Yes)              \
     S(set_process_name, NeedsBigProcessLock::No)            \
-    S(set_thread_name, NeedsBigProcessLock::Yes)            \
+    S(set_thread_name, NeedsBigProcessLock::No)             \
     S(setegid, NeedsBigProcessLock::No)                     \
     S(seteuid, NeedsBigProcessLock::No)                     \
     S(setgid, NeedsBigProcessLock::No)                      \

+ 1 - 1
Kernel/FileSystem/SysFS/Subsystems/Kernel/Processes.cpp

@@ -118,7 +118,7 @@ ErrorOr<void> SysFSOverallProcesses::try_generate(KBufferBuilder& builder)
             TRY(thread_object.add("lock_count"sv, thread.lock_count()));
 #endif
             TRY(thread_object.add("tid"sv, thread.tid().value()));
-            TRY(thread_object.add("name"sv, thread.name()));
+            TRY(thread.name().with([&](auto& thread_name) { return thread_object.add("name"sv, thread_name->view()); }));
             TRY(thread_object.add("times_scheduled"sv, thread.times_scheduled()));
             TRY(thread_object.add("time_user"sv, thread.time_in_user()));
             TRY(thread_object.add("time_kernel"sv, thread.time_in_kernel()));

+ 12 - 12
Kernel/Syscalls/thread.cpp

@@ -180,7 +180,7 @@ ErrorOr<FlatPtr> Process::sys$kill_thread(pid_t tid, int signal)
 
 ErrorOr<FlatPtr> Process::sys$set_thread_name(pid_t tid, Userspace<char const*> user_name, size_t user_name_length)
 {
-    VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this);
+    VERIFY_NO_PROCESS_BIG_LOCK(this);
     TRY(require_promise(Pledge::stdio));
 
     auto name = TRY(try_copy_kstring_from_user(user_name, user_name_length));
@@ -199,7 +199,7 @@ ErrorOr<FlatPtr> Process::sys$set_thread_name(pid_t tid, Userspace<char const*>
 
 ErrorOr<FlatPtr> Process::sys$get_thread_name(pid_t tid, Userspace<char*> buffer, size_t buffer_size)
 {
-    VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this);
+    VERIFY_NO_PROCESS_BIG_LOCK(this);
     TRY(require_promise(Pledge::thread));
     if (buffer_size == 0)
         return EINVAL;
@@ -208,19 +208,19 @@ ErrorOr<FlatPtr> Process::sys$get_thread_name(pid_t tid, Userspace<char*> buffer
     if (!thread || thread->pid() != pid())
         return ESRCH;
 
-    SpinlockLocker locker(thread->get_lock());
-    auto thread_name = thread->name();
+    TRY(thread->name().with([&](auto& thread_name) -> ErrorOr<void> {
+        if (thread_name->view().is_null()) {
+            char null_terminator = '\0';
+            TRY(copy_to_user(buffer, &null_terminator, sizeof(null_terminator)));
+            return {};
+        }
 
-    if (thread_name.is_null()) {
-        char null_terminator = '\0';
-        TRY(copy_to_user(buffer, &null_terminator, sizeof(null_terminator)));
-        return 0;
-    }
+        if (thread_name->length() + 1 > buffer_size)
+            return ENAMETOOLONG;
 
-    if (thread_name.length() + 1 > buffer_size)
-        return ENAMETOOLONG;
+        return copy_to_user(buffer, thread_name->characters(), thread_name->length() + 1);
+    }));
 
-    TRY(copy_to_user(buffer, thread_name.characters_without_null_termination(), thread_name.length() + 1));
     return 0;
 }
 

+ 8 - 0
Kernel/Thread.cpp

@@ -1466,6 +1466,14 @@ void Thread::track_lock_release(LockRank rank)
 
     m_lock_rank_mask ^= rank;
 }
+
+void Thread::set_name(NonnullOwnPtr<KString> name)
+{
+    m_name.with([&](auto& this_name) {
+        this_name = move(name);
+    });
+}
+
 }
 
 ErrorOr<void> AK::Formatter<Kernel::Thread>::format(FormatBuilder& builder, Kernel::Thread const& value)

+ 4 - 12
Kernel/Thread.h

@@ -94,19 +94,11 @@ public:
     Process& process() { return m_process; }
     Process const& process() const { return m_process; }
 
-    // NOTE: This returns a null-terminated string.
-    StringView name() const
+    SpinlockProtected<NonnullOwnPtr<KString>, LockRank::None> const& name() const
     {
-        // NOTE: Whoever is calling this needs to be holding our lock while reading the name.
-        VERIFY(m_lock.is_locked_by_current_processor());
-        return m_name->view();
-    }
-
-    void set_name(NonnullOwnPtr<KString> name)
-    {
-        SpinlockLocker lock(m_lock);
-        m_name = move(name);
+        return m_name;
     }
+    void set_name(NonnullOwnPtr<KString> name);
 
     void finalize();
 
@@ -1229,7 +1221,7 @@ private:
 
     FPUState m_fpu_state {};
     State m_state { Thread::State::Invalid };
-    NonnullOwnPtr<KString> m_name;
+    SpinlockProtected<NonnullOwnPtr<KString>, LockRank::None> m_name;
     u32 m_priority { THREAD_PRIORITY_NORMAL };
 
     State m_stop_state { Thread::State::Invalid };