From a3063dfd337cb43903ba6ce66e7f2cef70e5fba5 Mon Sep 17 00:00:00 2001
From: Andreas Kling <kling@serenityos.org>
Date: Tue, 20 Jul 2021 14:43:15 +0200
Subject: [PATCH] Kernel: Simplify ProcFS generated buffer caching

Use a Mutex instead of a SpinLock to protect the per-FileDescription
generated data cache. This allows processes to go to sleep while
waiting their turn.

Also don't try to be clever by reusing existing cache buffers.
Just allocate KBuffers as needed (and make sure to surface failures.)
---
 Kernel/ProcessExposed.cpp | 61 +++++++++++++++++----------------------
 Kernel/ProcessExposed.h   |  4 +--
 2 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/Kernel/ProcessExposed.cpp b/Kernel/ProcessExposed.cpp
index 15a906eac26..c2864768056 100644
--- a/Kernel/ProcessExposed.cpp
+++ b/Kernel/ProcessExposed.cpp
@@ -69,7 +69,7 @@ ProcFSExposedLink::ProcFSExposedLink(StringView name, InodeIndex preallocated_in
 }
 
 struct ProcFSInodeData : public FileDescriptionData {
-    RefPtr<KBufferImpl> buffer;
+    OwnPtr<KBuffer> buffer;
 };
 
 KResultOr<size_t> ProcFSGlobalInformation::read_bytes(off_t offset, size_t count, UserOrKernelBuffer& buffer, FileDescription* description) const
@@ -81,13 +81,16 @@ KResultOr<size_t> ProcFSGlobalInformation::read_bytes(off_t offset, size_t count
 
     if (!description)
         return KResult(EIO);
+
+    MutexLocker locker(m_refresh_lock);
+
     if (!description->data()) {
         dbgln("ProcFSGlobalInformation: Do not have cached data!");
         return KResult(EIO);
     }
 
-    // Be sure to keep a reference to data_buffer while we use it!
-    RefPtr<KBufferImpl> data_buffer = static_cast<ProcFSInodeData&>(*description->data()).buffer;
+    auto& typed_cached_data = static_cast<ProcFSInodeData&>(*description->data());
+    auto& data_buffer = typed_cached_data.buffer;
 
     if (!data_buffer || (size_t)offset >= data_buffer->size())
         return 0;
@@ -101,26 +104,19 @@ KResultOr<size_t> ProcFSGlobalInformation::read_bytes(off_t offset, size_t count
 
 KResult ProcFSGlobalInformation::refresh_data(FileDescription& description) const
 {
-    ScopedSpinLock lock(m_refresh_lock);
+    MutexLocker lock(m_refresh_lock);
     auto& cached_data = description.data();
-    if (!cached_data)
+    if (!cached_data) {
         cached_data = adopt_own_if_nonnull(new (nothrow) ProcFSInodeData);
-    VERIFY(description.data());
-    auto& buffer = static_cast<ProcFSInodeData&>(*cached_data).buffer;
-    if (buffer) {
-        // If we're reusing the buffer, reset the size to 0 first. This
-        // ensures we don't accidentally leak previously written data.
-        buffer->set_size(0);
+        if (!cached_data)
+            return ENOMEM;
     }
-    KBufferBuilder builder(buffer, true);
+    KBufferBuilder builder;
     if (!const_cast<ProcFSGlobalInformation&>(*this).output(builder))
         return ENOENT;
-    // We don't use builder.build() here, which would steal our buffer
-    // and turn it into an OwnPtr. Instead, just flush to the buffer so
-    // that we can read all the data that was written.
-    if (!builder.flush())
-        return ENOMEM;
-    if (!buffer)
+    auto& typed_cached_data = static_cast<ProcFSInodeData&>(*cached_data);
+    typed_cached_data.buffer = builder.build();
+    if (!typed_cached_data.buffer)
         return ENOMEM;
     return KSuccess;
 }
@@ -139,8 +135,10 @@ KResultOr<size_t> ProcFSProcessInformation::read_bytes(off_t offset, size_t coun
         return KResult(EIO);
     }
 
-    // Be sure to keep a reference to data_buffer while we use it!
-    RefPtr<KBufferImpl> data_buffer = static_cast<ProcFSInodeData&>(*description->data()).buffer;
+    MutexLocker locker(m_refresh_lock);
+
+    auto& typed_cached_data = static_cast<ProcFSInodeData&>(*description->data());
+    auto& data_buffer = typed_cached_data.buffer;
 
     if (!data_buffer || (size_t)offset >= data_buffer->size())
         return 0;
@@ -171,26 +169,19 @@ KResult ProcFSProcessInformation::refresh_data(FileDescription& description) con
     ScopeGuard guard = [&] {
         process->ptrace_lock().unlock();
     };
-    ScopedSpinLock lock(m_refresh_lock);
+    MutexLocker locker(m_refresh_lock);
     auto& cached_data = description.data();
-    if (!cached_data)
+    if (!cached_data) {
         cached_data = adopt_own_if_nonnull(new (nothrow) ProcFSInodeData);
-    VERIFY(description.data());
-    auto& buffer = static_cast<ProcFSInodeData&>(*cached_data).buffer;
-    if (buffer) {
-        // If we're reusing the buffer, reset the size to 0 first. This
-        // ensures we don't accidentally leak previously written data.
-        buffer->set_size(0);
+        if (!cached_data)
+            return ENOMEM;
     }
-    KBufferBuilder builder(buffer, true);
+    KBufferBuilder builder;
     if (!const_cast<ProcFSProcessInformation&>(*this).output(builder))
         return ENOENT;
-    // We don't use builder.build() here, which would steal our buffer
-    // and turn it into an OwnPtr. Instead, just flush to the buffer so
-    // that we can read all the data that was written.
-    if (!builder.flush())
-        return ENOMEM;
-    if (!buffer)
+    auto& typed_cached_data = static_cast<ProcFSInodeData&>(*cached_data);
+    typed_cached_data.buffer = builder.build();
+    if (!typed_cached_data.buffer)
         return ENOMEM;
     return KSuccess;
 }
diff --git a/Kernel/ProcessExposed.h b/Kernel/ProcessExposed.h
index d3336bb71ac..5c4d325342a 100644
--- a/Kernel/ProcessExposed.h
+++ b/Kernel/ProcessExposed.h
@@ -187,7 +187,7 @@ protected:
     virtual KResult refresh_data(FileDescription&) const override;
     virtual bool output(KBufferBuilder& builder) = 0;
 
-    mutable SpinLock<u8> m_refresh_lock;
+    mutable Mutex m_refresh_lock;
 };
 
 class ProcFSSystemBoolean : public ProcFSGlobalInformation {
@@ -245,7 +245,7 @@ protected:
     virtual bool output(KBufferBuilder& builder) = 0;
 
     WeakPtr<ProcFSProcessDirectory> m_parent_directory;
-    mutable SpinLock<u8> m_refresh_lock;
+    mutable Mutex m_refresh_lock;
 };
 
 }