From 1cca5142afbd76833deedfdb238230ac53424855 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 18 Oct 2019 14:55:04 +0200 Subject: [PATCH] Kernel: Make DoubleBuffer use a KBuffer instead of kmalloc()ing Background: DoubleBuffer is a handy buffer class in the kernel that allows you to keep writing to it from the "outside" while the "inside" reads from it. It's used for things like LocalSocket and PTY's. Internally, it has a read buffer and a write buffer, but the two will swap places when the read buffer is exhausted (by reading from it.) Before this patch, it was internally implemented as two Vector that we would swap between when the reader side had exhausted the data in the read buffer. Now instead we preallocate a large KBuffer (64KB*2) on DoubleBuffer construction and use that throughout its lifetime. This removes all the kmalloc heap traffic caused by DoubleBuffers :^) --- Kernel/DoubleBuffer.cpp | 45 +++++++++++++++++++++++++------------- Kernel/DoubleBuffer.h | 34 +++++++++++++++------------- Kernel/FileSystem/FIFO.cpp | 2 +- Kernel/Net/LocalSocket.cpp | 4 ++-- Kernel/TTY/MasterPTY.cpp | 2 +- 5 files changed, 53 insertions(+), 34 deletions(-) diff --git a/Kernel/DoubleBuffer.cpp b/Kernel/DoubleBuffer.cpp index d4015f12e5e..512c5064071 100644 --- a/Kernel/DoubleBuffer.cpp +++ b/Kernel/DoubleBuffer.cpp @@ -1,20 +1,32 @@ #include -inline void DoubleBuffer::compute_emptiness() +inline void DoubleBuffer::compute_lockfree_metadata() { - m_empty = m_read_buffer_index >= m_read_buffer->size() && m_write_buffer->is_empty(); + InterruptDisabler disabler; + m_empty = m_read_buffer_index >= m_read_buffer->size && m_write_buffer->size == 0; + m_space_for_writing = m_capacity - m_write_buffer->size; +} + +DoubleBuffer::DoubleBuffer(size_t capacity) + : m_write_buffer(&m_buffer1) + , m_read_buffer(&m_buffer2) + , m_storage(KBuffer::create_with_size(capacity * 2)) + , m_capacity(capacity) +{ + m_buffer1.data = m_storage.data(); + m_buffer1.size = 0; + m_buffer2.data = m_storage.data() + capacity; + m_buffer2.size = 0; + m_space_for_writing = capacity; } void DoubleBuffer::flip() { - ASSERT(m_read_buffer_index == m_read_buffer->size()); + ASSERT(m_read_buffer_index == m_read_buffer->size); swap(m_read_buffer, m_write_buffer); - if (m_write_buffer->capacity() < 32) - m_write_buffer->clear_with_capacity(); - else - m_write_buffer->clear(); + m_write_buffer->size = 0; m_read_buffer_index = 0; - compute_emptiness(); + compute_lockfree_metadata(); } ssize_t DoubleBuffer::write(const u8* data, ssize_t size) @@ -22,8 +34,11 @@ ssize_t DoubleBuffer::write(const u8* data, ssize_t size) if (!size) return 0; LOCKER(m_lock); - m_write_buffer->append(data, size); - compute_emptiness(); + ASSERT(size <= (ssize_t)space_for_writing()); + u8* write_ptr = m_write_buffer->data + m_write_buffer->size; + m_write_buffer->size += size; + compute_lockfree_metadata(); + memcpy(write_ptr, data, size); return size; } @@ -32,13 +47,13 @@ ssize_t DoubleBuffer::read(u8* data, ssize_t size) if (!size) return 0; LOCKER(m_lock); - if (m_read_buffer_index >= m_read_buffer->size() && !m_write_buffer->is_empty()) + if (m_read_buffer_index >= m_read_buffer->size && m_write_buffer->size != 0) flip(); - if (m_read_buffer_index >= m_read_buffer->size()) + if (m_read_buffer_index >= m_read_buffer->size) return 0; - ssize_t nread = min((ssize_t)m_read_buffer->size() - m_read_buffer_index, size); - memcpy(data, m_read_buffer->data() + m_read_buffer_index, nread); + ssize_t nread = min((ssize_t)m_read_buffer->size - (ssize_t)m_read_buffer_index, size); + memcpy(data, m_read_buffer->data + m_read_buffer_index, nread); m_read_buffer_index += nread; - compute_emptiness(); + compute_lockfree_metadata(); return nread; } diff --git a/Kernel/DoubleBuffer.h b/Kernel/DoubleBuffer.h index 44094829c9b..c2f0f9cab49 100644 --- a/Kernel/DoubleBuffer.h +++ b/Kernel/DoubleBuffer.h @@ -1,34 +1,38 @@ #pragma once #include -#include +#include #include class DoubleBuffer { public: - DoubleBuffer() - : m_write_buffer(&m_buffer1) - , m_read_buffer(&m_buffer2) - { - } + explicit DoubleBuffer(size_t capacity = 65536); ssize_t write(const u8*, ssize_t); ssize_t read(u8*, ssize_t); bool is_empty() const { return m_empty; } - // FIXME: Isn't this racy? What if we get interrupted between getting the buffer pointer and dereferencing it? - ssize_t bytes_in_write_buffer() const { return (ssize_t)m_write_buffer->size(); } + size_t space_for_writing() const { return m_space_for_writing; } private: void flip(); - void compute_emptiness(); + void compute_lockfree_metadata(); - Vector* m_write_buffer { nullptr }; - Vector* m_read_buffer { nullptr }; - Vector m_buffer1; - Vector m_buffer2; - ssize_t m_read_buffer_index { 0 }; + struct InnerBuffer { + u8* data { nullptr }; + size_t size; + }; + + InnerBuffer* m_write_buffer { nullptr }; + InnerBuffer* m_read_buffer { nullptr }; + InnerBuffer m_buffer1; + InnerBuffer m_buffer2; + + KBuffer m_storage; + size_t m_capacity { 0 }; + size_t m_read_buffer_index { 0 }; + size_t m_space_for_writing { 0 }; bool m_empty { true }; - Lock m_lock { "DoubleBuffer" }; + mutable Lock m_lock { "DoubleBuffer" }; }; diff --git a/Kernel/FileSystem/FIFO.cpp b/Kernel/FileSystem/FIFO.cpp index 6fc77a7b2bf..1e2982f5196 100644 --- a/Kernel/FileSystem/FIFO.cpp +++ b/Kernel/FileSystem/FIFO.cpp @@ -90,7 +90,7 @@ bool FIFO::can_read(FileDescription&) const bool FIFO::can_write(FileDescription&) const { - return m_buffer.bytes_in_write_buffer() < 4096 || !m_readers; + return m_buffer.space_for_writing() || !m_readers; } ssize_t FIFO::read(FileDescription&, u8* buffer, ssize_t size) diff --git a/Kernel/Net/LocalSocket.cpp b/Kernel/Net/LocalSocket.cpp index c59ab676994..e8fe103a95d 100644 --- a/Kernel/Net/LocalSocket.cpp +++ b/Kernel/Net/LocalSocket.cpp @@ -210,9 +210,9 @@ bool LocalSocket::can_write(FileDescription& description) const { auto role = this->role(description); if (role == Role::Accepted) - return !has_attached_peer(description) || m_for_client.bytes_in_write_buffer() < 16384; + return !has_attached_peer(description) || m_for_client.space_for_writing(); if (role == Role::Connected) - return !has_attached_peer(description) || m_for_server.bytes_in_write_buffer() < 16384; + return !has_attached_peer(description) || m_for_server.space_for_writing(); ASSERT_NOT_REACHED(); } diff --git a/Kernel/TTY/MasterPTY.cpp b/Kernel/TTY/MasterPTY.cpp index a3caf1b17b2..5afeec53554 100644 --- a/Kernel/TTY/MasterPTY.cpp +++ b/Kernel/TTY/MasterPTY.cpp @@ -81,7 +81,7 @@ bool MasterPTY::can_write_from_slave() const { if (m_closed) return true; - return m_buffer.bytes_in_write_buffer() < 4096; + return m_buffer.space_for_writing(); } void MasterPTY::close()