From bda818840e25a7f3909cfaa1056b073f64c3b8ab Mon Sep 17 00:00:00 2001 From: Jude Melton-Houghton Date: Fri, 19 May 2023 01:09:21 +0200 Subject: [PATCH 01/29] Add IPC channel (for sscsm) --- src/threading/CMakeLists.txt | 1 + src/threading/ipc_channel.cpp | 281 ++++++++++++++++++++++++++++++++ src/threading/ipc_channel.h | 143 ++++++++++++++++ src/unittest/test_threading.cpp | 61 +++++++ 4 files changed, 486 insertions(+) create mode 100644 src/threading/ipc_channel.cpp create mode 100644 src/threading/ipc_channel.h diff --git a/src/threading/CMakeLists.txt b/src/threading/CMakeLists.txt index 6771b715fd..030d37f28c 100644 --- a/src/threading/CMakeLists.txt +++ b/src/threading/CMakeLists.txt @@ -2,5 +2,6 @@ set(threading_SRCS ${CMAKE_CURRENT_SOURCE_DIR}/event.cpp ${CMAKE_CURRENT_SOURCE_DIR}/thread.cpp ${CMAKE_CURRENT_SOURCE_DIR}/semaphore.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/ipc_channel.cpp PARENT_SCOPE) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp new file mode 100644 index 0000000000..082c20461f --- /dev/null +++ b/src/threading/ipc_channel.cpp @@ -0,0 +1,281 @@ +/* +Minetest +Copyright (C) 2022 Desour +Copyright (C) 2022 TurkeyMcMac, Jude Melton-Houghton + +This program is free software; you can redistribute it and/or modify +it under the terms of the GNU Lesser General Public License as published by +the Free Software Foundation; either version 2.1 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public License along +with this program; if not, write to the Free Software Foundation, Inc., +51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +*/ + +#include "ipc_channel.h" +#include "debug.h" +#include "exceptions.h" +#include "porting.h" +#include +#include +#if defined(__linux__) +#include +#include +#include +#include +#if defined(__i386__) || defined(__x86_64__) +#include +#endif +#endif + +IPCChannelBuffer::IPCChannelBuffer() +{ +#if !defined(__linux__) && !defined(_WIN32) + pthread_condattr_t condattr; + pthread_mutexattr_t mutexattr; + if (pthread_condattr_init(&condattr) != 0) + goto error_condattr_init; + if (pthread_mutexattr_init(&mutexattr) != 0) + goto error_mutexattr_init; + if (pthread_condattr_setpshared(&condattr, 1) != 0) + goto error_condattr_setpshared; + if (pthread_mutexattr_setpshared(&mutexattr, 1) != 0) + goto error_mutexattr_setpshared; + if (pthread_cond_init(&cond, &condattr) != 0) + goto error_cond_init; + if (pthread_mutex_init(&mutex, &mutexattr) != 0) + goto error_mutex_init; + pthread_mutexattr_destroy(&mutexattr); + pthread_condattr_destroy(&condattr); + return; + +error_mutex_init: + pthread_cond_destroy(&cond); +error_cond_init: +error_mutexattr_setpshared: +error_condattr_setpshared: + pthread_mutexattr_destroy(&mutexattr); +error_mutexattr_init: + pthread_condattr_destroy(&condattr); +error_condattr_init: + throw BaseException("Unable to initialize IPCChannelBuffer"); +#endif // !defined(__linux__) && !defined(_WIN32) +} + +IPCChannelBuffer::~IPCChannelBuffer() +{ +#if !defined(__linux__) && !defined(_WIN32) + pthread_mutex_destroy(&mutex); + pthread_cond_destroy(&cond); +#endif // !defined(__linux__) && !defined(_WIN32) +} + +#if defined(_WIN32) + +static bool wait(HANDLE sem, DWORD timeout) +{ + return WaitForSingleObject(sem, timeout) == WAIT_OBJECT_0; +} + +static void post(HANDLE sem) +{ + if (!ReleaseSemaphore(sem, 1, nullptr)) + FATAL_ERROR("ReleaseSemaphore failed unexpectedly"); +} + +#else + +#if defined(__linux__) + +#if defined(__i386__) || defined(__x86_64__) +static void busy_wait(int n) noexcept +{ + for (int i = 0; i < n; i++) + _mm_pause(); +} +#endif // defined(__i386__) || defined(__x86_64__) + +static int futex(std::atomic *uaddr, int futex_op, u32 val, + const struct timespec *timeout, u32 *uaddr2, u32 val3) noexcept +{ + return syscall(SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3); +} + +#endif // defined(__linux__) + +static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept +{ +#if defined(__linux__) + // try busy waiting + for (int i = 0; i < 100; i++) { + // posted? + if (buf->futex.exchange(0) == 1) + return true; // yes +#if defined(__i386__) || defined(__x86_64__) + busy_wait(40); +#else + break; // Busy wait not implemented +#endif + } + // wait with futex + while (true) { + // write 2 to show that we're futexing + if (buf->futex.exchange(2) == 1) { + // futex was posted => change 2 to 0 (or 1 to 1) + buf->futex.fetch_and(1); + return true; + } + int s = futex(&buf->futex, FUTEX_WAIT, 2, timeout, nullptr, 0); + if (s == -1 && errno != EAGAIN) + return false; + } +#else + bool timed_out = false; + pthread_mutex_lock(&buf->mutex); + if (!buf->posted) { + if (timeout) + timed_out = pthread_cond_timedwait(&buf->cond, &buf->mutex, timeout) == ETIMEDOUT; + else + pthread_cond_wait(&buf->cond, &buf->mutex); + } + buf->posted = false; + pthread_mutex_unlock(&buf->mutex); + return !timed_out; +#endif // !defined(__linux__) +} + +static void post(IPCChannelBuffer *buf) noexcept +{ +#if defined(__linux__) + if (buf->futex.exchange(1) == 2) { + int s = futex(&buf->futex, FUTEX_WAKE, 1, nullptr, nullptr, 0); + if (s == -1) + FATAL_ERROR("FUTEX_WAKE failed unexpectedly"); + } +#else + pthread_mutex_lock(&buf->mutex); + buf->posted = true; + pthread_cond_broadcast(&buf->cond); + pthread_mutex_unlock(&buf->mutex); +#endif // !defined(__linux__) +} + +#endif // !defined(_WIN32) + +#if defined(_WIN32) +static DWORD get_timeout(int timeout_ms) +{ + return timeout_ms < 0 ? INFINITE : (DWORD)timeout_ms; +} +#else +static struct timespec *set_timespec(struct timespec *ts, int ms) +{ + if (ms < 0) + return nullptr; + u64 msu = ms; +#if !defined(__linux__) + msu += porting::getTimeMs(); // Absolute time +#endif + ts->tv_sec = msu / 1000; + ts->tv_nsec = msu % 1000 * 1000000UL; + return ts; +} +#endif // !defined(_WIN32) + +bool IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept +{ + m_out->size = size; + memcpy(m_out->data, data, size); +#if defined(_WIN32) + post(m_sem_out); +#else + post(m_out); +#endif + return true; +} + +bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noexcept +{ +#if defined(_WIN32) + DWORD timeout = get_timeout(timeout_ms); +#else + struct timespec timeout, *timeoutp = set_timespec(&timeout, timeout_ms); +#endif + m_out->size = size; + do { + memcpy(m_out->data, data, IPC_CHANNEL_MSG_SIZE); +#if defined(_WIN32) + post(m_sem_out); +#else + post(m_out); +#endif +#if defined(_WIN32) + if (!wait(m_sem_in, timeout)) +#else + if (!wait(m_in, timeoutp)) +#endif + return false; + size -= IPC_CHANNEL_MSG_SIZE; + data = (u8 *)data + IPC_CHANNEL_MSG_SIZE; + } while (size > IPC_CHANNEL_MSG_SIZE); + memcpy(m_out->data, data, size); +#if defined(_WIN32) + post(m_sem_out); +#else + post(m_out); +#endif + return true; +} + +bool IPCChannelEnd::recv(int timeout_ms) noexcept +{ +#if defined(_WIN32) + DWORD timeout = get_timeout(timeout_ms); +#else + struct timespec timeout, *timeoutp = set_timespec(&timeout, timeout_ms); +#endif +#if defined(_WIN32) + if (!wait(m_sem_in, timeout)) +#else + if (!wait(m_in, timeoutp)) +#endif + return false; + size_t size = m_in->size; + if (size <= IPC_CHANNEL_MSG_SIZE) { + m_recv_size = size; + m_recv_data = m_in->data; + } else { + try { + m_large_recv.resize(size); + } catch (...) { + return false; + } + u8 *recv_data = m_large_recv.data(); + m_recv_size = size; + m_recv_data = recv_data; + do { + memcpy(recv_data, m_in->data, IPC_CHANNEL_MSG_SIZE); + size -= IPC_CHANNEL_MSG_SIZE; + recv_data += IPC_CHANNEL_MSG_SIZE; +#if defined(_WIN32) + post(m_sem_out); +#else + post(m_out); +#endif +#if defined(_WIN32) + if (!wait(m_sem_in, timeout)) +#else + if (!wait(m_in, timeoutp)) +#endif + return false; + } while (size > IPC_CHANNEL_MSG_SIZE); + memcpy(recv_data, m_in->data, size); + } + return true; +} diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h new file mode 100644 index 0000000000..3b1a44d291 --- /dev/null +++ b/src/threading/ipc_channel.h @@ -0,0 +1,143 @@ +/* +Minetest +Copyright (C) 2022 Desour +Copyright (C) 2022 TurkeyMcMac, Jude Melton-Houghton + +This program is free software; you can redistribute it and/or modify +it under the terms of the GNU Lesser General Public License as published by +the Free Software Foundation; either version 2.1 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public License along +with this program; if not, write to the Free Software Foundation, Inc., +51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +*/ + +#pragma once + +#include "irrlichttypes.h" +#include +#include +#include +#if defined(_WIN32) +#include +#elif defined(__linux__) +#include +#else +#include +#endif + +/* + An IPC channel is used for synchronous communication between two processes. + Sending two messages in succession from one end is not allowed; messages + must alternate back and forth. + + IPCChannelShared is situated in shared memory and is used by both ends of + the channel. +*/ + +#define IPC_CHANNEL_MSG_SIZE 8192U + +struct IPCChannelBuffer +{ +#if !defined(_WIN32) +#if defined(__linux__) + std::atomic futex = ATOMIC_VAR_INIT(0U); +#else + pthread_cond_t cond; + pthread_mutex_t mutex; + // TODO: use atomic? + bool posted = false; +#endif +#endif // !defined(_WIN32) + size_t size; + u8 data[IPC_CHANNEL_MSG_SIZE]; + + IPCChannelBuffer(); + ~IPCChannelBuffer(); +}; + +struct IPCChannelShared +{ + IPCChannelBuffer a; + IPCChannelBuffer b; +}; + +class IPCChannelEnd +{ +public: + IPCChannelEnd() = default; + +#if defined(_WIN32) + static IPCChannelEnd makeA(IPCChannelShared *shared, HANDLE sem_a, HANDLE sem_b) + { + return IPCChannelEnd(&shared->a, &shared->b, sem_a, sem_b); + } + + static IPCChannelEnd makeB(IPCChannelShared *shared, HANDLE sem_a, HANDLE sem_b) + { + return IPCChannelEnd(&shared->b, &shared->a, sem_b, sem_a); + } +#else + static IPCChannelEnd makeA(IPCChannelShared *shared) + { + return IPCChannelEnd(&shared->a, &shared->b); + } + + static IPCChannelEnd makeB(IPCChannelShared *shared) + { + return IPCChannelEnd(&shared->b, &shared->a); + } +#endif // !defined(_WIN32) + + // If send, recv, or exchange return false, stop using the channel. + // Note: timeouts may be for receiving any response, not a whole message. + + bool send(const void *data, size_t size, int timeout_ms = -1) noexcept + { + if (size <= IPC_CHANNEL_MSG_SIZE) { + return sendSmall(data, size); + } else { + return sendLarge(data, size, timeout_ms); + } + } + + bool recv(int timeout_ms = -1) noexcept; + + bool exchange(const void *data, size_t size, int timeout_ms = -1) noexcept + { + return send(data, size, timeout_ms) && recv(timeout_ms); + } + + // Get information about the last received message + inline const void *getRecvData() const noexcept { return m_recv_data; } + inline size_t getRecvSize() const noexcept { return m_recv_size; } + +private: +#if defined(_WIN32) + IPCChannelEnd(IPCChannelBuffer *in, IPCChannelBuffer *out, HANDLE sem_in, HANDLE sem_out): + m_in(in), m_out(out), m_sem_in(sem_in), m_sem_out(sem_out) + {} +#else + IPCChannelEnd(IPCChannelBuffer *in, IPCChannelBuffer *out): m_in(in), m_out(out) {} +#endif + + bool sendSmall(const void *data, size_t size) noexcept; + + bool sendLarge(const void *data, size_t size, int timeout_ms) noexcept; + + IPCChannelBuffer *m_in = nullptr; + IPCChannelBuffer *m_out = nullptr; +#if defined(_WIN32) + HANDLE m_sem_in; + HANDLE m_sem_out; +#endif + const void *m_recv_data; + size_t m_recv_size; + std::vector m_large_recv; +}; diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index 77842964bd..71742e6c13 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -6,6 +6,10 @@ #include #include +#if defined(_WIN32) +#include +#endif +#include "threading/ipc_channel.h" #include "threading/semaphore.h" #include "threading/thread.h" @@ -19,6 +23,7 @@ public: void testStartStopWait(); void testAtomicSemaphoreThread(); void testTLS(); + void testIPCChannel(); }; static TestThreading g_test_instance; @@ -28,6 +33,7 @@ void TestThreading::runTests(IGameDef *gamedef) TEST(testStartStopWait); TEST(testAtomicSemaphoreThread); TEST(testTLS); + TEST(testIPCChannel); } class SimpleTestThread : public Thread { @@ -227,3 +233,58 @@ void TestThreading::testTLS() } } } + +void TestThreading::testIPCChannel() +{ +#if defined(_WIN32) + HANDLE sem_a = CreateSemaphoreA(nullptr, 0, 1, nullptr); + UASSERT(sem_a != INVALID_HANDLE_VALUE); + + HANDLE sem_b = CreateSemaphoreA(nullptr, 0, 1, nullptr); + UASSERT(sem_b != INVALID_HANDLE_VALUE); +#endif + + IPCChannelShared shared, *sharedp = &shared; + +#if defined(_WIN32) + IPCChannelEnd end_a = IPCChannelEnd::makeA(sharedp, sem_a, sem_b); +#else + IPCChannelEnd end_a = IPCChannelEnd::makeA(sharedp); +#endif + + std::thread thread_b([=] { +#if defined(_WIN32) + IPCChannelEnd end_b = IPCChannelEnd::makeB(sharedp, sem_a, sem_b); +#else + IPCChannelEnd end_b = IPCChannelEnd::makeB(sharedp); +#endif + + for (;;) { + end_b.recv(); + end_b.send(end_b.getRecvData(), end_b.getRecvSize()); + if (end_b.getRecvSize() == 0) + break; + } + }); + + char buf[20000] = {}; + for (int i = sizeof(buf); i > 0; i -= 1000) { + buf[i - 1] = 123; + end_a.exchange(buf, i); + UASSERTEQ(int, end_a.getRecvSize(), i); + UASSERTEQ(int, ((const char *)end_a.getRecvData())[i - 1], 123); + } + + end_a.exchange(buf, 0); + UASSERTEQ(int, end_a.getRecvSize(), 0); + + thread_b.join(); + + UASSERT(!end_a.exchange(buf, 0, 1000)); + +#if defined(_WIN32) + CloseHandle(sem_b); + + CloseHandle(sem_a); +#endif +} From 8cdf8ab95aae9e581b1e0f951b3051876b66e47c Mon Sep 17 00:00:00 2001 From: Desour Date: Wed, 7 Jun 2023 23:48:32 +0200 Subject: [PATCH 02/29] IPCChannelEnd: make the signature of makeA and makeB platform independent and let the IPCChannelEnd own the shared mem and other stuff --- src/threading/ipc_channel.cpp | 33 +++++++++++++++++- src/threading/ipc_channel.h | 60 ++++++++++++++++++--------------- src/unittest/test_threading.cpp | 59 +++++++++++++++++++++----------- 3 files changed, 104 insertions(+), 48 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 082c20461f..202a91ecb6 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -1,6 +1,6 @@ /* Minetest -Copyright (C) 2022 Desour +Copyright (C) 2022 DS Copyright (C) 2022 TurkeyMcMac, Jude Melton-Houghton This program is free software; you can redistribute it and/or modify @@ -188,6 +188,37 @@ static struct timespec *set_timespec(struct timespec *ts, int ms) } #endif // !defined(_WIN32) +#if defined(_WIN32) +IPCChannelEnd IPCChannelEnd::makeA(std::unique_ptr stuff) +{ + IPCChannelShared *shared = stuff->getShared(); + HANDLE sem_a = stuff->getSemA(); + HANDLE sem_b = stuff->getSemB(); + return IPCChannelEnd(std::move(stuff), &shared->a, &shared->b, sem_a, sem_b); +} + +IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr stuff) +{ + IPCChannelShared *shared = stuff->getShared(); + HANDLE sem_a = stuff->getSemA(); + HANDLE sem_b = stuff->getSemB(); + return IPCChannelEnd(std::move(stuff), &shared->b, &shared->a, sem_b, sem_a); +} + +#else // defined(_WIN32) +IPCChannelEnd IPCChannelEnd::makeA(std::unique_ptr stuff) +{ + IPCChannelShared *shared = stuff->getShared(); + return IPCChannelEnd(std::move(stuff), &shared->a, &shared->b); +} + +IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr stuff) +{ + IPCChannelShared *shared = stuff->getShared(); + return IPCChannelEnd(std::move(stuff), &shared->b, &shared->a); +} +#endif // !defined(_WIN32) + bool IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept { m_out->size = size; diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 3b1a44d291..b16b24da21 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -1,6 +1,6 @@ /* Minetest -Copyright (C) 2022 Desour +Copyright (C) 2022 DS Copyright (C) 2022 TurkeyMcMac, Jude Melton-Houghton This program is free software; you can redistribute it and/or modify @@ -21,9 +21,11 @@ with this program; if not, write to the Free Software Foundation, Inc., #pragma once #include "irrlichttypes.h" +#include #include #include #include + #if defined(_WIN32) #include #elif defined(__linux__) @@ -47,7 +49,7 @@ struct IPCChannelBuffer { #if !defined(_WIN32) #if defined(__linux__) - std::atomic futex = ATOMIC_VAR_INIT(0U); + std::atomic futex{0}; #else pthread_cond_t cond; pthread_mutex_t mutex; @@ -56,7 +58,7 @@ struct IPCChannelBuffer #endif #endif // !defined(_WIN32) size_t size; - u8 data[IPC_CHANNEL_MSG_SIZE]; + u8 data[IPC_CHANNEL_MSG_SIZE]; //TODO: volatile? IPCChannelBuffer(); ~IPCChannelBuffer(); @@ -68,32 +70,25 @@ struct IPCChannelShared IPCChannelBuffer b; }; +// opaque owner for the shared mem and stuff +// users have to implement this +struct IPCChannelStuff +{ + virtual ~IPCChannelStuff() = default; + virtual IPCChannelShared *getShared() = 0; +#ifdef _WIN32 + virtual HANDLE getSemA() = 0; + virtual HANDLE getSemB() = 0; +#endif +}; + class IPCChannelEnd { public: IPCChannelEnd() = default; -#if defined(_WIN32) - static IPCChannelEnd makeA(IPCChannelShared *shared, HANDLE sem_a, HANDLE sem_b) - { - return IPCChannelEnd(&shared->a, &shared->b, sem_a, sem_b); - } - - static IPCChannelEnd makeB(IPCChannelShared *shared, HANDLE sem_a, HANDLE sem_b) - { - return IPCChannelEnd(&shared->b, &shared->a, sem_b, sem_a); - } -#else - static IPCChannelEnd makeA(IPCChannelShared *shared) - { - return IPCChannelEnd(&shared->a, &shared->b); - } - - static IPCChannelEnd makeB(IPCChannelShared *shared) - { - return IPCChannelEnd(&shared->b, &shared->a); - } -#endif // !defined(_WIN32) + static IPCChannelEnd makeA(std::unique_ptr stuff); + static IPCChannelEnd makeB(std::unique_ptr stuff); // If send, recv, or exchange return false, stop using the channel. // Note: timeouts may be for receiving any response, not a whole message. @@ -120,17 +115,28 @@ public: private: #if defined(_WIN32) - IPCChannelEnd(IPCChannelBuffer *in, IPCChannelBuffer *out, HANDLE sem_in, HANDLE sem_out): - m_in(in), m_out(out), m_sem_in(sem_in), m_sem_out(sem_out) + IPCChannelEnd( + std::unique_ptr stuff, + IPCChannelBuffer *in, IPCChannelBuffer *out, + HANDLE sem_in, HANDLE sem_out) : + m_stuff(std::move(stuff)), + m_in(in), m_out(out), + m_sem_in(sem_in), m_sem_out(sem_out) {} #else - IPCChannelEnd(IPCChannelBuffer *in, IPCChannelBuffer *out): m_in(in), m_out(out) {} + IPCChannelEnd( + std::unique_ptr stuff, + IPCChannelBuffer *in, IPCChannelBuffer *out) : + m_stuff(std::move(stuff)), + m_in(in), m_out(out) + {} #endif bool sendSmall(const void *data, size_t size) noexcept; bool sendLarge(const void *data, size_t size, int timeout_ms) noexcept; + std::unique_ptr m_stuff; IPCChannelBuffer *m_in = nullptr; IPCChannelBuffer *m_out = nullptr; #if defined(_WIN32) diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index 71742e6c13..ed56abe61a 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -236,28 +236,53 @@ void TestThreading::testTLS() void TestThreading::testIPCChannel() { + struct Stuff + { + IPCChannelShared shared{}; #if defined(_WIN32) - HANDLE sem_a = CreateSemaphoreA(nullptr, 0, 1, nullptr); - UASSERT(sem_a != INVALID_HANDLE_VALUE); - - HANDLE sem_b = CreateSemaphoreA(nullptr, 0, 1, nullptr); - UASSERT(sem_b != INVALID_HANDLE_VALUE); + HANDLE sem_a; + HANDLE sem_b; #endif + Stuff() + { +#ifdef _WIN32 + HANDLE sem_a = CreateSemaphoreA(nullptr, 0, 1, nullptr); + UASSERT(sem_a != INVALID_HANDLE_VALUE); - IPCChannelShared shared, *sharedp = &shared; + HANDLE sem_b = CreateSemaphoreA(nullptr, 0, 1, nullptr); + UASSERT(sem_b != INVALID_HANDLE_VALUE); +#endif + } + ~Stuff() + { +#ifdef _WIN32 + CloseHandle(sem_b); + CloseHandle(sem_a); +#endif + } + }; + + struct IPCChannelStuffSingleProcess final : public IPCChannelStuff + { + std::shared_ptr stuff; + + IPCChannelStuffSingleProcess(std::shared_ptr stuff) : stuff(std::move(stuff)) {} + ~IPCChannelStuffSingleProcess() override = default; + + IPCChannelShared *getShared() override { return &stuff->shared; } #if defined(_WIN32) - IPCChannelEnd end_a = IPCChannelEnd::makeA(sharedp, sem_a, sem_b); -#else - IPCChannelEnd end_a = IPCChannelEnd::makeA(sharedp); + HANDLE getSemA() override { return stuff->sem_a; } + HANDLE getSemB() override { return stuff->sem_b; } #endif + }; + + auto stuff = std::make_shared(); + + IPCChannelEnd end_a = IPCChannelEnd::makeA(std::make_unique(stuff)); std::thread thread_b([=] { -#if defined(_WIN32) - IPCChannelEnd end_b = IPCChannelEnd::makeB(sharedp, sem_a, sem_b); -#else - IPCChannelEnd end_b = IPCChannelEnd::makeB(sharedp); -#endif + IPCChannelEnd end_b = IPCChannelEnd::makeB(std::make_unique(stuff)); for (;;) { end_b.recv(); @@ -281,10 +306,4 @@ void TestThreading::testIPCChannel() thread_b.join(); UASSERT(!end_a.exchange(buf, 0, 1000)); - -#if defined(_WIN32) - CloseHandle(sem_b); - - CloseHandle(sem_a); -#endif } From 5f13c0aa4863c814e7c5264bb1edccc106e24882 Mon Sep 17 00:00:00 2001 From: Desour Date: Thu, 8 Jun 2023 01:45:26 +0200 Subject: [PATCH 03/29] add some comments, and improve error handling --- src/threading/ipc_channel.cpp | 57 +++++++++++++++++++++-------------- src/threading/ipc_channel.h | 30 ++++++++++++------ 2 files changed, 55 insertions(+), 32 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 202a91ecb6..113a7f701c 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -78,6 +78,7 @@ IPCChannelBuffer::~IPCChannelBuffer() #if defined(_WIN32) +// returns false on timeout static bool wait(HANDLE sem, DWORD timeout) { return WaitForSingleObject(sem, timeout) == WAIT_OBJECT_0; @@ -109,6 +110,8 @@ static int futex(std::atomic *uaddr, int futex_op, u32 val, #endif // defined(__linux__) +// timeout: relative on linux, and absolute on other posix +// returns false on timeout static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept { #if defined(__linux__) @@ -132,8 +135,15 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept return true; } int s = futex(&buf->futex, FUTEX_WAIT, 2, timeout, nullptr, 0); - if (s == -1 && errno != EAGAIN) - return false; + if (s == -1) { + if (errno == ETIMEDOUT) { + return false; + } else if (errno != EAGAIN && errno != EINTR) { + std::string errmsg = std::string("FUTEX_WAIT failed unexpectedly: ") + + std::strerror(errno); + FATAL_ERROR(errmsg.c_str()); + } + } } #else bool timed_out = false; @@ -154,9 +164,13 @@ static void post(IPCChannelBuffer *buf) noexcept { #if defined(__linux__) if (buf->futex.exchange(1) == 2) { + // 2 means reader needs to be notified int s = futex(&buf->futex, FUTEX_WAKE, 1, nullptr, nullptr, 0); - if (s == -1) - FATAL_ERROR("FUTEX_WAKE failed unexpectedly"); + if (s == -1) { + std::string errmsg = std::string("FUTEX_WAKE failed unexpectedly: ") + + std::strerror(errno); + FATAL_ERROR(errmsg.c_str()); + } } #else pthread_mutex_lock(&buf->mutex); @@ -188,38 +202,31 @@ static struct timespec *set_timespec(struct timespec *ts, int ms) } #endif // !defined(_WIN32) -#if defined(_WIN32) IPCChannelEnd IPCChannelEnd::makeA(std::unique_ptr stuff) { IPCChannelShared *shared = stuff->getShared(); +#if defined(_WIN32) HANDLE sem_a = stuff->getSemA(); HANDLE sem_b = stuff->getSemB(); return IPCChannelEnd(std::move(stuff), &shared->a, &shared->b, sem_a, sem_b); +#else + return IPCChannelEnd(std::move(stuff), &shared->a, &shared->b); +#endif // !defined(_WIN32) } IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr stuff) { IPCChannelShared *shared = stuff->getShared(); +#if defined(_WIN32) HANDLE sem_a = stuff->getSemA(); HANDLE sem_b = stuff->getSemB(); return IPCChannelEnd(std::move(stuff), &shared->b, &shared->a, sem_b, sem_a); -} - -#else // defined(_WIN32) -IPCChannelEnd IPCChannelEnd::makeA(std::unique_ptr stuff) -{ - IPCChannelShared *shared = stuff->getShared(); - return IPCChannelEnd(std::move(stuff), &shared->a, &shared->b); -} - -IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr stuff) -{ - IPCChannelShared *shared = stuff->getShared(); +#else return IPCChannelEnd(std::move(stuff), &shared->b, &shared->a); -} #endif // !defined(_WIN32) +} -bool IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept +void IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept { m_out->size = size; memcpy(m_out->data, data, size); @@ -228,7 +235,6 @@ bool IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept #else post(m_out); #endif - return true; } bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noexcept @@ -236,7 +242,8 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe #if defined(_WIN32) DWORD timeout = get_timeout(timeout_ms); #else - struct timespec timeout, *timeoutp = set_timespec(&timeout, timeout_ms); + struct timespec timeout; + struct timespec *timeoutp = set_timespec(&timeout, timeout_ms); #endif m_out->size = size; do { @@ -269,7 +276,8 @@ bool IPCChannelEnd::recv(int timeout_ms) noexcept #if defined(_WIN32) DWORD timeout = get_timeout(timeout_ms); #else - struct timespec timeout, *timeoutp = set_timespec(&timeout, timeout_ms); + struct timespec timeout; + struct timespec *timeoutp = set_timespec(&timeout, timeout_ms); #endif #if defined(_WIN32) if (!wait(m_sem_in, timeout)) @@ -285,7 +293,10 @@ bool IPCChannelEnd::recv(int timeout_ms) noexcept try { m_large_recv.resize(size); } catch (...) { - return false; + // it's ok for us if an attacker wants to make us abort + std::string errmsg = std::string("std::vector::resize failed, size was: ") + + std::to_string(size); + FATAL_ERROR(errmsg.c_str()); } u8 *recv_data = m_large_recv.data(); m_recv_size = size; diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index b16b24da21..b085d286af 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -41,6 +41,11 @@ with this program; if not, write to the Free Software Foundation, Inc., IPCChannelShared is situated in shared memory and is used by both ends of the channel. + + There are currently 3 implementations for synchronisation: + * win32: uses win32 semaphore + * linux: uses futex, and does busy waiting if on x86/x86_64 + * other posix: uses posix mutex and condition variable */ #define IPC_CHANNEL_MSG_SIZE 8192U @@ -49,16 +54,21 @@ struct IPCChannelBuffer { #if !defined(_WIN32) #if defined(__linux__) + // possible values: + // 0: futex is not posted. reader will check value before blocking => no + // notify needed when posting + // 1: futex is posted + // 2: futex is not posted. reader is waiting with futex syscall, and needs + // to be notified std::atomic futex{0}; #else pthread_cond_t cond; pthread_mutex_t mutex; - // TODO: use atomic? - bool posted = false; + bool posted = false; // protected by mutex #endif #endif // !defined(_WIN32) size_t size; - u8 data[IPC_CHANNEL_MSG_SIZE]; //TODO: volatile? + u8 data[IPC_CHANNEL_MSG_SIZE]; IPCChannelBuffer(); ~IPCChannelBuffer(); @@ -90,13 +100,14 @@ public: static IPCChannelEnd makeA(std::unique_ptr stuff); static IPCChannelEnd makeB(std::unique_ptr stuff); - // If send, recv, or exchange return false, stop using the channel. + // If send, recv, or exchange return false (=timeout), stop using the channel. // Note: timeouts may be for receiving any response, not a whole message. bool send(const void *data, size_t size, int timeout_ms = -1) noexcept { if (size <= IPC_CHANNEL_MSG_SIZE) { - return sendSmall(data, size); + sendSmall(data, size); + return true; } else { return sendLarge(data, size, timeout_ms); } @@ -109,7 +120,7 @@ public: return send(data, size, timeout_ms) && recv(timeout_ms); } - // Get information about the last received message + // Get the content of the last received message inline const void *getRecvData() const noexcept { return m_recv_data; } inline size_t getRecvSize() const noexcept { return m_recv_size; } @@ -132,8 +143,9 @@ private: {} #endif - bool sendSmall(const void *data, size_t size) noexcept; + void sendSmall(const void *data, size_t size) noexcept; + // returns false on timeout bool sendLarge(const void *data, size_t size, int timeout_ms) noexcept; std::unique_ptr m_stuff; @@ -143,7 +155,7 @@ private: HANDLE m_sem_in; HANDLE m_sem_out; #endif - const void *m_recv_data; - size_t m_recv_size; + const void *m_recv_data = nullptr; + size_t m_recv_size = 0; std::vector m_large_recv; }; From 1ee61c4aff38faef7200e99a3be561bb333e60c2 Mon Sep 17 00:00:00 2001 From: Jude Melton-Houghton Date: Thu, 8 Jun 2023 02:01:13 +0200 Subject: [PATCH 04/29] add linux headers as dependency --- Dockerfile | 2 +- doc/compiling/linux.md | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Dockerfile b/Dockerfile index c9a9848c53..0bddf0d91c 100644 --- a/Dockerfile +++ b/Dockerfile @@ -5,7 +5,7 @@ ENV LUAJIT_VERSION v2.1 RUN apk add --no-cache git build-base cmake curl-dev zlib-dev zstd-dev \ sqlite-dev postgresql-dev hiredis-dev leveldb-dev \ - gmp-dev jsoncpp-dev ninja ca-certificates + gmp-dev jsoncpp-dev linux-headers ninja ca-certificates WORKDIR /usr/src/ RUN git clone --recursive https://github.com/jupp0r/prometheus-cpp && \ diff --git a/doc/compiling/linux.md b/doc/compiling/linux.md index 946d88dac1..d2372fc305 100644 --- a/doc/compiling/linux.md +++ b/doc/compiling/linux.md @@ -22,7 +22,7 @@ For Debian/Ubuntu users: - sudo apt install g++ make libc6-dev cmake libpng-dev libjpeg-dev libgl1-mesa-dev libsqlite3-dev libogg-dev libvorbis-dev libopenal-dev libcurl4-gnutls-dev libfreetype6-dev zlib1g-dev libgmp-dev libjsoncpp-dev libzstd-dev libluajit-5.1-dev gettext libsdl2-dev + sudo apt install g++ make libc6-dev cmake linux-libc-dev libpng-dev libjpeg-dev libgl1-mesa-dev libsqlite3-dev libogg-dev libvorbis-dev libopenal-dev libcurl4-gnutls-dev libfreetype6-dev zlib1g-dev libgmp-dev libjsoncpp-dev libzstd-dev libluajit-5.1-dev gettext libsdl2-dev For Fedora users: @@ -34,11 +34,11 @@ For openSUSE users: For Arch users: - sudo pacman -S --needed base-devel libcurl-gnutls cmake libpng libjpeg-turbo sqlite libogg libvorbis openal freetype2 jsoncpp gmp luajit leveldb ncurses zstd gettext sdl2 + sudo pacman -S --needed base-devel libcurl-gnutls cmake linux-api-headers libpng libjpeg-turbo sqlite libogg libvorbis openal freetype2 jsoncpp gmp luajit leveldb ncurses zstd gettext sdl2 For Alpine users: - sudo apk add build-base cmake libpng-dev jpeg-dev mesa-dev sqlite-dev libogg-dev libvorbis-dev openal-soft-dev curl-dev freetype-dev zlib-dev gmp-dev jsoncpp-dev luajit-dev zstd-dev gettext sdl2-dev + sudo apk add build-base cmake linux-headers libpng-dev jpeg-dev mesa-dev sqlite-dev libogg-dev libvorbis-dev openal-soft-dev curl-dev freetype-dev zlib-dev gmp-dev jsoncpp-dev luajit-dev zstd-dev gettext sdl2-dev For Void users: From 4f0085af000d2693f90556683b851e84f4beb567 Mon Sep 17 00:00:00 2001 From: Desour Date: Thu, 8 Jun 2023 22:19:59 +0200 Subject: [PATCH 05/29] make sure we copy from the shared memory only once --- src/threading/ipc_channel.cpp | 25 ++++++++++++++++++------- src/threading/ipc_channel.h | 16 ++++++++++------ src/unittest/test_threading.cpp | 8 ++++---- 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 113a7f701c..1ea61e7fff 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -202,6 +202,18 @@ static struct timespec *set_timespec(struct timespec *ts, int ms) } #endif // !defined(_WIN32) +template +static inline void write_once(volatile T *var, const T val) +{ + *var = val; +} + +template +static inline T read_once(const volatile T *var) +{ + return *var; +} + IPCChannelEnd IPCChannelEnd::makeA(std::unique_ptr stuff) { IPCChannelShared *shared = stuff->getShared(); @@ -228,7 +240,7 @@ IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr stuff) void IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept { - m_out->size = size; + write_once(&m_out->size, size); memcpy(m_out->data, data, size); #if defined(_WIN32) post(m_sem_out); @@ -245,7 +257,7 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe struct timespec timeout; struct timespec *timeoutp = set_timespec(&timeout, timeout_ms); #endif - m_out->size = size; + write_once(&m_out->size, size); do { memcpy(m_out->data, data, IPC_CHANNEL_MSG_SIZE); #if defined(_WIN32) @@ -285,10 +297,11 @@ bool IPCChannelEnd::recv(int timeout_ms) noexcept if (!wait(m_in, timeoutp)) #endif return false; - size_t size = m_in->size; + size_t size = read_once(&m_in->size); + m_recv_size = size; if (size <= IPC_CHANNEL_MSG_SIZE) { - m_recv_size = size; - m_recv_data = m_in->data; + // m_large_recv.size() is always >= IPC_CHANNEL_MSG_SIZE + memcpy(m_large_recv.data(), m_in->data, size); } else { try { m_large_recv.resize(size); @@ -299,8 +312,6 @@ bool IPCChannelEnd::recv(int timeout_ms) noexcept FATAL_ERROR(errmsg.c_str()); } u8 *recv_data = m_large_recv.data(); - m_recv_size = size; - m_recv_data = recv_data; do { memcpy(recv_data, m_in->data, IPC_CHANNEL_MSG_SIZE); size -= IPC_CHANNEL_MSG_SIZE; diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index b085d286af..8247eddb58 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -48,7 +48,7 @@ with this program; if not, write to the Free Software Foundation, Inc., * other posix: uses posix mutex and condition variable */ -#define IPC_CHANNEL_MSG_SIZE 8192U +#define IPC_CHANNEL_MSG_SIZE 0x2000U struct IPCChannelBuffer { @@ -67,8 +67,11 @@ struct IPCChannelBuffer bool posted = false; // protected by mutex #endif #endif // !defined(_WIN32) - size_t size; - u8 data[IPC_CHANNEL_MSG_SIZE]; + // Note: If the other side isn't acting cooperatively, they might write to + // this at any times. So we must make sure to copy out the data once, and + // only access that copy. + size_t size = 0; + u8 data[IPC_CHANNEL_MSG_SIZE] = {}; IPCChannelBuffer(); ~IPCChannelBuffer(); @@ -121,7 +124,7 @@ public: } // Get the content of the last received message - inline const void *getRecvData() const noexcept { return m_recv_data; } + inline const void *getRecvData() const noexcept { return m_large_recv.data(); } inline size_t getRecvSize() const noexcept { return m_recv_size; } private: @@ -155,7 +158,8 @@ private: HANDLE m_sem_in; HANDLE m_sem_out; #endif - const void *m_recv_data = nullptr; size_t m_recv_size = 0; - std::vector m_large_recv; + // we always copy from the shared buffer into this + // (this buffer only grows) + std::vector m_large_recv = std::vector(IPC_CHANNEL_MSG_SIZE); }; diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index ed56abe61a..28bde44ee7 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -285,8 +285,8 @@ void TestThreading::testIPCChannel() IPCChannelEnd end_b = IPCChannelEnd::makeB(std::make_unique(stuff)); for (;;) { - end_b.recv(); - end_b.send(end_b.getRecvData(), end_b.getRecvSize()); + UASSERT(end_b.recv()); + UASSERT(end_b.send(end_b.getRecvData(), end_b.getRecvSize())); if (end_b.getRecvSize() == 0) break; } @@ -295,12 +295,12 @@ void TestThreading::testIPCChannel() char buf[20000] = {}; for (int i = sizeof(buf); i > 0; i -= 1000) { buf[i - 1] = 123; - end_a.exchange(buf, i); + UASSERT(end_a.exchange(buf, i)); UASSERTEQ(int, end_a.getRecvSize(), i); UASSERTEQ(int, ((const char *)end_a.getRecvData())[i - 1], 123); } - end_a.exchange(buf, 0); + UASSERT(end_a.exchange(buf, 0)); UASSERTEQ(int, end_a.getRecvSize(), 0); thread_b.join(); From 1c76f32d19292d768af1f3cd86d595ff56fa860e Mon Sep 17 00:00:00 2001 From: Desour Date: Thu, 8 Jun 2023 22:54:31 +0200 Subject: [PATCH 06/29] use more abstract macros to decide which implementation to compile --- src/threading/ipc_channel.cpp | 69 +++++++++++++++++---------------- src/threading/ipc_channel.h | 26 ++++++++----- src/unittest/test_threading.cpp | 10 ++--- 3 files changed, 57 insertions(+), 48 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 1ea61e7fff..982e3831ed 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -22,11 +22,12 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "debug.h" #include "exceptions.h" #include "porting.h" -#include +#include #include -#if defined(__linux__) + +#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) #include -#include +#include #include #include #if defined(__i386__) || defined(__x86_64__) @@ -36,7 +37,7 @@ with this program; if not, write to the Free Software Foundation, Inc., IPCChannelBuffer::IPCChannelBuffer() { -#if !defined(__linux__) && !defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) pthread_condattr_t condattr; pthread_mutexattr_t mutexattr; if (pthread_condattr_init(&condattr) != 0) @@ -65,18 +66,18 @@ error_mutexattr_init: pthread_condattr_destroy(&condattr); error_condattr_init: throw BaseException("Unable to initialize IPCChannelBuffer"); -#endif // !defined(__linux__) && !defined(_WIN32) +#endif // defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) } IPCChannelBuffer::~IPCChannelBuffer() { -#if !defined(__linux__) && !defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) pthread_mutex_destroy(&mutex); pthread_cond_destroy(&cond); -#endif // !defined(__linux__) && !defined(_WIN32) +#endif // defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) } -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) // returns false on timeout static bool wait(HANDLE sem, DWORD timeout) @@ -92,7 +93,7 @@ static void post(HANDLE sem) #else -#if defined(__linux__) +#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) #if defined(__i386__) || defined(__x86_64__) static void busy_wait(int n) noexcept @@ -108,13 +109,13 @@ static int futex(std::atomic *uaddr, int futex_op, u32 val, return syscall(SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3); } -#endif // defined(__linux__) +#endif // defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) // timeout: relative on linux, and absolute on other posix // returns false on timeout static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept { -#if defined(__linux__) +#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) // try busy waiting for (int i = 0; i < 100; i++) { // posted? @@ -145,7 +146,7 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept } } } -#else +#elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) bool timed_out = false; pthread_mutex_lock(&buf->mutex); if (!buf->posted) { @@ -157,12 +158,12 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept buf->posted = false; pthread_mutex_unlock(&buf->mutex); return !timed_out; -#endif // !defined(__linux__) +#endif } static void post(IPCChannelBuffer *buf) noexcept { -#if defined(__linux__) +#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) if (buf->futex.exchange(1) == 2) { // 2 means reader needs to be notified int s = futex(&buf->futex, FUTEX_WAKE, 1, nullptr, nullptr, 0); @@ -172,35 +173,35 @@ static void post(IPCChannelBuffer *buf) noexcept FATAL_ERROR(errmsg.c_str()); } } -#else +#elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) pthread_mutex_lock(&buf->mutex); buf->posted = true; pthread_cond_broadcast(&buf->cond); pthread_mutex_unlock(&buf->mutex); -#endif // !defined(__linux__) +#endif } -#endif // !defined(_WIN32) +#endif // !defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) static DWORD get_timeout(int timeout_ms) { return timeout_ms < 0 ? INFINITE : (DWORD)timeout_ms; } -#else +#elif defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) || defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) static struct timespec *set_timespec(struct timespec *ts, int ms) { if (ms < 0) return nullptr; u64 msu = ms; -#if !defined(__linux__) +#if defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) msu += porting::getTimeMs(); // Absolute time #endif ts->tv_sec = msu / 1000; ts->tv_nsec = msu % 1000 * 1000000UL; return ts; } -#endif // !defined(_WIN32) +#endif template static inline void write_once(volatile T *var, const T val) @@ -217,32 +218,32 @@ static inline T read_once(const volatile T *var) IPCChannelEnd IPCChannelEnd::makeA(std::unique_ptr stuff) { IPCChannelShared *shared = stuff->getShared(); -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) HANDLE sem_a = stuff->getSemA(); HANDLE sem_b = stuff->getSemB(); return IPCChannelEnd(std::move(stuff), &shared->a, &shared->b, sem_a, sem_b); #else return IPCChannelEnd(std::move(stuff), &shared->a, &shared->b); -#endif // !defined(_WIN32) +#endif // !defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) } IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr stuff) { IPCChannelShared *shared = stuff->getShared(); -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) HANDLE sem_a = stuff->getSemA(); HANDLE sem_b = stuff->getSemB(); return IPCChannelEnd(std::move(stuff), &shared->b, &shared->a, sem_b, sem_a); #else return IPCChannelEnd(std::move(stuff), &shared->b, &shared->a); -#endif // !defined(_WIN32) +#endif // !defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) } void IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept { write_once(&m_out->size, size); memcpy(m_out->data, data, size); -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) post(m_sem_out); #else post(m_out); @@ -251,7 +252,7 @@ void IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noexcept { -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) DWORD timeout = get_timeout(timeout_ms); #else struct timespec timeout; @@ -260,12 +261,12 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe write_once(&m_out->size, size); do { memcpy(m_out->data, data, IPC_CHANNEL_MSG_SIZE); -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) post(m_sem_out); #else post(m_out); #endif -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) if (!wait(m_sem_in, timeout)) #else if (!wait(m_in, timeoutp)) @@ -275,7 +276,7 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe data = (u8 *)data + IPC_CHANNEL_MSG_SIZE; } while (size > IPC_CHANNEL_MSG_SIZE); memcpy(m_out->data, data, size); -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) post(m_sem_out); #else post(m_out); @@ -285,13 +286,13 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe bool IPCChannelEnd::recv(int timeout_ms) noexcept { -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) DWORD timeout = get_timeout(timeout_ms); #else struct timespec timeout; struct timespec *timeoutp = set_timespec(&timeout, timeout_ms); #endif -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) if (!wait(m_sem_in, timeout)) #else if (!wait(m_in, timeoutp)) @@ -316,12 +317,12 @@ bool IPCChannelEnd::recv(int timeout_ms) noexcept memcpy(recv_data, m_in->data, IPC_CHANNEL_MSG_SIZE); size -= IPC_CHANNEL_MSG_SIZE; recv_data += IPC_CHANNEL_MSG_SIZE; -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) post(m_sem_out); #else post(m_out); #endif -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) if (!wait(m_sem_in, timeout)) #else if (!wait(m_in, timeoutp)) diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 8247eddb58..a6ea47303d 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -27,10 +27,18 @@ with this program; if not, write to the Free Software Foundation, Inc., #include #if defined(_WIN32) -#include +#define IPC_CHANNEL_IMPLEMENTATION_WIN32 #elif defined(__linux__) -#include +#define IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX #else +#define IPC_CHANNEL_IMPLEMENTATION_POSIX +#endif + +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) +#include +#elif defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) +#include +#elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) #include #endif @@ -52,8 +60,7 @@ with this program; if not, write to the Free Software Foundation, Inc., struct IPCChannelBuffer { -#if !defined(_WIN32) -#if defined(__linux__) +#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) // possible values: // 0: futex is not posted. reader will check value before blocking => no // notify needed when posting @@ -61,12 +68,13 @@ struct IPCChannelBuffer // 2: futex is not posted. reader is waiting with futex syscall, and needs // to be notified std::atomic futex{0}; -#else + +#elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) pthread_cond_t cond; pthread_mutex_t mutex; bool posted = false; // protected by mutex #endif -#endif // !defined(_WIN32) + // Note: If the other side isn't acting cooperatively, they might write to // this at any times. So we must make sure to copy out the data once, and // only access that copy. @@ -89,7 +97,7 @@ struct IPCChannelStuff { virtual ~IPCChannelStuff() = default; virtual IPCChannelShared *getShared() = 0; -#ifdef _WIN32 +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) virtual HANDLE getSemA() = 0; virtual HANDLE getSemB() = 0; #endif @@ -128,7 +136,7 @@ public: inline size_t getRecvSize() const noexcept { return m_recv_size; } private: -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) IPCChannelEnd( std::unique_ptr stuff, IPCChannelBuffer *in, IPCChannelBuffer *out, @@ -154,7 +162,7 @@ private: std::unique_ptr m_stuff; IPCChannelBuffer *m_in = nullptr; IPCChannelBuffer *m_out = nullptr; -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) HANDLE m_sem_in; HANDLE m_sem_out; #endif diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index 28bde44ee7..9b6f4e5325 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -6,7 +6,7 @@ #include #include -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) || defined(_WIN32) #include #endif #include "threading/ipc_channel.h" @@ -239,13 +239,13 @@ void TestThreading::testIPCChannel() struct Stuff { IPCChannelShared shared{}; -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) HANDLE sem_a; HANDLE sem_b; #endif Stuff() { -#ifdef _WIN32 +#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 HANDLE sem_a = CreateSemaphoreA(nullptr, 0, 1, nullptr); UASSERT(sem_a != INVALID_HANDLE_VALUE); @@ -256,7 +256,7 @@ void TestThreading::testIPCChannel() ~Stuff() { -#ifdef _WIN32 +#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 CloseHandle(sem_b); CloseHandle(sem_a); #endif @@ -271,7 +271,7 @@ void TestThreading::testIPCChannel() ~IPCChannelStuffSingleProcess() override = default; IPCChannelShared *getShared() override { return &stuff->shared; } -#if defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) HANDLE getSemA() override { return stuff->sem_a; } HANDLE getSemB() override { return stuff->sem_b; } #endif From 2638dc41d1dfa0bc1e55281b8fcdf5c3d52f2cb0 Mon Sep 17 00:00:00 2001 From: Desour Date: Thu, 8 Jun 2023 23:07:46 +0200 Subject: [PATCH 07/29] use PTHREAD_PROCESS_SHARED instead of magic 1 --- src/threading/ipc_channel.cpp | 4 ++-- src/threading/ipc_channel.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 982e3831ed..dce4ed6029 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -44,9 +44,9 @@ IPCChannelBuffer::IPCChannelBuffer() goto error_condattr_init; if (pthread_mutexattr_init(&mutexattr) != 0) goto error_mutexattr_init; - if (pthread_condattr_setpshared(&condattr, 1) != 0) + if (pthread_condattr_setpshared(&condattr, PTHREAD_PROCESS_SHARED) != 0) goto error_condattr_setpshared; - if (pthread_mutexattr_setpshared(&mutexattr, 1) != 0) + if (pthread_mutexattr_setpshared(&mutexattr, PTHREAD_PROCESS_SHARED) != 0) goto error_mutexattr_setpshared; if (pthread_cond_init(&cond, &condattr) != 0) goto error_cond_init; diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index a6ea47303d..5c68173ea4 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -82,7 +82,7 @@ struct IPCChannelBuffer u8 data[IPC_CHANNEL_MSG_SIZE] = {}; IPCChannelBuffer(); - ~IPCChannelBuffer(); + ~IPCChannelBuffer(); // Note: only destruct once, i.e. in one process }; struct IPCChannelShared From 6fca066a262c1869834032453af59ad5e86b6011 Mon Sep 17 00:00:00 2001 From: Desour Date: Wed, 7 Feb 2024 12:42:12 +0100 Subject: [PATCH 08/29] tmp --- src/unittest/test_threading.cpp | 2 +- src/util/container.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index 9b6f4e5325..e84fe8a24c 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -6,7 +6,7 @@ #include #include -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) || defined(_WIN32) +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) #include #endif #include "threading/ipc_channel.h" diff --git a/src/util/container.h b/src/util/container.h index 2ab573c19d..a6fb95a10d 100644 --- a/src/util/container.h +++ b/src/util/container.h @@ -9,6 +9,7 @@ #include "exceptions.h" #include "threading/mutex_auto_lock.h" #include "threading/semaphore.h" +#include "debug.h" #include #include #include From 157f22ef95eea4075a8dc5c301d4785d3301bab1 Mon Sep 17 00:00:00 2001 From: Desour Date: Sat, 2 Mar 2024 01:38:49 +0100 Subject: [PATCH 09/29] IPCChannelStuff -> IPCChannelResources I think it should be simpler now. --- src/threading/ipc_channel.cpp | 24 ++++----- src/threading/ipc_channel.h | 88 ++++++++++++++++++++++++++------- src/unittest/test_threading.cpp | 57 ++++++++++----------- 3 files changed, 110 insertions(+), 59 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index dce4ed6029..30460a650a 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -215,27 +215,27 @@ static inline T read_once(const volatile T *var) return *var; } -IPCChannelEnd IPCChannelEnd::makeA(std::unique_ptr stuff) +IPCChannelEnd IPCChannelEnd::makeA(std::unique_ptr resources) { - IPCChannelShared *shared = stuff->getShared(); + IPCChannelShared *shared = resources->data.shared; #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - HANDLE sem_a = stuff->getSemA(); - HANDLE sem_b = stuff->getSemB(); - return IPCChannelEnd(std::move(stuff), &shared->a, &shared->b, sem_a, sem_b); + HANDLE sem_a = resources->data.sem_a; + HANDLE sem_b = resources->data.sem_b; + return IPCChannelEnd(std::move(resources), &shared->a, &shared->b, sem_a, sem_b); #else - return IPCChannelEnd(std::move(stuff), &shared->a, &shared->b); + return IPCChannelEnd(std::move(resources), &shared->a, &shared->b); #endif // !defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) } -IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr stuff) +IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr resources) { - IPCChannelShared *shared = stuff->getShared(); + IPCChannelShared *shared = resources->data.shared; #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - HANDLE sem_a = stuff->getSemA(); - HANDLE sem_b = stuff->getSemB(); - return IPCChannelEnd(std::move(stuff), &shared->b, &shared->a, sem_b, sem_a); + HANDLE sem_a = resources->data.sem_a; + HANDLE sem_b = resources->data.sem_b; + return IPCChannelEnd(std::move(resources), &shared->b, &shared->a, sem_b, sem_a); #else - return IPCChannelEnd(std::move(stuff), &shared->b, &shared->a); + return IPCChannelEnd(std::move(resources), &shared->b, &shared->a); #endif // !defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) } diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 5c68173ea4..5b0a381e4a 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -21,6 +21,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #pragma once #include "irrlichttypes.h" +#include "util/basic_macros.h" #include #include #include @@ -87,20 +88,73 @@ struct IPCChannelBuffer struct IPCChannelShared { - IPCChannelBuffer a; - IPCChannelBuffer b; + // Both ends unmap, but last deleter also deletes shared resources. + std::atomic refcount{1}; + + IPCChannelBuffer a{}; + IPCChannelBuffer b{}; }; -// opaque owner for the shared mem and stuff -// users have to implement this -struct IPCChannelStuff +struct IPCChannelResources { - virtual ~IPCChannelStuff() = default; - virtual IPCChannelShared *getShared() = 0; + // new struct, because the win32 #if is annoying + struct Data + { + IPCChannelShared *shared = nullptr; + #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - virtual HANDLE getSemA() = 0; - virtual HANDLE getSemB() = 0; + HANDLE sem_a; + HANDLE sem_b; #endif + }; + + Data data; + + // Used for previously unmanaged data_ (move semantics) + void setFirst(Data data_) + { + data = data_; + } + + // Used for data_ that is already managed by a IPCChannelResources (grab() + // semantics) + bool setSecond(Data data_) + { + if (data_.shared->refcount.fetch_add(1) == 0) { + // other end dead, can't use resources + data_.shared->refcount.fetch_sub(1); + return false; + } + data = data_; + return true; + } + + virtual void cleanupLast() noexcept = 0; + virtual void cleanupNotLast() noexcept = 0; + + void cleanup() noexcept + { + if (!data.shared) { + // No owned resources. Maybe setSecond failed. + return; + } + if (data.shared->refcount.fetch_sub(1) == 1) { + // We are last, we clean up. + cleanupLast(); + } else { + // We are not responsible for cleanup. + // Note: Shared resources may already be invalid by now. + cleanupNotLast(); + } + } + + IPCChannelResources() = default; + DISABLE_CLASS_COPY(IPCChannelResources) + + // Child should call cleanup(). + // (Parent destructor can not do this, because when it's called the child is + // already dead.) + virtual ~IPCChannelResources() = default; }; class IPCChannelEnd @@ -108,10 +162,10 @@ class IPCChannelEnd public: IPCChannelEnd() = default; - static IPCChannelEnd makeA(std::unique_ptr stuff); - static IPCChannelEnd makeB(std::unique_ptr stuff); + static IPCChannelEnd makeA(std::unique_ptr resources); + static IPCChannelEnd makeB(std::unique_ptr resources); - // If send, recv, or exchange return false (=timeout), stop using the channel. + // If send, recv, or exchange return false (=timeout), stop using the channel. <--- TODO:why? // Note: timeouts may be for receiving any response, not a whole message. bool send(const void *data, size_t size, int timeout_ms = -1) noexcept @@ -138,18 +192,18 @@ public: private: #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) IPCChannelEnd( - std::unique_ptr stuff, + std::unique_ptr resources, IPCChannelBuffer *in, IPCChannelBuffer *out, HANDLE sem_in, HANDLE sem_out) : - m_stuff(std::move(stuff)), + m_resources(std::move(resources)), m_in(in), m_out(out), m_sem_in(sem_in), m_sem_out(sem_out) {} #else IPCChannelEnd( - std::unique_ptr stuff, + std::unique_ptr resources, IPCChannelBuffer *in, IPCChannelBuffer *out) : - m_stuff(std::move(stuff)), + m_resources(std::move(resources)), m_in(in), m_out(out) {} #endif @@ -159,7 +213,7 @@ private: // returns false on timeout bool sendLarge(const void *data, size_t size, int timeout_ms) noexcept; - std::unique_ptr m_stuff; + std::unique_ptr m_resources; IPCChannelBuffer *m_in = nullptr; IPCChannelBuffer *m_out = nullptr; #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index e84fe8a24c..a8950d114b 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -236,53 +236,50 @@ void TestThreading::testTLS() void TestThreading::testIPCChannel() { - struct Stuff + struct IPCChannelResourcesSingleProcess final : public IPCChannelResources { - IPCChannelShared shared{}; -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - HANDLE sem_a; - HANDLE sem_b; -#endif - Stuff() + void cleanupLast() noexcept override { + delete data.shared; #ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 - HANDLE sem_a = CreateSemaphoreA(nullptr, 0, 1, nullptr); - UASSERT(sem_a != INVALID_HANDLE_VALUE); - - HANDLE sem_b = CreateSemaphoreA(nullptr, 0, 1, nullptr); - UASSERT(sem_b != INVALID_HANDLE_VALUE); + CloseHandle(data.sem_b); + CloseHandle(data.sem_a); #endif } - ~Stuff() + void cleanupNotLast() noexcept override { -#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 - CloseHandle(sem_b); - CloseHandle(sem_a); -#endif + // nothing to do (i.e. no unmapping needed) } + + ~IPCChannelResourcesSingleProcess() override { cleanup(); } }; - struct IPCChannelStuffSingleProcess final : public IPCChannelStuff - { - std::shared_ptr stuff; + auto resource_data = [] { + auto shared = new IPCChannelShared(); - IPCChannelStuffSingleProcess(std::shared_ptr stuff) : stuff(std::move(stuff)) {} - ~IPCChannelStuffSingleProcess() override = default; +#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 + HANDLE sem_a = CreateSemaphoreA(nullptr, 0, 1, nullptr); + UASSERT(sem_a != INVALID_HANDLE_VALUE); - IPCChannelShared *getShared() override { return &stuff->shared; } -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - HANDLE getSemA() override { return stuff->sem_a; } - HANDLE getSemB() override { return stuff->sem_b; } + HANDLE sem_b = CreateSemaphoreA(nullptr, 0, 1, nullptr); + UASSERT(sem_b != INVALID_HANDLE_VALUE); + + return IPCChannelResources::Data{shared, sem_a, sem_b}; +#else + return IPCChannelResources::Data{shared}; #endif - }; + }(); - auto stuff = std::make_shared(); + auto resources_first = std::make_unique(); + resources_first->setFirst(resource_data); - IPCChannelEnd end_a = IPCChannelEnd::makeA(std::make_unique(stuff)); + IPCChannelEnd end_a = IPCChannelEnd::makeA(std::move(resources_first)); std::thread thread_b([=] { - IPCChannelEnd end_b = IPCChannelEnd::makeB(std::make_unique(stuff)); + auto resources_second = std::make_unique(); + resources_second->setSecond(resource_data); + IPCChannelEnd end_b = IPCChannelEnd::makeB(std::move(resources_second)); for (;;) { UASSERT(end_b.recv()); From d96894e606570bde284d7365ae4a1966edd698ac Mon Sep 17 00:00:00 2001 From: Desour Date: Sat, 2 Mar 2024 21:03:27 +0100 Subject: [PATCH 10/29] make timeout explicit --- src/threading/ipc_channel.cpp | 4 ++-- src/threading/ipc_channel.h | 39 ++++++++++++++++++++++++++++----- src/unittest/test_threading.cpp | 13 ++++++----- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 30460a650a..e2ed456375 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -284,7 +284,7 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe return true; } -bool IPCChannelEnd::recv(int timeout_ms) noexcept +bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept { #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) DWORD timeout = get_timeout(timeout_ms); @@ -329,7 +329,7 @@ bool IPCChannelEnd::recv(int timeout_ms) noexcept #endif return false; } while (size > IPC_CHANNEL_MSG_SIZE); - memcpy(recv_data, m_in->data, size); + memcpy(recv_data, m_in->data, size); //TODO: memcpy volatile save? } return true; } diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 5b0a381e4a..b267856180 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -168,7 +168,9 @@ public: // If send, recv, or exchange return false (=timeout), stop using the channel. <--- TODO:why? // Note: timeouts may be for receiving any response, not a whole message. - bool send(const void *data, size_t size, int timeout_ms = -1) noexcept + // Returns false on timeout + [[nodiscard]] + bool sendWithTimeout(const void *data, size_t size, int timeout_ms) noexcept { if (size <= IPC_CHANNEL_MSG_SIZE) { sendSmall(data, size); @@ -178,14 +180,40 @@ public: } } - bool recv(int timeout_ms = -1) noexcept; - - bool exchange(const void *data, size_t size, int timeout_ms = -1) noexcept + // Same as above + void send(const void *data, size_t size) noexcept { - return send(data, size, timeout_ms) && recv(timeout_ms); + (void)sendWithTimeout(data, size, -1); + } + + // Returns false on timeout. + // Otherwise returns true, and data is available via getRecvData(). + [[nodiscard]] + bool recvWithTimeout(int timeout_ms) noexcept; + + // Same as above + void recv() noexcept + { + (void)recvWithTimeout(-1); + } + + // Returns false on timeout + // Otherwise returns true, and data is available via getRecvData(). + [[nodiscard]] + bool exchangeWithTimeout(const void *data, size_t size, int timeout_ms) noexcept + { + return sendWithTimeout(data, size, timeout_ms) + && recvWithTimeout(timeout_ms); + } + + // Same as above + void exchange(const void *data, size_t size) noexcept + { + (void)exchangeWithTimeout(data, size, -1); } // Get the content of the last received message + // TODO: u8 *, or string_view? inline const void *getRecvData() const noexcept { return m_large_recv.data(); } inline size_t getRecvSize() const noexcept { return m_recv_size; } @@ -208,6 +236,7 @@ private: {} #endif + // TODO: u8 *, or string_view? void sendSmall(const void *data, size_t size) noexcept; // returns false on timeout diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index a8950d114b..0918df732f 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -282,25 +282,26 @@ void TestThreading::testIPCChannel() IPCChannelEnd end_b = IPCChannelEnd::makeB(std::move(resources_second)); for (;;) { - UASSERT(end_b.recv()); - UASSERT(end_b.send(end_b.getRecvData(), end_b.getRecvSize())); + UASSERT(end_b.recvWithTimeout(-1)); + UASSERT(end_b.sendWithTimeout(end_b.getRecvData(), end_b.getRecvSize(), -1)); if (end_b.getRecvSize() == 0) break; } }); char buf[20000] = {}; - for (int i = sizeof(buf); i > 0; i -= 1000) { + for (int i = sizeof(buf); i > 0; i -= 100) { buf[i - 1] = 123; - UASSERT(end_a.exchange(buf, i)); + UASSERT(end_a.exchangeWithTimeout(buf, i, -1)); UASSERTEQ(int, end_a.getRecvSize(), i); UASSERTEQ(int, ((const char *)end_a.getRecvData())[i - 1], 123); } - UASSERT(end_a.exchange(buf, 0)); + UASSERT(end_a.exchangeWithTimeout(buf, 0, -1)); UASSERTEQ(int, end_a.getRecvSize(), 0); thread_b.join(); - UASSERT(!end_a.exchange(buf, 0, 1000)); + // other side dead ==> should time out + UASSERT(!end_a.exchangeWithTimeout(buf, 0, 200)); } From 22b4f3becf140d2a36eae6329cec30039cd2c2d9 Mon Sep 17 00:00:00 2001 From: Desour Date: Sat, 2 Mar 2024 21:58:18 +0100 Subject: [PATCH 11/29] add benchmark --- src/benchmark/CMakeLists.txt | 3 +- src/benchmark/benchmark_ipc_channel.cpp | 95 +++++++++++++++++++++++++ src/threading/ipc_channel.cpp | 6 +- src/unittest/test_threading.cpp | 2 + 4 files changed, 103 insertions(+), 3 deletions(-) create mode 100644 src/benchmark/benchmark_ipc_channel.cpp diff --git a/src/benchmark/CMakeLists.txt b/src/benchmark/CMakeLists.txt index e8150848a0..2ecab382dd 100644 --- a/src/benchmark/CMakeLists.txt +++ b/src/benchmark/CMakeLists.txt @@ -1,10 +1,11 @@ set (BENCHMARK_SRCS ${CMAKE_CURRENT_SOURCE_DIR}/benchmark.cpp ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_activeobjectmgr.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_ipc_channel.cpp ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_lighting.cpp - ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_serialize.cpp ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_mapblock.cpp ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_mapmodify.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_serialize.cpp ${CMAKE_CURRENT_SOURCE_DIR}/benchmark_sha.cpp PARENT_SCOPE) diff --git a/src/benchmark/benchmark_ipc_channel.cpp b/src/benchmark/benchmark_ipc_channel.cpp new file mode 100644 index 0000000000..86d8fde6e0 --- /dev/null +++ b/src/benchmark/benchmark_ipc_channel.cpp @@ -0,0 +1,95 @@ +/* +Minetest +Copyright (C) 2024 DS + +This program is free software; you can redistribute it and/or modify +it under the terms of the GNU Lesser General Public License as published by +the Free Software Foundation; either version 2.1 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public License along +with this program; if not, write to the Free Software Foundation, Inc., +51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +*/ + +#include "benchmark_setup.h" +#include "threading/ipc_channel.h" +#include + +#include "log.h" + +TEST_CASE("benchmark_ipc_channel") +{ + // same as in test_threading.cpp (TODO: remove duplication) + struct IPCChannelResourcesSingleProcess final : public IPCChannelResources + { + void cleanupLast() noexcept override + { + delete data.shared; +#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 + CloseHandle(data.sem_b); + CloseHandle(data.sem_a); +#endif + } + + void cleanupNotLast() noexcept override + { + // nothing to do (i.e. no unmapping needed) + } + + ~IPCChannelResourcesSingleProcess() override { cleanup(); } + }; + + auto resource_data = [] { + auto shared = new IPCChannelShared(); + +#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 + HANDLE sem_a = CreateSemaphoreA(nullptr, 0, 1, nullptr); + REQUIRE(sem_a != INVALID_HANDLE_VALUE); + + HANDLE sem_b = CreateSemaphoreA(nullptr, 0, 1, nullptr); + REQUIRE(sem_b != INVALID_HANDLE_VALUE); + + return IPCChannelResources::Data{shared, sem_a, sem_b}; +#else + return IPCChannelResources::Data{shared}; +#endif + }(); + + auto resources_first = std::make_unique(); + resources_first->setFirst(resource_data); + + IPCChannelEnd end_a = IPCChannelEnd::makeA(std::move(resources_first)); + + // echos back messages. stops if "" is sent + std::thread thread_b([=] { + auto resources_second = std::make_unique(); + resources_second->setSecond(resource_data); + IPCChannelEnd end_b = IPCChannelEnd::makeB(std::move(resources_second)); + + for (;;) { + end_b.recv(); + end_b.send(end_b.getRecvData(), end_b.getRecvSize()); + if (end_b.getRecvSize() == 0) + break; + } + }); + + BENCHMARK("simple_call", i) { + char buf[16] = {}; + buf[i & 0xf] = i; + end_a.exchange(buf, 16); + return reinterpret_cast(end_a.getRecvData())[i & 0xf]; + }; + + // stop thread_b + end_a.exchange(nullptr, 0); + REQUIRE(end_a.getRecvSize() == 0); + + thread_b.join(); +} diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index e2ed456375..0cb2ce29b2 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -242,7 +242,8 @@ IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr resource void IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept { write_once(&m_out->size, size); - memcpy(m_out->data, data, size); + if (size != 0) + memcpy(m_out->data, data, size); #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) post(m_sem_out); #else @@ -275,7 +276,8 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe size -= IPC_CHANNEL_MSG_SIZE; data = (u8 *)data + IPC_CHANNEL_MSG_SIZE; } while (size > IPC_CHANNEL_MSG_SIZE); - memcpy(m_out->data, data, size); + if (size != 0) + memcpy(m_out->data, data, size); #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) post(m_sem_out); #else diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index 0918df732f..2d82e4353b 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -276,6 +276,7 @@ void TestThreading::testIPCChannel() IPCChannelEnd end_a = IPCChannelEnd::makeA(std::move(resources_first)); + // echos back messages. stops if "" is sent std::thread thread_b([=] { auto resources_second = std::make_unique(); resources_second->setSecond(resource_data); @@ -297,6 +298,7 @@ void TestThreading::testIPCChannel() UASSERTEQ(int, ((const char *)end_a.getRecvData())[i - 1], 123); } + // stop thread_b UASSERT(end_a.exchangeWithTimeout(buf, 0, -1)); UASSERTEQ(int, end_a.getRecvSize(), 0); From 668d22f39f443020f621efc8e6a56f01dae4c23a Mon Sep 17 00:00:00 2001 From: Desour Date: Fri, 4 Oct 2024 03:19:06 +0200 Subject: [PATCH 12/29] some include fixes --- src/benchmark/benchmark_ipc_channel.cpp | 16 ++++++++++++---- src/threading/ipc_channel.cpp | 2 +- src/threading/ipc_channel.h | 3 +-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/benchmark/benchmark_ipc_channel.cpp b/src/benchmark/benchmark_ipc_channel.cpp index 86d8fde6e0..4232739e19 100644 --- a/src/benchmark/benchmark_ipc_channel.cpp +++ b/src/benchmark/benchmark_ipc_channel.cpp @@ -17,12 +17,10 @@ with this program; if not, write to the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ -#include "benchmark_setup.h" +#include "catch.h" #include "threading/ipc_channel.h" #include -#include "log.h" - TEST_CASE("benchmark_ipc_channel") { // same as in test_threading.cpp (TODO: remove duplication) @@ -80,13 +78,23 @@ TEST_CASE("benchmark_ipc_channel") } }); - BENCHMARK("simple_call", i) { + BENCHMARK("simple_call_1", i) { char buf[16] = {}; buf[i & 0xf] = i; end_a.exchange(buf, 16); return reinterpret_cast(end_a.getRecvData())[i & 0xf]; }; + BENCHMARK("simple_call_1000", i) { + char buf[16] = {}; + buf[i & 0xf] = i; + for (int k = 0; k < 1000; ++k) { + buf[0] = k & 0xff; + end_a.exchange(buf, 16); + } + return reinterpret_cast(end_a.getRecvData())[i & 0xf]; + }; + // stop thread_b end_a.exchange(nullptr, 0); REQUIRE(end_a.getRecvSize() == 0); diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 0cb2ce29b2..bcd7e68307 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -24,10 +24,10 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "porting.h" #include #include +#include #if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) #include -#include #include #include #if defined(__i386__) || defined(__x86_64__) diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index b267856180..843ff41e9f 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -26,6 +26,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include #include #include +#include #if defined(_WIN32) #define IPC_CHANNEL_IMPLEMENTATION_WIN32 @@ -37,8 +38,6 @@ with this program; if not, write to the Free Software Foundation, Inc., #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) #include -#elif defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) -#include #elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) #include #endif From 76b5d120e8ceb9664b6a4b6cfee3039ffacdcfd0 Mon Sep 17 00:00:00 2001 From: Desour Date: Sat, 5 Oct 2024 09:17:13 +0200 Subject: [PATCH 13/29] use weaker atomic operations (havent noticed a performance improvement. but it's nice having it correct) --- src/threading/ipc_channel.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index bcd7e68307..3a9f244f60 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -119,8 +119,13 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept // try busy waiting for (int i = 0; i < 100; i++) { // posted? - if (buf->futex.exchange(0) == 1) - return true; // yes + if (buf->futex.load(std::memory_order_acquire) == 1) { + // yes + // reset it. (relaxed ordering is sufficient, because the other thread + // does not need to see the side effects we did before writing 0) + buf->futex.store(0, std::memory_order_relaxed); + return true; + } #if defined(__i386__) || defined(__x86_64__) busy_wait(40); #else @@ -130,9 +135,9 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept // wait with futex while (true) { // write 2 to show that we're futexing - if (buf->futex.exchange(2) == 1) { - // futex was posted => change 2 to 0 (or 1 to 1) - buf->futex.fetch_and(1); + if (buf->futex.exchange(2, std::memory_order_acq_rel) == 1) { + // it was posted in the meantime + buf->futex.store(0, std::memory_order_relaxed); return true; } int s = futex(&buf->futex, FUTEX_WAIT, 2, timeout, nullptr, 0); @@ -164,7 +169,7 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept static void post(IPCChannelBuffer *buf) noexcept { #if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) - if (buf->futex.exchange(1) == 2) { + if (buf->futex.exchange(1, std::memory_order_acq_rel) == 2) { // 2 means reader needs to be notified int s = futex(&buf->futex, FUTEX_WAKE, 1, nullptr, nullptr, 0); if (s == -1) { From 47c6f94e877e239b1057b8d8dec0075818e8078e Mon Sep 17 00:00:00 2001 From: Desour Date: Sat, 5 Oct 2024 10:23:46 +0200 Subject: [PATCH 14/29] add comment for memcpy, but don't replace it (it's likely fine) --- src/threading/ipc_channel.cpp | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 3a9f244f60..8bd07f252d 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -269,15 +269,13 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe memcpy(m_out->data, data, IPC_CHANNEL_MSG_SIZE); #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) post(m_sem_out); + if (!wait(m_sem_in, timeout)) + return false; #else post(m_out); -#endif -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - if (!wait(m_sem_in, timeout)) -#else if (!wait(m_in, timeoutp)) -#endif return false; +#endif size -= IPC_CHANNEL_MSG_SIZE; data = (u8 *)data + IPC_CHANNEL_MSG_SIZE; } while (size > IPC_CHANNEL_MSG_SIZE); @@ -295,22 +293,25 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept { #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) DWORD timeout = get_timeout(timeout_ms); + if (!wait(m_sem_in, timeout)) + return false; #else struct timespec timeout; struct timespec *timeoutp = set_timespec(&timeout, timeout_ms); -#endif -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - if (!wait(m_sem_in, timeout)) -#else if (!wait(m_in, timeoutp)) -#endif return false; +#endif size_t size = read_once(&m_in->size); m_recv_size = size; + // Note about memcpy: If the other thread is evil, it might change the contents + // of the memory while it's memcopied. We're assuming here that memcpy doesn't + // cause vulnerabilities due to this. if (size <= IPC_CHANNEL_MSG_SIZE) { - // m_large_recv.size() is always >= IPC_CHANNEL_MSG_SIZE + // small msg + // (m_large_recv.size() is always >= IPC_CHANNEL_MSG_SIZE) memcpy(m_large_recv.data(), m_in->data, size); } else { + // large msg try { m_large_recv.resize(size); } catch (...) { @@ -326,17 +327,15 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept recv_data += IPC_CHANNEL_MSG_SIZE; #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) post(m_sem_out); + if (!wait(m_sem_in, timeout)) + return false; #else post(m_out); -#endif -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - if (!wait(m_sem_in, timeout)) -#else if (!wait(m_in, timeoutp)) -#endif return false; +#endif } while (size > IPC_CHANNEL_MSG_SIZE); - memcpy(recv_data, m_in->data, size); //TODO: memcpy volatile save? + memcpy(recv_data, m_in->data, size); } return true; } From 9b6244c6c2132db082e2079730c3f7ac74c12a3d Mon Sep 17 00:00:00 2001 From: Desour Date: Sat, 5 Oct 2024 11:43:01 +0200 Subject: [PATCH 15/29] improve code readbility --- src/threading/ipc_channel.cpp | 168 +++++++++++++++++----------------- src/threading/ipc_channel.h | 53 ++++++----- 2 files changed, 114 insertions(+), 107 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 8bd07f252d..c57a77310c 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -20,8 +20,6 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "ipc_channel.h" #include "debug.h" -#include "exceptions.h" -#include "porting.h" #include #include #include @@ -30,9 +28,22 @@ with this program; if not, write to the Free Software Foundation, Inc., #include #include #include -#if defined(__i386__) || defined(__x86_64__) -#include #endif + +#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) && (defined(__i386__) || defined(__x86_64__)) +#include + +#define HAVE_BUSY_WAIT 1 + +[[maybe_unused]] +static void busy_wait(int n) noexcept +{ + for (int i = 0; i < n; i++) + _mm_pause(); +} + +#else +#define HAVE_BUSY_WAIT 0 #endif IPCChannelBuffer::IPCChannelBuffer() @@ -91,17 +102,7 @@ static void post(HANDLE sem) FATAL_ERROR("ReleaseSemaphore failed unexpectedly"); } -#else - -#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) - -#if defined(__i386__) || defined(__x86_64__) -static void busy_wait(int n) noexcept -{ - for (int i = 0; i < n; i++) - _mm_pause(); -} -#endif // defined(__i386__) || defined(__x86_64__) +#elif defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) static int futex(std::atomic *uaddr, int futex_op, u32 val, const struct timespec *timeout, u32 *uaddr2, u32 val3) noexcept @@ -109,13 +110,10 @@ static int futex(std::atomic *uaddr, int futex_op, u32 val, return syscall(SYS_futex, uaddr, futex_op, val, timeout, uaddr2, val3); } -#endif // defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) - -// timeout: relative on linux, and absolute on other posix +// timeout is relative // returns false on timeout static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept { -#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) // try busy waiting for (int i = 0; i < 100; i++) { // posted? @@ -126,10 +124,10 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept buf->futex.store(0, std::memory_order_relaxed); return true; } -#if defined(__i386__) || defined(__x86_64__) +#if HAVE_BUSY_WAIT busy_wait(40); #else - break; // Busy wait not implemented + break; #endif } // wait with futex @@ -151,7 +149,27 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept } } } +} + +static void post(IPCChannelBuffer *buf) noexcept +{ + if (buf->futex.exchange(1, std::memory_order_acq_rel) == 2) { + // 2 means reader needs to be notified + int s = futex(&buf->futex, FUTEX_WAKE, 1, nullptr, nullptr, 0); + if (s == -1) { + std::string errmsg = std::string("FUTEX_WAKE failed unexpectedly: ") + + std::strerror(errno); + FATAL_ERROR(errmsg.c_str()); + } + } +} + #elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) + +// timeout is absolute +// returns false on timeout +static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept +{ bool timed_out = false; pthread_mutex_lock(&buf->mutex); if (!buf->posted) { @@ -163,30 +181,38 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept buf->posted = false; pthread_mutex_unlock(&buf->mutex); return !timed_out; -#endif } static void post(IPCChannelBuffer *buf) noexcept { -#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) - if (buf->futex.exchange(1, std::memory_order_acq_rel) == 2) { - // 2 means reader needs to be notified - int s = futex(&buf->futex, FUTEX_WAKE, 1, nullptr, nullptr, 0); - if (s == -1) { - std::string errmsg = std::string("FUTEX_WAKE failed unexpectedly: ") - + std::strerror(errno); - FATAL_ERROR(errmsg.c_str()); - } - } -#elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) pthread_mutex_lock(&buf->mutex); buf->posted = true; pthread_cond_broadcast(&buf->cond); pthread_mutex_unlock(&buf->mutex); -#endif } -#endif // !defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) +#endif + +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) +static bool wait_in(IPCChannelEnd::Dir *dir, DWORD timeout) +{ + return wait(dir->sem_in, timeout); +} +#else +static bool wait_in(IPCChannelEnd::Dir *dir, const struct timespec *timeout) +{ + return wait(dir->buf_in, timeout); +} +#endif + +static void post_out(IPCChannelEnd::Dir *dir) +{ +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) + post(dir->sem_out); +#else + post(dir->buf_out); +#endif +} #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) static DWORD get_timeout(int timeout_ms) @@ -226,9 +252,9 @@ IPCChannelEnd IPCChannelEnd::makeA(std::unique_ptr resource #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) HANDLE sem_a = resources->data.sem_a; HANDLE sem_b = resources->data.sem_b; - return IPCChannelEnd(std::move(resources), &shared->a, &shared->b, sem_a, sem_b); + return IPCChannelEnd(std::move(resources), Dir{&shared->a, &shared->b, sem_a, sem_b}); #else - return IPCChannelEnd(std::move(resources), &shared->a, &shared->b); + return IPCChannelEnd(std::move(resources), Dir{&shared->a, &shared->b}); #endif // !defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) } @@ -238,22 +264,18 @@ IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr resource #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) HANDLE sem_a = resources->data.sem_a; HANDLE sem_b = resources->data.sem_b; - return IPCChannelEnd(std::move(resources), &shared->b, &shared->a, sem_b, sem_a); + return IPCChannelEnd(std::move(resources), Dir{&shared->b, &shared->a, sem_b, sem_a}); #else - return IPCChannelEnd(std::move(resources), &shared->b, &shared->a); + return IPCChannelEnd(std::move(resources), Dir{&shared->b, &shared->a}); #endif // !defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) } void IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept { - write_once(&m_out->size, size); + write_once(&m_dir.buf_out->size, size); if (size != 0) - memcpy(m_out->data, data, size); -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - post(m_sem_out); -#else - post(m_out); -#endif + memcpy(m_dir.buf_out->data, data, size); + post_out(&m_dir); } bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noexcept @@ -261,31 +283,21 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) DWORD timeout = get_timeout(timeout_ms); #else - struct timespec timeout; - struct timespec *timeoutp = set_timespec(&timeout, timeout_ms); + struct timespec timeout_s; + struct timespec *timeout = set_timespec(&timeout_s, timeout_ms); #endif - write_once(&m_out->size, size); + write_once(&m_dir.buf_out->size, size); do { - memcpy(m_out->data, data, IPC_CHANNEL_MSG_SIZE); -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - post(m_sem_out); - if (!wait(m_sem_in, timeout)) + memcpy(m_dir.buf_out->data, data, IPC_CHANNEL_MSG_SIZE); + post_out(&m_dir); + if (!wait_in(&m_dir, timeout)) // TODO: always relative timeout, or always absolute return false; -#else - post(m_out); - if (!wait(m_in, timeoutp)) - return false; -#endif size -= IPC_CHANNEL_MSG_SIZE; data = (u8 *)data + IPC_CHANNEL_MSG_SIZE; } while (size > IPC_CHANNEL_MSG_SIZE); if (size != 0) - memcpy(m_out->data, data, size); -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - post(m_sem_out); -#else - post(m_out); -#endif + memcpy(m_dir.buf_out->data, data, size); + post_out(&m_dir); return true; } @@ -293,15 +305,13 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept { #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) DWORD timeout = get_timeout(timeout_ms); - if (!wait(m_sem_in, timeout)) - return false; #else - struct timespec timeout; - struct timespec *timeoutp = set_timespec(&timeout, timeout_ms); - if (!wait(m_in, timeoutp)) - return false; + struct timespec timeout_s; + struct timespec *timeout = set_timespec(&timeout_s, timeout_ms); #endif - size_t size = read_once(&m_in->size); + if (!wait_in(&m_dir, timeout)) + return false; + size_t size = read_once(&m_dir.buf_in->size); m_recv_size = size; // Note about memcpy: If the other thread is evil, it might change the contents // of the memory while it's memcopied. We're assuming here that memcpy doesn't @@ -309,7 +319,7 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept if (size <= IPC_CHANNEL_MSG_SIZE) { // small msg // (m_large_recv.size() is always >= IPC_CHANNEL_MSG_SIZE) - memcpy(m_large_recv.data(), m_in->data, size); + memcpy(m_large_recv.data(), m_dir.buf_in->data, size); } else { // large msg try { @@ -322,20 +332,14 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept } u8 *recv_data = m_large_recv.data(); do { - memcpy(recv_data, m_in->data, IPC_CHANNEL_MSG_SIZE); + memcpy(recv_data, m_dir.buf_in->data, IPC_CHANNEL_MSG_SIZE); size -= IPC_CHANNEL_MSG_SIZE; recv_data += IPC_CHANNEL_MSG_SIZE; -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - post(m_sem_out); - if (!wait(m_sem_in, timeout)) + post_out(&m_dir); + if (!wait_in(&m_dir, timeout)) return false; -#else - post(m_out); - if (!wait(m_in, timeoutp)) - return false; -#endif } while (size > IPC_CHANNEL_MSG_SIZE); - memcpy(recv_data, m_in->data, size); + memcpy(recv_data, m_dir.buf_in->data, size); } return true; } diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 843ff41e9f..c701947e4e 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -23,8 +23,6 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "irrlichttypes.h" #include "util/basic_macros.h" #include -#include -#include #include #include @@ -85,6 +83,7 @@ struct IPCChannelBuffer ~IPCChannelBuffer(); // Note: only destruct once, i.e. in one process }; +// Data in shared memory struct IPCChannelShared { // Both ends unmap, but last deleter also deletes shared resources. @@ -94,6 +93,18 @@ struct IPCChannelShared IPCChannelBuffer b{}; }; +struct IPCChannelDirection +{ + IPCChannelBuffer *buf_in; + IPCChannelBuffer *buf_out; +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) + HANDLE sem_in; + HANDLE sem_out; +#endif +}; + +// Each end holds this. One is A, one is B. +// Implementors of this struct decide how to allocate buffers (i.e. malloc or mmap). struct IPCChannelResources { // new struct, because the win32 #if is annoying @@ -115,7 +126,7 @@ struct IPCChannelResources data = data_; } - // Used for data_ that is already managed by a IPCChannelResources (grab() + // Used for data_ that is already managed by an IPCChannelResources (grab() // semantics) bool setSecond(Data data_) { @@ -159,6 +170,17 @@ struct IPCChannelResources class IPCChannelEnd { public: + // Direction. References into IPCChannelResources. + struct Dir + { + IPCChannelBuffer *buf_in = nullptr; + IPCChannelBuffer *buf_out = nullptr; +#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) + HANDLE sem_in; + HANDLE sem_out; +#endif + }; + IPCChannelEnd() = default; static IPCChannelEnd makeA(std::unique_ptr resources); @@ -217,23 +239,9 @@ public: inline size_t getRecvSize() const noexcept { return m_recv_size; } private: -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - IPCChannelEnd( - std::unique_ptr resources, - IPCChannelBuffer *in, IPCChannelBuffer *out, - HANDLE sem_in, HANDLE sem_out) : - m_resources(std::move(resources)), - m_in(in), m_out(out), - m_sem_in(sem_in), m_sem_out(sem_out) + IPCChannelEnd(std::unique_ptr resources, Dir dir) : + m_resources(std::move(resources)), m_dir(dir) {} -#else - IPCChannelEnd( - std::unique_ptr resources, - IPCChannelBuffer *in, IPCChannelBuffer *out) : - m_resources(std::move(resources)), - m_in(in), m_out(out) - {} -#endif // TODO: u8 *, or string_view? void sendSmall(const void *data, size_t size) noexcept; @@ -242,12 +250,7 @@ private: bool sendLarge(const void *data, size_t size, int timeout_ms) noexcept; std::unique_ptr m_resources; - IPCChannelBuffer *m_in = nullptr; - IPCChannelBuffer *m_out = nullptr; -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - HANDLE m_sem_in; - HANDLE m_sem_out; -#endif + Dir m_dir; size_t m_recv_size = 0; // we always copy from the shared buffer into this // (this buffer only grows) From d6dd5b4d4fa5e1f4aaf65245692ed719c31188bd Mon Sep 17 00:00:00 2001 From: Desour Date: Sat, 5 Oct 2024 12:01:04 +0200 Subject: [PATCH 16/29] make timeout behaviour consistent --- src/threading/ipc_channel.cpp | 72 +++++++++++++++-------------------- 1 file changed, 30 insertions(+), 42 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index c57a77310c..2e5eafe2f9 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -20,6 +20,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "ipc_channel.h" #include "debug.h" +#include "porting.h" #include #include #include @@ -193,17 +194,34 @@ static void post(IPCChannelBuffer *buf) noexcept #endif +static bool wait_in(IPCChannelEnd::Dir *dir, int timeout_ms, u64 t0) +{ #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) -static bool wait_in(IPCChannelEnd::Dir *dir, DWORD timeout) -{ + DWORD timeout = INFINITE; + if (timeout_ms >= 0) { + timeout = (DWORD)timeout_ms; + timeout_msu -= porting::getTimeMs() - t0; // Relative time + } return wait(dir->sem_in, timeout); -} + #else -static bool wait_in(IPCChannelEnd::Dir *dir, const struct timespec *timeout) -{ - return wait(dir->buf_in, timeout); -} + struct timespec timeout; + struct timespec *timeoutp = nullptr; + if (timeout_ms >= 0) { + u64 timeout_msu = timeout_ms; +#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) + timeout_msu -= porting::getTimeMs() - t0; // Relative time +#elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) + timeout_msu += t0; // Absolute time #endif + timeout.tv_sec = timeout_msu / 1000; + timeout.tv_nsec = timeout_msu % 1000 * 1000000UL; + timeoutp = &timeout; + } + + return wait(dir->buf_in, timeoutp); +#endif +} static void post_out(IPCChannelEnd::Dir *dir) { @@ -214,26 +232,6 @@ static void post_out(IPCChannelEnd::Dir *dir) #endif } -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) -static DWORD get_timeout(int timeout_ms) -{ - return timeout_ms < 0 ? INFINITE : (DWORD)timeout_ms; -} -#elif defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) || defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) -static struct timespec *set_timespec(struct timespec *ts, int ms) -{ - if (ms < 0) - return nullptr; - u64 msu = ms; -#if defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) - msu += porting::getTimeMs(); // Absolute time -#endif - ts->tv_sec = msu / 1000; - ts->tv_nsec = msu % 1000 * 1000000UL; - return ts; -} -#endif - template static inline void write_once(volatile T *var, const T val) { @@ -280,17 +278,12 @@ void IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noexcept { -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - DWORD timeout = get_timeout(timeout_ms); -#else - struct timespec timeout_s; - struct timespec *timeout = set_timespec(&timeout_s, timeout_ms); -#endif + u64 t0 = porting::getTimeMs(); write_once(&m_dir.buf_out->size, size); do { memcpy(m_dir.buf_out->data, data, IPC_CHANNEL_MSG_SIZE); post_out(&m_dir); - if (!wait_in(&m_dir, timeout)) // TODO: always relative timeout, or always absolute + if (!wait_in(&m_dir, timeout_ms, t0)) return false; size -= IPC_CHANNEL_MSG_SIZE; data = (u8 *)data + IPC_CHANNEL_MSG_SIZE; @@ -303,13 +296,8 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept { -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - DWORD timeout = get_timeout(timeout_ms); -#else - struct timespec timeout_s; - struct timespec *timeout = set_timespec(&timeout_s, timeout_ms); -#endif - if (!wait_in(&m_dir, timeout)) + u64 t0 = porting::getTimeMs(); + if (!wait_in(&m_dir, timeout_ms, t0)) return false; size_t size = read_once(&m_dir.buf_in->size); m_recv_size = size; @@ -336,7 +324,7 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept size -= IPC_CHANNEL_MSG_SIZE; recv_data += IPC_CHANNEL_MSG_SIZE; post_out(&m_dir); - if (!wait_in(&m_dir, timeout)) + if (!wait_in(&m_dir, timeout_ms, t0)) return false; } while (size > IPC_CHANNEL_MSG_SIZE); memcpy(recv_data, m_dir.buf_in->data, size); From 37358bd8fee218abae2457a09e7cf2a00cb3d0e9 Mon Sep 17 00:00:00 2001 From: Desour Date: Sat, 5 Oct 2024 12:18:24 +0200 Subject: [PATCH 17/29] timeout_ms_abs --- src/threading/ipc_channel.cpp | 142 ++++++++++++++++++++-------------- 1 file changed, 82 insertions(+), 60 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 2e5eafe2f9..922dde34c0 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -47,48 +47,6 @@ static void busy_wait(int n) noexcept #define HAVE_BUSY_WAIT 0 #endif -IPCChannelBuffer::IPCChannelBuffer() -{ -#if defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) - pthread_condattr_t condattr; - pthread_mutexattr_t mutexattr; - if (pthread_condattr_init(&condattr) != 0) - goto error_condattr_init; - if (pthread_mutexattr_init(&mutexattr) != 0) - goto error_mutexattr_init; - if (pthread_condattr_setpshared(&condattr, PTHREAD_PROCESS_SHARED) != 0) - goto error_condattr_setpshared; - if (pthread_mutexattr_setpshared(&mutexattr, PTHREAD_PROCESS_SHARED) != 0) - goto error_mutexattr_setpshared; - if (pthread_cond_init(&cond, &condattr) != 0) - goto error_cond_init; - if (pthread_mutex_init(&mutex, &mutexattr) != 0) - goto error_mutex_init; - pthread_mutexattr_destroy(&mutexattr); - pthread_condattr_destroy(&condattr); - return; - -error_mutex_init: - pthread_cond_destroy(&cond); -error_cond_init: -error_mutexattr_setpshared: -error_condattr_setpshared: - pthread_mutexattr_destroy(&mutexattr); -error_mutexattr_init: - pthread_condattr_destroy(&condattr); -error_condattr_init: - throw BaseException("Unable to initialize IPCChannelBuffer"); -#endif // defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) -} - -IPCChannelBuffer::~IPCChannelBuffer() -{ -#if defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) - pthread_mutex_destroy(&mutex); - pthread_cond_destroy(&cond); -#endif // defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) -} - #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) // returns false on timeout @@ -194,28 +152,37 @@ static void post(IPCChannelBuffer *buf) noexcept #endif -static bool wait_in(IPCChannelEnd::Dir *dir, int timeout_ms, u64 t0) +// timeout_ms_abs: absolute timeout (using porting::getTimeMs()), or 0 for no timeout +// returns false on timeout +static bool wait_in(IPCChannelEnd::Dir *dir, u64 timeout_ms_abs) { #if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) + // Relative time DWORD timeout = INFINITE; - if (timeout_ms >= 0) { - timeout = (DWORD)timeout_ms; - timeout_msu -= porting::getTimeMs() - t0; // Relative time + if (timeout_ms_abs > 0) { + u64 tnow = porting::getTimeMs(); + if (tnow > timeout_ms_abs) + return false; + timeout = (DWORD)(timeout_ms_abs - tnow); } return wait(dir->sem_in, timeout); #else struct timespec timeout; struct timespec *timeoutp = nullptr; - if (timeout_ms >= 0) { - u64 timeout_msu = timeout_ms; + if (timeout_ms_abs > 0) { #if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) - timeout_msu -= porting::getTimeMs() - t0; // Relative time + // Relative time + u64 tnow = porting::getTimeMs(); + if (tnow > timeout_ms_abs) + return false; + u64 timeout_ms = timeout_ms_abs - tnow; #elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) - timeout_msu += t0; // Absolute time + // Absolute time + u64 timeout_msu = timeout_ms_abs; #endif - timeout.tv_sec = timeout_msu / 1000; - timeout.tv_nsec = timeout_msu % 1000 * 1000000UL; + timeout.tv_sec = timeout_ms / 1000; + timeout.tv_nsec = timeout_ms % 1000 * 1000000UL; timeoutp = &timeout; } @@ -244,6 +211,48 @@ static inline T read_once(const volatile T *var) return *var; } +IPCChannelBuffer::IPCChannelBuffer() +{ +#if defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) + pthread_condattr_t condattr; + pthread_mutexattr_t mutexattr; + if (pthread_condattr_init(&condattr) != 0) + goto error_condattr_init; + if (pthread_mutexattr_init(&mutexattr) != 0) + goto error_mutexattr_init; + if (pthread_condattr_setpshared(&condattr, PTHREAD_PROCESS_SHARED) != 0) + goto error_condattr_setpshared; + if (pthread_mutexattr_setpshared(&mutexattr, PTHREAD_PROCESS_SHARED) != 0) + goto error_mutexattr_setpshared; + if (pthread_cond_init(&cond, &condattr) != 0) + goto error_cond_init; + if (pthread_mutex_init(&mutex, &mutexattr) != 0) + goto error_mutex_init; + pthread_mutexattr_destroy(&mutexattr); + pthread_condattr_destroy(&condattr); + return; + +error_mutex_init: + pthread_cond_destroy(&cond); +error_cond_init: +error_mutexattr_setpshared: +error_condattr_setpshared: + pthread_mutexattr_destroy(&mutexattr); +error_mutexattr_init: + pthread_condattr_destroy(&condattr); +error_condattr_init: + throw BaseException("Unable to initialize IPCChannelBuffer"); +#endif // defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) +} + +IPCChannelBuffer::~IPCChannelBuffer() +{ +#if defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) + pthread_mutex_destroy(&mutex); + pthread_cond_destroy(&cond); +#endif // defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) +} + IPCChannelEnd IPCChannelEnd::makeA(std::unique_ptr resources) { IPCChannelShared *shared = resources->data.shared; @@ -271,43 +280,56 @@ IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr resource void IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept { write_once(&m_dir.buf_out->size, size); + if (size != 0) memcpy(m_dir.buf_out->data, data, size); + post_out(&m_dir); } bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noexcept { - u64 t0 = porting::getTimeMs(); + u64 timeout_ms_abs = timeout_ms < 0 ? 0 : porting::getTimeMs() + timeout_ms; + write_once(&m_dir.buf_out->size, size); + do { memcpy(m_dir.buf_out->data, data, IPC_CHANNEL_MSG_SIZE); post_out(&m_dir); - if (!wait_in(&m_dir, timeout_ms, t0)) + + if (!wait_in(&m_dir, timeout_ms_abs)) return false; + size -= IPC_CHANNEL_MSG_SIZE; data = (u8 *)data + IPC_CHANNEL_MSG_SIZE; } while (size > IPC_CHANNEL_MSG_SIZE); + if (size != 0) memcpy(m_dir.buf_out->data, data, size); post_out(&m_dir); + return true; } bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept { - u64 t0 = porting::getTimeMs(); - if (!wait_in(&m_dir, timeout_ms, t0)) - return false; - size_t size = read_once(&m_dir.buf_in->size); - m_recv_size = size; // Note about memcpy: If the other thread is evil, it might change the contents // of the memory while it's memcopied. We're assuming here that memcpy doesn't // cause vulnerabilities due to this. + + u64 timeout_ms_abs = timeout_ms < 0 ? 0 : porting::getTimeMs() + timeout_ms; + + if (!wait_in(&m_dir, timeout_ms_abs)) + return false; + + size_t size = read_once(&m_dir.buf_in->size); + m_recv_size = size; + if (size <= IPC_CHANNEL_MSG_SIZE) { // small msg // (m_large_recv.size() is always >= IPC_CHANNEL_MSG_SIZE) memcpy(m_large_recv.data(), m_dir.buf_in->data, size); + } else { // large msg try { @@ -324,7 +346,7 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept size -= IPC_CHANNEL_MSG_SIZE; recv_data += IPC_CHANNEL_MSG_SIZE; post_out(&m_dir); - if (!wait_in(&m_dir, timeout_ms, t0)) + if (!wait_in(&m_dir, timeout_ms_abs)) return false; } while (size > IPC_CHANNEL_MSG_SIZE); memcpy(recv_data, m_dir.buf_in->data, size); From 2af85d05fa10d17d3b2c1f55b01a50b05b752a79 Mon Sep 17 00:00:00 2001 From: Desour Date: Sat, 5 Oct 2024 12:52:19 +0200 Subject: [PATCH 18/29] deduplicate test helper --- src/benchmark/benchmark_ipc_channel.cpp | 52 +++---------------------- src/threading/ipc_channel.cpp | 33 ++++++++++++++++ src/threading/ipc_channel.h | 28 +++++++++++++ src/unittest/test_threading.cpp | 48 +---------------------- 4 files changed, 68 insertions(+), 93 deletions(-) diff --git a/src/benchmark/benchmark_ipc_channel.cpp b/src/benchmark/benchmark_ipc_channel.cpp index 4232739e19..e81f55cf44 100644 --- a/src/benchmark/benchmark_ipc_channel.cpp +++ b/src/benchmark/benchmark_ipc_channel.cpp @@ -23,53 +23,8 @@ with this program; if not, write to the Free Software Foundation, Inc., TEST_CASE("benchmark_ipc_channel") { - // same as in test_threading.cpp (TODO: remove duplication) - struct IPCChannelResourcesSingleProcess final : public IPCChannelResources - { - void cleanupLast() noexcept override - { - delete data.shared; -#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 - CloseHandle(data.sem_b); - CloseHandle(data.sem_a); -#endif - } - - void cleanupNotLast() noexcept override - { - // nothing to do (i.e. no unmapping needed) - } - - ~IPCChannelResourcesSingleProcess() override { cleanup(); } - }; - - auto resource_data = [] { - auto shared = new IPCChannelShared(); - -#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 - HANDLE sem_a = CreateSemaphoreA(nullptr, 0, 1, nullptr); - REQUIRE(sem_a != INVALID_HANDLE_VALUE); - - HANDLE sem_b = CreateSemaphoreA(nullptr, 0, 1, nullptr); - REQUIRE(sem_b != INVALID_HANDLE_VALUE); - - return IPCChannelResources::Data{shared, sem_a, sem_b}; -#else - return IPCChannelResources::Data{shared}; -#endif - }(); - - auto resources_first = std::make_unique(); - resources_first->setFirst(resource_data); - - IPCChannelEnd end_a = IPCChannelEnd::makeA(std::move(resources_first)); - - // echos back messages. stops if "" is sent - std::thread thread_b([=] { - auto resources_second = std::make_unique(); - resources_second->setSecond(resource_data); - IPCChannelEnd end_b = IPCChannelEnd::makeB(std::move(resources_second)); - + auto end_a_thread_b_p = make_test_ipc_channel([](IPCChannelEnd end_b) { + // echos back messages. stops if "" is sent for (;;) { end_b.recv(); end_b.send(end_b.getRecvData(), end_b.getRecvSize()); @@ -77,6 +32,9 @@ TEST_CASE("benchmark_ipc_channel") break; } }); + // Can't use structured bindings before C++20, because of lamda captures below. + auto end_a = std::move(end_a_thread_b_p.first); + auto thread_b = std::move(end_a_thread_b_p.second); BENCHMARK("simple_call_1", i) { char buf[16] = {}; diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 922dde34c0..bf319fa381 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -353,3 +353,36 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept } return true; } + +std::pair make_test_ipc_channel( + const std::function &fun) +{ + auto resource_data = [] { + auto shared = new IPCChannelShared(); + +#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 + HANDLE sem_a = CreateSemaphoreA(nullptr, 0, 1, nullptr); + HANDLE sem_b = CreateSemaphoreA(nullptr, 0, 1, nullptr); + FATAL_ERROR_IF(!sem_a || !sem_b, "CreateSemaphoreA failed"); + + return IPCChannelResources::Data{shared, sem_a, sem_b}; +#else + return IPCChannelResources::Data{shared}; +#endif + }(); + + auto resources_first = std::make_unique(); + resources_first->setFirst(resource_data); + + IPCChannelEnd end_a = IPCChannelEnd::makeA(std::move(resources_first)); + + std::thread thread_b([=] { + auto resources_second = std::make_unique(); + resources_second->setSecond(resource_data); + IPCChannelEnd end_b = IPCChannelEnd::makeB(std::move(resources_second)); + + fun(std::move(end_b)); + }); + + return {std::move(end_a), std::move(thread_b)}; +} diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index c701947e4e..b83eda8ca1 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -25,6 +25,8 @@ with this program; if not, write to the Free Software Foundation, Inc., #include #include #include +#include +#include #if defined(_WIN32) #define IPC_CHANNEL_IMPLEMENTATION_WIN32 @@ -256,3 +258,29 @@ private: // (this buffer only grows) std::vector m_large_recv = std::vector(IPC_CHANNEL_MSG_SIZE); }; + +// For testing purposes +struct IPCChannelResourcesSingleProcess final : public IPCChannelResources +{ + void cleanupLast() noexcept override + { + delete data.shared; +#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 + CloseHandle(data.sem_b); + CloseHandle(data.sem_a); +#endif + } + + void cleanupNotLast() noexcept override + { + // nothing to do (i.e. no unmapping needed) + } + + ~IPCChannelResourcesSingleProcess() override { cleanup(); } +}; + +// For testing +// Returns one end and a thread holding the other end. The thread will execute +// fun, and pass it the other end. +std::pair make_test_ipc_channel( + const std::function &fun); diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index 2d82e4353b..e1b5e25c4a 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -236,52 +236,8 @@ void TestThreading::testTLS() void TestThreading::testIPCChannel() { - struct IPCChannelResourcesSingleProcess final : public IPCChannelResources - { - void cleanupLast() noexcept override - { - delete data.shared; -#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 - CloseHandle(data.sem_b); - CloseHandle(data.sem_a); -#endif - } - - void cleanupNotLast() noexcept override - { - // nothing to do (i.e. no unmapping needed) - } - - ~IPCChannelResourcesSingleProcess() override { cleanup(); } - }; - - auto resource_data = [] { - auto shared = new IPCChannelShared(); - -#ifdef IPC_CHANNEL_IMPLEMENTATION_WIN32 - HANDLE sem_a = CreateSemaphoreA(nullptr, 0, 1, nullptr); - UASSERT(sem_a != INVALID_HANDLE_VALUE); - - HANDLE sem_b = CreateSemaphoreA(nullptr, 0, 1, nullptr); - UASSERT(sem_b != INVALID_HANDLE_VALUE); - - return IPCChannelResources::Data{shared, sem_a, sem_b}; -#else - return IPCChannelResources::Data{shared}; -#endif - }(); - - auto resources_first = std::make_unique(); - resources_first->setFirst(resource_data); - - IPCChannelEnd end_a = IPCChannelEnd::makeA(std::move(resources_first)); - - // echos back messages. stops if "" is sent - std::thread thread_b([=] { - auto resources_second = std::make_unique(); - resources_second->setSecond(resource_data); - IPCChannelEnd end_b = IPCChannelEnd::makeB(std::move(resources_second)); - + auto [end_a, thread_b] = make_test_ipc_channel([](IPCChannelEnd end_b) { + // echos back messages. stops if "" is sent for (;;) { UASSERT(end_b.recvWithTimeout(-1)); UASSERT(end_b.sendWithTimeout(end_b.getRecvData(), end_b.getRecvSize(), -1)); From cc56fb8d29c9ed04af72fcf680d21e38f5ac914d Mon Sep 17 00:00:00 2001 From: Desour Date: Sat, 5 Oct 2024 12:54:55 +0200 Subject: [PATCH 19/29] it was written directly below all the time --- src/threading/ipc_channel.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index b83eda8ca1..7c151e32fd 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -188,8 +188,8 @@ public: static IPCChannelEnd makeA(std::unique_ptr resources); static IPCChannelEnd makeB(std::unique_ptr resources); - // If send, recv, or exchange return false (=timeout), stop using the channel. <--- TODO:why? // Note: timeouts may be for receiving any response, not a whole message. + // If send, recv, or exchange return false (=timeout), stop using the channel. // Returns false on timeout [[nodiscard]] From acdb29805bdf7953417cb7cb85988783279521aa Mon Sep 17 00:00:00 2001 From: Desour Date: Sun, 6 Oct 2024 15:13:51 +0200 Subject: [PATCH 20/29] some tiny fixes --- doc/compiling/linux.md | 2 +- src/threading/ipc_channel.cpp | 9 +++++---- src/threading/ipc_channel.h | 6 +++--- src/unittest/test_threading.cpp | 3 --- src/util/container.h | 1 - 5 files changed, 9 insertions(+), 12 deletions(-) diff --git a/doc/compiling/linux.md b/doc/compiling/linux.md index d2372fc305..d3a8917fcb 100644 --- a/doc/compiling/linux.md +++ b/doc/compiling/linux.md @@ -22,7 +22,7 @@ For Debian/Ubuntu users: - sudo apt install g++ make libc6-dev cmake linux-libc-dev libpng-dev libjpeg-dev libgl1-mesa-dev libsqlite3-dev libogg-dev libvorbis-dev libopenal-dev libcurl4-gnutls-dev libfreetype6-dev zlib1g-dev libgmp-dev libjsoncpp-dev libzstd-dev libluajit-5.1-dev gettext libsdl2-dev + sudo apt install g++ make libc6-dev cmake linux-libc-dev libpng-dev libjpeg-dev libgl1-mesa-dev libsqlite3-dev libogg-dev libvorbis-dev libopenal-dev libcurl4-gnutls-dev libfreetype6-dev zlib1g-dev libgmp-dev libjsoncpp-dev libzstd-dev libluajit-5.1-dev gettext libsdl2-dev For Fedora users: diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index bf319fa381..59b90c4f5d 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -1,7 +1,7 @@ /* Minetest -Copyright (C) 2022 DS Copyright (C) 2022 TurkeyMcMac, Jude Melton-Houghton +Copyright (C) 2024 DS This program is free software; you can redistribute it and/or modify it under the terms of the GNU Lesser General Public License as published by @@ -20,6 +20,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "ipc_channel.h" #include "debug.h" +#include "exceptions.h" #include "porting.h" #include #include @@ -179,7 +180,7 @@ static bool wait_in(IPCChannelEnd::Dir *dir, u64 timeout_ms_abs) u64 timeout_ms = timeout_ms_abs - tnow; #elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) // Absolute time - u64 timeout_msu = timeout_ms_abs; + u64 timeout_ms = timeout_ms_abs; #endif timeout.tv_sec = timeout_ms / 1000; timeout.tv_nsec = timeout_ms % 1000 * 1000000UL; @@ -200,13 +201,13 @@ static void post_out(IPCChannelEnd::Dir *dir) } template -static inline void write_once(volatile T *var, const T val) +static void write_once(volatile T *var, const T val) { *var = val; } template -static inline T read_once(const volatile T *var) +static T read_once(const volatile T *var) { return *var; } diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 7c151e32fd..0cd8b4962b 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -1,7 +1,7 @@ /* Minetest -Copyright (C) 2022 DS Copyright (C) 2022 TurkeyMcMac, Jude Melton-Houghton +Copyright (C) 2024 DS This program is free software; you can redistribute it and/or modify it under the terms of the GNU Lesser General Public License as published by @@ -237,8 +237,8 @@ public: // Get the content of the last received message // TODO: u8 *, or string_view? - inline const void *getRecvData() const noexcept { return m_large_recv.data(); } - inline size_t getRecvSize() const noexcept { return m_recv_size; } + const void *getRecvData() const noexcept { return m_large_recv.data(); } + size_t getRecvSize() const noexcept { return m_recv_size; } private: IPCChannelEnd(std::unique_ptr resources, Dir dir) : diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index e1b5e25c4a..7db619b041 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -6,9 +6,6 @@ #include #include -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) -#include -#endif #include "threading/ipc_channel.h" #include "threading/semaphore.h" #include "threading/thread.h" diff --git a/src/util/container.h b/src/util/container.h index a6fb95a10d..2ab573c19d 100644 --- a/src/util/container.h +++ b/src/util/container.h @@ -9,7 +9,6 @@ #include "exceptions.h" #include "threading/mutex_auto_lock.h" #include "threading/semaphore.h" -#include "debug.h" #include #include #include From 7e393e01dae4bf6691925f1f87a8dd6c662e678e Mon Sep 17 00:00:00 2001 From: Desour Date: Sun, 6 Oct 2024 15:45:15 +0200 Subject: [PATCH 21/29] use string_view? --- src/benchmark/benchmark_ipc_channel.cpp | 16 ++++++------- src/threading/ipc_channel.cpp | 25 +++++++++++--------- src/threading/ipc_channel.h | 31 ++++++++++++------------- src/unittest/test_threading.cpp | 16 ++++++------- 4 files changed, 45 insertions(+), 43 deletions(-) diff --git a/src/benchmark/benchmark_ipc_channel.cpp b/src/benchmark/benchmark_ipc_channel.cpp index e81f55cf44..2340d81471 100644 --- a/src/benchmark/benchmark_ipc_channel.cpp +++ b/src/benchmark/benchmark_ipc_channel.cpp @@ -27,8 +27,8 @@ TEST_CASE("benchmark_ipc_channel") // echos back messages. stops if "" is sent for (;;) { end_b.recv(); - end_b.send(end_b.getRecvData(), end_b.getRecvSize()); - if (end_b.getRecvSize() == 0) + end_b.send(end_b.getRecvData()); + if (end_b.getRecvData().size() == 0) break; } }); @@ -39,8 +39,8 @@ TEST_CASE("benchmark_ipc_channel") BENCHMARK("simple_call_1", i) { char buf[16] = {}; buf[i & 0xf] = i; - end_a.exchange(buf, 16); - return reinterpret_cast(end_a.getRecvData())[i & 0xf]; + end_a.exchange({buf, 16}); + return end_a.getRecvData()[i & 0xf]; }; BENCHMARK("simple_call_1000", i) { @@ -48,14 +48,14 @@ TEST_CASE("benchmark_ipc_channel") buf[i & 0xf] = i; for (int k = 0; k < 1000; ++k) { buf[0] = k & 0xff; - end_a.exchange(buf, 16); + end_a.exchange({buf, 16}); } - return reinterpret_cast(end_a.getRecvData())[i & 0xf]; + return end_a.getRecvData()[i & 0xf]; }; // stop thread_b - end_a.exchange(nullptr, 0); - REQUIRE(end_a.getRecvSize() == 0); + end_a.exchange({nullptr, 0}); + REQUIRE(end_a.getRecvData().size() == 0); thread_b.join(); } diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 59b90c4f5d..d053161c56 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -278,35 +278,38 @@ IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr resource #endif // !defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) } -void IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept +void IPCChannelEnd::sendSmall(std::string_view data) noexcept { - write_once(&m_dir.buf_out->size, size); + write_once(&m_dir.buf_out->size, data.size()); - if (size != 0) - memcpy(m_dir.buf_out->data, data, size); + if (data.size() != 0) + memcpy(m_dir.buf_out->data, data.data(), data.size()); post_out(&m_dir); } -bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noexcept +bool IPCChannelEnd::sendLarge(std::string_view data, int timeout_ms) noexcept { u64 timeout_ms_abs = timeout_ms < 0 ? 0 : porting::getTimeMs() + timeout_ms; - write_once(&m_dir.buf_out->size, size); + write_once(&m_dir.buf_out->size, data.size()); - do { - memcpy(m_dir.buf_out->data, data, IPC_CHANNEL_MSG_SIZE); + size_t size = data.size(); + const u8 *ptr = reinterpret_cast(data.data()); + + while (size > IPC_CHANNEL_MSG_SIZE) { + memcpy(m_dir.buf_out->data, ptr, IPC_CHANNEL_MSG_SIZE); post_out(&m_dir); if (!wait_in(&m_dir, timeout_ms_abs)) return false; size -= IPC_CHANNEL_MSG_SIZE; - data = (u8 *)data + IPC_CHANNEL_MSG_SIZE; - } while (size > IPC_CHANNEL_MSG_SIZE); + ptr = ptr + IPC_CHANNEL_MSG_SIZE; + } if (size != 0) - memcpy(m_dir.buf_out->data, data, size); + memcpy(m_dir.buf_out->data, ptr, size); post_out(&m_dir); return true; diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 0cd8b4962b..2d8e1a6892 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -27,6 +27,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #include #include #include +#include #if defined(_WIN32) #define IPC_CHANNEL_IMPLEMENTATION_WIN32 @@ -193,20 +194,20 @@ public: // Returns false on timeout [[nodiscard]] - bool sendWithTimeout(const void *data, size_t size, int timeout_ms) noexcept + bool sendWithTimeout(std::string_view data, int timeout_ms) noexcept { - if (size <= IPC_CHANNEL_MSG_SIZE) { - sendSmall(data, size); + if (data.size() <= IPC_CHANNEL_MSG_SIZE) { + sendSmall(data); return true; } else { - return sendLarge(data, size, timeout_ms); + return sendLarge(data, timeout_ms); } } // Same as above - void send(const void *data, size_t size) noexcept + void send(std::string_view data) noexcept { - (void)sendWithTimeout(data, size, -1); + (void)sendWithTimeout(data, -1); } // Returns false on timeout. @@ -223,33 +224,31 @@ public: // Returns false on timeout // Otherwise returns true, and data is available via getRecvData(). [[nodiscard]] - bool exchangeWithTimeout(const void *data, size_t size, int timeout_ms) noexcept + bool exchangeWithTimeout(std::string_view data, int timeout_ms) noexcept { - return sendWithTimeout(data, size, timeout_ms) + return sendWithTimeout(data, timeout_ms) && recvWithTimeout(timeout_ms); } // Same as above - void exchange(const void *data, size_t size) noexcept + void exchange(std::string_view data) noexcept { - (void)exchangeWithTimeout(data, size, -1); + (void)exchangeWithTimeout(data, -1); } // Get the content of the last received message - // TODO: u8 *, or string_view? - const void *getRecvData() const noexcept { return m_large_recv.data(); } - size_t getRecvSize() const noexcept { return m_recv_size; } + std::string_view getRecvData() const noexcept + { return {reinterpret_cast(m_large_recv.data()), m_recv_size}; } private: IPCChannelEnd(std::unique_ptr resources, Dir dir) : m_resources(std::move(resources)), m_dir(dir) {} - // TODO: u8 *, or string_view? - void sendSmall(const void *data, size_t size) noexcept; + void sendSmall(std::string_view data) noexcept; // returns false on timeout - bool sendLarge(const void *data, size_t size, int timeout_ms) noexcept; + bool sendLarge(std::string_view data, int timeout_ms) noexcept; std::unique_ptr m_resources; Dir m_dir; diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index 7db619b041..a2e1298bb1 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -237,8 +237,8 @@ void TestThreading::testIPCChannel() // echos back messages. stops if "" is sent for (;;) { UASSERT(end_b.recvWithTimeout(-1)); - UASSERT(end_b.sendWithTimeout(end_b.getRecvData(), end_b.getRecvSize(), -1)); - if (end_b.getRecvSize() == 0) + UASSERT(end_b.sendWithTimeout(end_b.getRecvData(), -1)); + if (end_b.getRecvData().size() == 0) break; } }); @@ -246,17 +246,17 @@ void TestThreading::testIPCChannel() char buf[20000] = {}; for (int i = sizeof(buf); i > 0; i -= 100) { buf[i - 1] = 123; - UASSERT(end_a.exchangeWithTimeout(buf, i, -1)); - UASSERTEQ(int, end_a.getRecvSize(), i); - UASSERTEQ(int, ((const char *)end_a.getRecvData())[i - 1], 123); + UASSERT(end_a.exchangeWithTimeout({buf, (size_t)i}, -1)); + UASSERTEQ(int, end_a.getRecvData().size(), i); + UASSERTEQ(int, end_a.getRecvData().data()[i - 1], 123); } // stop thread_b - UASSERT(end_a.exchangeWithTimeout(buf, 0, -1)); - UASSERTEQ(int, end_a.getRecvSize(), 0); + UASSERT(end_a.exchangeWithTimeout({buf, 0}, -1)); + UASSERTEQ(int, end_a.getRecvData().size(), 0); thread_b.join(); // other side dead ==> should time out - UASSERT(!end_a.exchangeWithTimeout(buf, 0, 200)); + UASSERT(!end_a.exchangeWithTimeout({buf, 0}, 200)); } From 04620f15656df9178b0721b633e97e1e89137029 Mon Sep 17 00:00:00 2001 From: Desour Date: Sun, 6 Oct 2024 15:45:26 +0200 Subject: [PATCH 22/29] Revert "use string_view?" This reverts commit eeedd9650df8590a46d8a88a82ade7617d3be971. --- src/benchmark/benchmark_ipc_channel.cpp | 16 ++++++------- src/threading/ipc_channel.cpp | 25 +++++++++----------- src/threading/ipc_channel.h | 31 +++++++++++++------------ src/unittest/test_threading.cpp | 16 ++++++------- 4 files changed, 43 insertions(+), 45 deletions(-) diff --git a/src/benchmark/benchmark_ipc_channel.cpp b/src/benchmark/benchmark_ipc_channel.cpp index 2340d81471..e81f55cf44 100644 --- a/src/benchmark/benchmark_ipc_channel.cpp +++ b/src/benchmark/benchmark_ipc_channel.cpp @@ -27,8 +27,8 @@ TEST_CASE("benchmark_ipc_channel") // echos back messages. stops if "" is sent for (;;) { end_b.recv(); - end_b.send(end_b.getRecvData()); - if (end_b.getRecvData().size() == 0) + end_b.send(end_b.getRecvData(), end_b.getRecvSize()); + if (end_b.getRecvSize() == 0) break; } }); @@ -39,8 +39,8 @@ TEST_CASE("benchmark_ipc_channel") BENCHMARK("simple_call_1", i) { char buf[16] = {}; buf[i & 0xf] = i; - end_a.exchange({buf, 16}); - return end_a.getRecvData()[i & 0xf]; + end_a.exchange(buf, 16); + return reinterpret_cast(end_a.getRecvData())[i & 0xf]; }; BENCHMARK("simple_call_1000", i) { @@ -48,14 +48,14 @@ TEST_CASE("benchmark_ipc_channel") buf[i & 0xf] = i; for (int k = 0; k < 1000; ++k) { buf[0] = k & 0xff; - end_a.exchange({buf, 16}); + end_a.exchange(buf, 16); } - return end_a.getRecvData()[i & 0xf]; + return reinterpret_cast(end_a.getRecvData())[i & 0xf]; }; // stop thread_b - end_a.exchange({nullptr, 0}); - REQUIRE(end_a.getRecvData().size() == 0); + end_a.exchange(nullptr, 0); + REQUIRE(end_a.getRecvSize() == 0); thread_b.join(); } diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index d053161c56..59b90c4f5d 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -278,38 +278,35 @@ IPCChannelEnd IPCChannelEnd::makeB(std::unique_ptr resource #endif // !defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) } -void IPCChannelEnd::sendSmall(std::string_view data) noexcept +void IPCChannelEnd::sendSmall(const void *data, size_t size) noexcept { - write_once(&m_dir.buf_out->size, data.size()); + write_once(&m_dir.buf_out->size, size); - if (data.size() != 0) - memcpy(m_dir.buf_out->data, data.data(), data.size()); + if (size != 0) + memcpy(m_dir.buf_out->data, data, size); post_out(&m_dir); } -bool IPCChannelEnd::sendLarge(std::string_view data, int timeout_ms) noexcept +bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noexcept { u64 timeout_ms_abs = timeout_ms < 0 ? 0 : porting::getTimeMs() + timeout_ms; - write_once(&m_dir.buf_out->size, data.size()); + write_once(&m_dir.buf_out->size, size); - size_t size = data.size(); - const u8 *ptr = reinterpret_cast(data.data()); - - while (size > IPC_CHANNEL_MSG_SIZE) { - memcpy(m_dir.buf_out->data, ptr, IPC_CHANNEL_MSG_SIZE); + do { + memcpy(m_dir.buf_out->data, data, IPC_CHANNEL_MSG_SIZE); post_out(&m_dir); if (!wait_in(&m_dir, timeout_ms_abs)) return false; size -= IPC_CHANNEL_MSG_SIZE; - ptr = ptr + IPC_CHANNEL_MSG_SIZE; - } + data = (u8 *)data + IPC_CHANNEL_MSG_SIZE; + } while (size > IPC_CHANNEL_MSG_SIZE); if (size != 0) - memcpy(m_dir.buf_out->data, ptr, size); + memcpy(m_dir.buf_out->data, data, size); post_out(&m_dir); return true; diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 2d8e1a6892..0cd8b4962b 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -27,7 +27,6 @@ with this program; if not, write to the Free Software Foundation, Inc., #include #include #include -#include #if defined(_WIN32) #define IPC_CHANNEL_IMPLEMENTATION_WIN32 @@ -194,20 +193,20 @@ public: // Returns false on timeout [[nodiscard]] - bool sendWithTimeout(std::string_view data, int timeout_ms) noexcept + bool sendWithTimeout(const void *data, size_t size, int timeout_ms) noexcept { - if (data.size() <= IPC_CHANNEL_MSG_SIZE) { - sendSmall(data); + if (size <= IPC_CHANNEL_MSG_SIZE) { + sendSmall(data, size); return true; } else { - return sendLarge(data, timeout_ms); + return sendLarge(data, size, timeout_ms); } } // Same as above - void send(std::string_view data) noexcept + void send(const void *data, size_t size) noexcept { - (void)sendWithTimeout(data, -1); + (void)sendWithTimeout(data, size, -1); } // Returns false on timeout. @@ -224,31 +223,33 @@ public: // Returns false on timeout // Otherwise returns true, and data is available via getRecvData(). [[nodiscard]] - bool exchangeWithTimeout(std::string_view data, int timeout_ms) noexcept + bool exchangeWithTimeout(const void *data, size_t size, int timeout_ms) noexcept { - return sendWithTimeout(data, timeout_ms) + return sendWithTimeout(data, size, timeout_ms) && recvWithTimeout(timeout_ms); } // Same as above - void exchange(std::string_view data) noexcept + void exchange(const void *data, size_t size) noexcept { - (void)exchangeWithTimeout(data, -1); + (void)exchangeWithTimeout(data, size, -1); } // Get the content of the last received message - std::string_view getRecvData() const noexcept - { return {reinterpret_cast(m_large_recv.data()), m_recv_size}; } + // TODO: u8 *, or string_view? + const void *getRecvData() const noexcept { return m_large_recv.data(); } + size_t getRecvSize() const noexcept { return m_recv_size; } private: IPCChannelEnd(std::unique_ptr resources, Dir dir) : m_resources(std::move(resources)), m_dir(dir) {} - void sendSmall(std::string_view data) noexcept; + // TODO: u8 *, or string_view? + void sendSmall(const void *data, size_t size) noexcept; // returns false on timeout - bool sendLarge(std::string_view data, int timeout_ms) noexcept; + bool sendLarge(const void *data, size_t size, int timeout_ms) noexcept; std::unique_ptr m_resources; Dir m_dir; diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index a2e1298bb1..7db619b041 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -237,8 +237,8 @@ void TestThreading::testIPCChannel() // echos back messages. stops if "" is sent for (;;) { UASSERT(end_b.recvWithTimeout(-1)); - UASSERT(end_b.sendWithTimeout(end_b.getRecvData(), -1)); - if (end_b.getRecvData().size() == 0) + UASSERT(end_b.sendWithTimeout(end_b.getRecvData(), end_b.getRecvSize(), -1)); + if (end_b.getRecvSize() == 0) break; } }); @@ -246,17 +246,17 @@ void TestThreading::testIPCChannel() char buf[20000] = {}; for (int i = sizeof(buf); i > 0; i -= 100) { buf[i - 1] = 123; - UASSERT(end_a.exchangeWithTimeout({buf, (size_t)i}, -1)); - UASSERTEQ(int, end_a.getRecvData().size(), i); - UASSERTEQ(int, end_a.getRecvData().data()[i - 1], 123); + UASSERT(end_a.exchangeWithTimeout(buf, i, -1)); + UASSERTEQ(int, end_a.getRecvSize(), i); + UASSERTEQ(int, ((const char *)end_a.getRecvData())[i - 1], 123); } // stop thread_b - UASSERT(end_a.exchangeWithTimeout({buf, 0}, -1)); - UASSERTEQ(int, end_a.getRecvData().size(), 0); + UASSERT(end_a.exchangeWithTimeout(buf, 0, -1)); + UASSERTEQ(int, end_a.getRecvSize(), 0); thread_b.join(); // other side dead ==> should time out - UASSERT(!end_a.exchangeWithTimeout({buf, 0}, 200)); + UASSERT(!end_a.exchangeWithTimeout(buf, 0, 200)); } From db55fdf73fa40df4e89c4264117361d8d41d982a Mon Sep 17 00:00:00 2001 From: Desour Date: Sun, 6 Oct 2024 15:49:42 +0200 Subject: [PATCH 23/29] dont use string_view I don't like that it's implicitly constructible from a const char *, and assumes it's 0-terminated. so void * it is. --- src/benchmark/benchmark_ipc_channel.cpp | 4 ++-- src/threading/ipc_channel.cpp | 2 +- src/threading/ipc_channel.h | 2 -- src/unittest/test_threading.cpp | 4 ++-- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/benchmark/benchmark_ipc_channel.cpp b/src/benchmark/benchmark_ipc_channel.cpp index e81f55cf44..b59ffe572a 100644 --- a/src/benchmark/benchmark_ipc_channel.cpp +++ b/src/benchmark/benchmark_ipc_channel.cpp @@ -37,14 +37,14 @@ TEST_CASE("benchmark_ipc_channel") auto thread_b = std::move(end_a_thread_b_p.second); BENCHMARK("simple_call_1", i) { - char buf[16] = {}; + u8 buf[16] = {}; buf[i & 0xf] = i; end_a.exchange(buf, 16); return reinterpret_cast(end_a.getRecvData())[i & 0xf]; }; BENCHMARK("simple_call_1000", i) { - char buf[16] = {}; + u8 buf[16] = {}; buf[i & 0xf] = i; for (int k = 0; k < 1000; ++k) { buf[0] = k & 0xff; diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 59b90c4f5d..fbe65a3cb9 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -302,7 +302,7 @@ bool IPCChannelEnd::sendLarge(const void *data, size_t size, int timeout_ms) noe return false; size -= IPC_CHANNEL_MSG_SIZE; - data = (u8 *)data + IPC_CHANNEL_MSG_SIZE; + data = reinterpret_cast(data) + IPC_CHANNEL_MSG_SIZE; } while (size > IPC_CHANNEL_MSG_SIZE); if (size != 0) diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 0cd8b4962b..3f8ae33bd5 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -236,7 +236,6 @@ public: } // Get the content of the last received message - // TODO: u8 *, or string_view? const void *getRecvData() const noexcept { return m_large_recv.data(); } size_t getRecvSize() const noexcept { return m_recv_size; } @@ -245,7 +244,6 @@ private: m_resources(std::move(resources)), m_dir(dir) {} - // TODO: u8 *, or string_view? void sendSmall(const void *data, size_t size) noexcept; // returns false on timeout diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index 7db619b041..18b5198384 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -243,12 +243,12 @@ void TestThreading::testIPCChannel() } }); - char buf[20000] = {}; + u8 buf[20000] = {}; for (int i = sizeof(buf); i > 0; i -= 100) { buf[i - 1] = 123; UASSERT(end_a.exchangeWithTimeout(buf, i, -1)); UASSERTEQ(int, end_a.getRecvSize(), i); - UASSERTEQ(int, ((const char *)end_a.getRecvData())[i - 1], 123); + UASSERTEQ(int, reinterpret_cast(end_a.getRecvData())[i - 1], 123); } // stop thread_b From d43cbafc37218e0080f44229aafe239be1cdc36a Mon Sep 17 00:00:00 2001 From: Desour Date: Sun, 6 Oct 2024 16:06:01 +0200 Subject: [PATCH 24/29] first unlock, then broadcast --- src/threading/ipc_channel.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index fbe65a3cb9..4e9fe8a727 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -147,8 +147,8 @@ static void post(IPCChannelBuffer *buf) noexcept { pthread_mutex_lock(&buf->mutex); buf->posted = true; - pthread_cond_broadcast(&buf->cond); pthread_mutex_unlock(&buf->mutex); + pthread_cond_broadcast(&buf->cond); } #endif From 75467e36952093c5965292a3f5dc5ff7d466fddb Mon Sep 17 00:00:00 2001 From: Desour Date: Sun, 6 Oct 2024 17:13:13 +0200 Subject: [PATCH 25/29] posix implementation: use correct clock --- src/threading/ipc_channel.cpp | 24 +++++++++++++++++------- src/threading/ipc_channel.h | 1 + 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 4e9fe8a727..73ef7b4d7c 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -126,7 +126,7 @@ static void post(IPCChannelBuffer *buf) noexcept #elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) -// timeout is absolute +// timeout is absolute (using cond_clockid) // returns false on timeout static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept { @@ -172,18 +172,25 @@ static bool wait_in(IPCChannelEnd::Dir *dir, u64 timeout_ms_abs) struct timespec timeout; struct timespec *timeoutp = nullptr; if (timeout_ms_abs > 0) { -#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) // Relative time u64 tnow = porting::getTimeMs(); if (tnow > timeout_ms_abs) return false; - u64 timeout_ms = timeout_ms_abs - tnow; + u64 timeout_ms_rel = timeout_ms_abs - tnow; +#if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) + timeout.tv_sec = 0; + timeout.tv_nsec = 0; #elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) - // Absolute time - u64 timeout_ms = timeout_ms_abs; + // Absolute time, relative to cond_clockid + FATAL_ERROR_IF(clock_gettime(dir->buf_in->cond_clockid, &timeout) < 0, + "clock_gettime failed"); + if (timeout.tv_nsec >= 1000'000'000L) { + timeout.tv_nsec -= 1000'000'000L; + timeout.tv_sec += 1; + } #endif - timeout.tv_sec = timeout_ms / 1000; - timeout.tv_nsec = timeout_ms % 1000 * 1000000UL; + timeout.tv_sec += timeout_ms_rel / 1000; + timeout.tv_nsec += timeout_ms_rel % 1000 * 1000'000L; timeoutp = &timeout; } @@ -225,6 +232,8 @@ IPCChannelBuffer::IPCChannelBuffer() goto error_condattr_setpshared; if (pthread_mutexattr_setpshared(&mutexattr, PTHREAD_PROCESS_SHARED) != 0) goto error_mutexattr_setpshared; + if (pthread_condattr_getclock(&condattr, &cond_clockid) != 0) + goto error_condattr_getclock; if (pthread_cond_init(&cond, &condattr) != 0) goto error_cond_init; if (pthread_mutex_init(&mutex, &mutexattr) != 0) @@ -236,6 +245,7 @@ IPCChannelBuffer::IPCChannelBuffer() error_mutex_init: pthread_cond_destroy(&cond); error_cond_init: +error_condattr_getclock: error_mutexattr_setpshared: error_condattr_setpshared: pthread_mutexattr_destroy(&mutexattr); diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 3f8ae33bd5..68a570ed82 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -71,6 +71,7 @@ struct IPCChannelBuffer #elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) pthread_cond_t cond; + clockid_t cond_clockid; pthread_mutex_t mutex; bool posted = false; // protected by mutex #endif From cb99ce0f99ae012086d7afe1c6b550272d4f96e7 Mon Sep 17 00:00:00 2001 From: Desour Date: Mon, 7 Oct 2024 13:18:49 +0200 Subject: [PATCH 26/29] some more little fixes and improvements --- src/benchmark/benchmark_ipc_channel.cpp | 2 +- src/threading/ipc_channel.cpp | 39 +++++++++--------- src/threading/ipc_channel.h | 54 ++++++++++++++----------- src/unittest/test_threading.cpp | 23 +++++++---- 4 files changed, 68 insertions(+), 50 deletions(-) diff --git a/src/benchmark/benchmark_ipc_channel.cpp b/src/benchmark/benchmark_ipc_channel.cpp index b59ffe572a..a8bcfad5ec 100644 --- a/src/benchmark/benchmark_ipc_channel.cpp +++ b/src/benchmark/benchmark_ipc_channel.cpp @@ -25,7 +25,7 @@ TEST_CASE("benchmark_ipc_channel") { auto end_a_thread_b_p = make_test_ipc_channel([](IPCChannelEnd end_b) { // echos back messages. stops if "" is sent - for (;;) { + while (true) { end_b.recv(); end_b.send(end_b.getRecvData(), end_b.getRecvSize()); if (end_b.getRecvSize() == 0) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 73ef7b4d7c..4b0f37a541 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -80,7 +80,7 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept if (buf->futex.load(std::memory_order_acquire) == 1) { // yes // reset it. (relaxed ordering is sufficient, because the other thread - // does not need to see the side effects we did before writing 0) + // does not need to see the side effects we did before unposting) buf->futex.store(0, std::memory_order_relaxed); return true; } @@ -93,7 +93,7 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept // wait with futex while (true) { // write 2 to show that we're futexing - if (buf->futex.exchange(2, std::memory_order_acq_rel) == 1) { + if (buf->futex.exchange(2, std::memory_order_acquire) == 1) { // it was posted in the meantime buf->futex.store(0, std::memory_order_relaxed); return true; @@ -113,7 +113,7 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept static void post(IPCChannelBuffer *buf) noexcept { - if (buf->futex.exchange(1, std::memory_order_acq_rel) == 2) { + if (buf->futex.exchange(1, std::memory_order_release) == 2) { // 2 means reader needs to be notified int s = futex(&buf->futex, FUTEX_WAKE, 1, nullptr, nullptr, 0); if (s == -1) { @@ -130,17 +130,18 @@ static void post(IPCChannelBuffer *buf) noexcept // returns false on timeout static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept { - bool timed_out = false; pthread_mutex_lock(&buf->mutex); - if (!buf->posted) { - if (timeout) - timed_out = pthread_cond_timedwait(&buf->cond, &buf->mutex, timeout) == ETIMEDOUT; - else + while (!buf->posted) { + if (timeout) { + if (pthread_cond_timedwait(&buf->cond, &buf->mutex, timeout) == ETIMEDOUT) + return false; + } else { pthread_cond_wait(&buf->cond, &buf->mutex); + } } buf->posted = false; pthread_mutex_unlock(&buf->mutex); - return !timed_out; + return true; } static void post(IPCChannelBuffer *buf) noexcept @@ -172,18 +173,19 @@ static bool wait_in(IPCChannelEnd::Dir *dir, u64 timeout_ms_abs) struct timespec timeout; struct timespec *timeoutp = nullptr; if (timeout_ms_abs > 0) { - // Relative time u64 tnow = porting::getTimeMs(); if (tnow > timeout_ms_abs) return false; u64 timeout_ms_rel = timeout_ms_abs - tnow; #if defined(IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX) + // Relative time timeout.tv_sec = 0; timeout.tv_nsec = 0; #elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) // Absolute time, relative to cond_clockid FATAL_ERROR_IF(clock_gettime(dir->buf_in->cond_clockid, &timeout) < 0, "clock_gettime failed"); + // prevent overflow if (timeout.tv_nsec >= 1000'000'000L) { timeout.tv_nsec -= 1000'000'000L; timeout.tv_sec += 1; @@ -339,7 +341,8 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept if (size <= IPC_CHANNEL_MSG_SIZE) { // small msg // (m_large_recv.size() is always >= IPC_CHANNEL_MSG_SIZE) - memcpy(m_large_recv.data(), m_dir.buf_in->data, size); + if (size != 0) + memcpy(m_large_recv.data(), m_dir.buf_in->data, size); } else { // large msg @@ -360,7 +363,8 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept if (!wait_in(&m_dir, timeout_ms_abs)) return false; } while (size > IPC_CHANNEL_MSG_SIZE); - memcpy(recv_data, m_dir.buf_in->data, size); + if (size != 0) + memcpy(recv_data, m_dir.buf_in->data, size); } return true; } @@ -382,18 +386,15 @@ std::pair make_test_ipc_channel( #endif }(); - auto resources_first = std::make_unique(); - resources_first->setFirst(resource_data); - - IPCChannelEnd end_a = IPCChannelEnd::makeA(std::move(resources_first)); - std::thread thread_b([=] { - auto resources_second = std::make_unique(); - resources_second->setSecond(resource_data); + auto resources_second = IPCChannelResourcesSingleProcess::makeSecond(resource_data); IPCChannelEnd end_b = IPCChannelEnd::makeB(std::move(resources_second)); fun(std::move(end_b)); }); + auto resources_first = IPCChannelResourcesSingleProcess::makeFirst(resource_data); + IPCChannelEnd end_a = IPCChannelEnd::makeA(std::move(resources_first)); + return {std::move(end_a), std::move(thread_b)}; } diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 68a570ed82..09acd09ec0 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -56,7 +56,7 @@ with this program; if not, write to the Free Software Foundation, Inc., * other posix: uses posix mutex and condition variable */ -#define IPC_CHANNEL_MSG_SIZE 0x2000U +constexpr size_t IPC_CHANNEL_MSG_SIZE = 0x2000; struct IPCChannelBuffer { @@ -83,6 +83,7 @@ struct IPCChannelBuffer u8 data[IPC_CHANNEL_MSG_SIZE] = {}; IPCChannelBuffer(); + DISABLE_CLASS_COPY(IPCChannelBuffer) ~IPCChannelBuffer(); // Note: only destruct once, i.e. in one process }; @@ -96,18 +97,8 @@ struct IPCChannelShared IPCChannelBuffer b{}; }; -struct IPCChannelDirection -{ - IPCChannelBuffer *buf_in; - IPCChannelBuffer *buf_out; -#if defined(IPC_CHANNEL_IMPLEMENTATION_WIN32) - HANDLE sem_in; - HANDLE sem_out; -#endif -}; - -// Each end holds this. One is A, one is B. -// Implementors of this struct decide how to allocate buffers (i.e. malloc or mmap). +// Interface for managing the shared resources. +// Implementors decide whether to use malloc or mmap. struct IPCChannelResources { // new struct, because the win32 #if is annoying @@ -123,6 +114,15 @@ struct IPCChannelResources Data data; + IPCChannelResources() = default; + DISABLE_CLASS_COPY(IPCChannelResources) + + // Child should call cleanup(). + // (Parent destructor can not do this, because when it's called the child is + // already dead.) + virtual ~IPCChannelResources() = default; + +protected: // Used for previously unmanaged data_ (move semantics) void setFirst(Data data_) { @@ -160,14 +160,6 @@ struct IPCChannelResources cleanupNotLast(); } } - - IPCChannelResources() = default; - DISABLE_CLASS_COPY(IPCChannelResources) - - // Child should call cleanup(). - // (Parent destructor can not do this, because when it's called the child is - // already dead.) - virtual ~IPCChannelResources() = default; }; class IPCChannelEnd @@ -184,13 +176,15 @@ public: #endif }; + // Unusable empty end IPCChannelEnd() = default; + // Construct end A or end B from resources static IPCChannelEnd makeA(std::unique_ptr resources); static IPCChannelEnd makeB(std::unique_ptr resources); - // Note: timeouts may be for receiving any response, not a whole message. - // If send, recv, or exchange return false (=timeout), stop using the channel. + // Note: Timeouts may be for receiving any response, not a whole message. + // Therefore, if a timeout occurs, stop using the channel. // Returns false on timeout [[nodiscard]] @@ -276,6 +270,20 @@ struct IPCChannelResourcesSingleProcess final : public IPCChannelResources } ~IPCChannelResourcesSingleProcess() override { cleanup(); } + + static std::unique_ptr makeFirst(Data data) + { + auto ret = std::make_unique(); + ret->setFirst(data); + return ret; + } + + static std::unique_ptr makeSecond(Data data) + { + auto ret = std::make_unique(); + ret->setSecond(data); + return ret; + } }; // For testing diff --git a/src/unittest/test_threading.cpp b/src/unittest/test_threading.cpp index 18b5198384..51d757e5ec 100644 --- a/src/unittest/test_threading.cpp +++ b/src/unittest/test_threading.cpp @@ -235,7 +235,7 @@ void TestThreading::testIPCChannel() { auto [end_a, thread_b] = make_test_ipc_channel([](IPCChannelEnd end_b) { // echos back messages. stops if "" is sent - for (;;) { + while (true) { UASSERT(end_b.recvWithTimeout(-1)); UASSERT(end_b.sendWithTimeout(end_b.getRecvData(), end_b.getRecvSize(), -1)); if (end_b.getRecvSize() == 0) @@ -243,20 +243,29 @@ void TestThreading::testIPCChannel() } }); - u8 buf[20000] = {}; - for (int i = sizeof(buf); i > 0; i -= 100) { - buf[i - 1] = 123; - UASSERT(end_a.exchangeWithTimeout(buf, i, -1)); + u8 buf1[20000] = {}; + for (int i = sizeof(buf1); i > 0; i -= 100) { + buf1[i - 1] = 123; + UASSERT(end_a.exchangeWithTimeout(buf1, i, -1)); UASSERTEQ(int, end_a.getRecvSize(), i); UASSERTEQ(int, reinterpret_cast(end_a.getRecvData())[i - 1], 123); } + u8 buf2[IPC_CHANNEL_MSG_SIZE * 3 + 10]; + end_a.exchange(buf2, IPC_CHANNEL_MSG_SIZE * 3 + 10); + end_a.exchange(buf2, IPC_CHANNEL_MSG_SIZE * 3); + end_a.exchange(buf2, IPC_CHANNEL_MSG_SIZE); + end_a.exchange(buf2, IPC_CHANNEL_MSG_SIZE * 2); + end_a.exchange(buf2, IPC_CHANNEL_MSG_SIZE - 1); + end_a.exchange(buf2, IPC_CHANNEL_MSG_SIZE + 1); + end_a.exchange(buf2, 1); + // stop thread_b - UASSERT(end_a.exchangeWithTimeout(buf, 0, -1)); + UASSERT(end_a.exchangeWithTimeout(nullptr, 0, -1)); UASSERTEQ(int, end_a.getRecvSize(), 0); thread_b.join(); // other side dead ==> should time out - UASSERT(!end_a.exchangeWithTimeout(buf, 0, 200)); + UASSERT(!end_a.exchangeWithTimeout(nullptr, 0, 200)); } From 618ef90c77f754540d6c0c596e14958b24e78498 Mon Sep 17 00:00:00 2001 From: Desour Date: Mon, 7 Oct 2024 16:33:43 +0200 Subject: [PATCH 27/29] fix build, CLOCK_REALTIME is default, and getclock is too new. and fix infinite loop with lock becuase of nsec too big it took me way too long long to figure both of these out. why are the man pages missing all the important details?! at least the new versions of the posix standard are helpful: https://pubs.opengroup.org/onlinepubs/9699919799/ --- src/threading/ipc_channel.cpp | 53 +++++++++++++++++++++++++++-------- src/threading/ipc_channel.h | 3 +- 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 4b0f37a541..1cfd074125 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -128,20 +128,35 @@ static void post(IPCChannelBuffer *buf) noexcept // timeout is absolute (using cond_clockid) // returns false on timeout -static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept +static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout, const char *timestr = nullptr) noexcept { + if (timestr) + errorstream << timestr << std::endl; + bool timed_out = false; pthread_mutex_lock(&buf->mutex); while (!buf->posted) { if (timeout) { - if (pthread_cond_timedwait(&buf->cond, &buf->mutex, timeout) == ETIMEDOUT) - return false; + auto err = pthread_cond_timedwait(&buf->cond, &buf->mutex, timeout); + if (err == ETIMEDOUT) { + timed_out = true; + break; + } else if (err == EINTR || err == EOWNERDEAD || err == ENOTRECOVERABLE || err == EPERM || err == EINVAL) { + continue; + } else if (err != 0) { + pthread_mutex_unlock(&buf->mutex); + auto msg = "err: " + std::to_string(err); + FATAL_ERROR(msg.c_str()); + bool ret = wait(buf, timeout, msg.c_str()); + errorstream << msg << std::endl; + return ret; + } } else { pthread_cond_wait(&buf->cond, &buf->mutex); } } buf->posted = false; pthread_mutex_unlock(&buf->mutex); - return true; + return !timed_out; } static void post(IPCChannelBuffer *buf) noexcept @@ -172,6 +187,8 @@ static bool wait_in(IPCChannelEnd::Dir *dir, u64 timeout_ms_abs) #else struct timespec timeout; struct timespec *timeoutp = nullptr; + char timestr[201]; + timestr[0] = '\0'; if (timeout_ms_abs > 0) { u64 tnow = porting::getTimeMs(); if (tnow > timeout_ms_abs) @@ -182,21 +199,26 @@ static bool wait_in(IPCChannelEnd::Dir *dir, u64 timeout_ms_abs) timeout.tv_sec = 0; timeout.tv_nsec = 0; #elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) - // Absolute time, relative to cond_clockid - FATAL_ERROR_IF(clock_gettime(dir->buf_in->cond_clockid, &timeout) < 0, + // Absolute time + FATAL_ERROR_IF(clock_gettime(CLOCK_REALTIME, &timeout) < 0, "clock_gettime failed"); - // prevent overflow +#endif + timeout.tv_sec += timeout_ms_rel / 1000; + timeout.tv_nsec += timeout_ms_rel % 1000 * 1000'000L; + // tv_nsec must be smaller than 1 sec, or else pthread_cond_timedwait fails if (timeout.tv_nsec >= 1000'000'000L) { timeout.tv_nsec -= 1000'000'000L; timeout.tv_sec += 1; } -#endif - timeout.tv_sec += timeout_ms_rel / 1000; - timeout.tv_nsec += timeout_ms_rel % 1000 * 1000'000L; timeoutp = &timeout; + + time_t timeout_tt = timeout.tv_sec + timeout.tv_nsec / 1000'000'000L; + tm timeout_tm; + localtime_r(&timeout_tt, &timeout_tm); + strftime(timestr, 200, "%F %T ", &timeout_tm); } - return wait(dir->buf_in, timeoutp); + return wait(dir->buf_in, timeoutp, timestr); #endif } @@ -226,6 +248,7 @@ IPCChannelBuffer::IPCChannelBuffer() #if defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) pthread_condattr_t condattr; pthread_mutexattr_t mutexattr; + clockid_t cond_clockid; if (pthread_condattr_init(&condattr) != 0) goto error_condattr_init; if (pthread_mutexattr_init(&mutexattr) != 0) @@ -242,6 +265,12 @@ IPCChannelBuffer::IPCChannelBuffer() goto error_mutex_init; pthread_mutexattr_destroy(&mutexattr); pthread_condattr_destroy(&condattr); +/* + { + std::string bla = std::string("realt: ") + std::to_string(CLOCK_REALTIME) + " cond_clockid: " + std::to_string(cond_clockid); + FATAL_ERROR(bla.c_str()); + }*/ + FATAL_ERROR_IF(cond_clockid != CLOCK_REALTIME, "wrong clock"); return; error_mutex_init: @@ -350,7 +379,7 @@ bool IPCChannelEnd::recvWithTimeout(int timeout_ms) noexcept m_large_recv.resize(size); } catch (...) { // it's ok for us if an attacker wants to make us abort - std::string errmsg = std::string("std::vector::resize failed, size was: ") + std::string errmsg = "std::vector::resize failed, size was: " + std::to_string(size); FATAL_ERROR(errmsg.c_str()); } diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 09acd09ec0..eafc2c2a60 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -30,7 +30,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #if defined(_WIN32) #define IPC_CHANNEL_IMPLEMENTATION_WIN32 -#elif defined(__linux__) +#elif defined(__linux__) && 0 #define IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX #else #define IPC_CHANNEL_IMPLEMENTATION_POSIX @@ -71,7 +71,6 @@ struct IPCChannelBuffer #elif defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) pthread_cond_t cond; - clockid_t cond_clockid; pthread_mutex_t mutex; bool posted = false; // protected by mutex #endif From 733f8baa20b8eb4ffa979a4933f971d6522b88c3 Mon Sep 17 00:00:00 2001 From: Desour Date: Mon, 7 Oct 2024 16:42:05 +0200 Subject: [PATCH 28/29] clean up my pain --- src/threading/ipc_channel.cpp | 33 +++------------------------------ src/threading/ipc_channel.h | 2 +- 2 files changed, 4 insertions(+), 31 deletions(-) diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 1cfd074125..24a76f9016 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -128,10 +128,8 @@ static void post(IPCChannelBuffer *buf) noexcept // timeout is absolute (using cond_clockid) // returns false on timeout -static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout, const char *timestr = nullptr) noexcept +static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout) noexcept { - if (timestr) - errorstream << timestr << std::endl; bool timed_out = false; pthread_mutex_lock(&buf->mutex); while (!buf->posted) { @@ -140,16 +138,8 @@ static bool wait(IPCChannelBuffer *buf, const struct timespec *timeout, const ch if (err == ETIMEDOUT) { timed_out = true; break; - } else if (err == EINTR || err == EOWNERDEAD || err == ENOTRECOVERABLE || err == EPERM || err == EINVAL) { - continue; - } else if (err != 0) { - pthread_mutex_unlock(&buf->mutex); - auto msg = "err: " + std::to_string(err); - FATAL_ERROR(msg.c_str()); - bool ret = wait(buf, timeout, msg.c_str()); - errorstream << msg << std::endl; - return ret; } + FATAL_ERROR_IF(err != 0 && err != EINTR, "pthread_cond_timedwait failed"); } else { pthread_cond_wait(&buf->cond, &buf->mutex); } @@ -187,8 +177,6 @@ static bool wait_in(IPCChannelEnd::Dir *dir, u64 timeout_ms_abs) #else struct timespec timeout; struct timespec *timeoutp = nullptr; - char timestr[201]; - timestr[0] = '\0'; if (timeout_ms_abs > 0) { u64 tnow = porting::getTimeMs(); if (tnow > timeout_ms_abs) @@ -211,14 +199,9 @@ static bool wait_in(IPCChannelEnd::Dir *dir, u64 timeout_ms_abs) timeout.tv_sec += 1; } timeoutp = &timeout; - - time_t timeout_tt = timeout.tv_sec + timeout.tv_nsec / 1000'000'000L; - tm timeout_tm; - localtime_r(&timeout_tt, &timeout_tm); - strftime(timestr, 200, "%F %T ", &timeout_tm); } - return wait(dir->buf_in, timeoutp, timestr); + return wait(dir->buf_in, timeoutp); #endif } @@ -248,7 +231,6 @@ IPCChannelBuffer::IPCChannelBuffer() #if defined(IPC_CHANNEL_IMPLEMENTATION_POSIX) pthread_condattr_t condattr; pthread_mutexattr_t mutexattr; - clockid_t cond_clockid; if (pthread_condattr_init(&condattr) != 0) goto error_condattr_init; if (pthread_mutexattr_init(&mutexattr) != 0) @@ -257,26 +239,17 @@ IPCChannelBuffer::IPCChannelBuffer() goto error_condattr_setpshared; if (pthread_mutexattr_setpshared(&mutexattr, PTHREAD_PROCESS_SHARED) != 0) goto error_mutexattr_setpshared; - if (pthread_condattr_getclock(&condattr, &cond_clockid) != 0) - goto error_condattr_getclock; if (pthread_cond_init(&cond, &condattr) != 0) goto error_cond_init; if (pthread_mutex_init(&mutex, &mutexattr) != 0) goto error_mutex_init; pthread_mutexattr_destroy(&mutexattr); pthread_condattr_destroy(&condattr); -/* - { - std::string bla = std::string("realt: ") + std::to_string(CLOCK_REALTIME) + " cond_clockid: " + std::to_string(cond_clockid); - FATAL_ERROR(bla.c_str()); - }*/ - FATAL_ERROR_IF(cond_clockid != CLOCK_REALTIME, "wrong clock"); return; error_mutex_init: pthread_cond_destroy(&cond); error_cond_init: -error_condattr_getclock: error_mutexattr_setpshared: error_condattr_setpshared: pthread_mutexattr_destroy(&mutexattr); diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index eafc2c2a60..50da2e193d 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -30,7 +30,7 @@ with this program; if not, write to the Free Software Foundation, Inc., #if defined(_WIN32) #define IPC_CHANNEL_IMPLEMENTATION_WIN32 -#elif defined(__linux__) && 0 +#elif defined(__linux__) #define IPC_CHANNEL_IMPLEMENTATION_LINUX_FUTEX #else #define IPC_CHANNEL_IMPLEMENTATION_POSIX From bddc337594d2aeeecadf3ee9293572192b12e5ac Mon Sep 17 00:00:00 2001 From: Desour Date: Thu, 20 Feb 2025 22:55:47 +0100 Subject: [PATCH 29/29] update copyright header style (and "Luanti authors" instead of individual names) --- src/benchmark/benchmark_ipc_channel.cpp | 21 +++------------------ src/threading/ipc_channel.cpp | 22 +++------------------- src/threading/ipc_channel.h | 22 +++------------------- 3 files changed, 9 insertions(+), 56 deletions(-) diff --git a/src/benchmark/benchmark_ipc_channel.cpp b/src/benchmark/benchmark_ipc_channel.cpp index a8bcfad5ec..a0d25e1778 100644 --- a/src/benchmark/benchmark_ipc_channel.cpp +++ b/src/benchmark/benchmark_ipc_channel.cpp @@ -1,21 +1,6 @@ -/* -Minetest -Copyright (C) 2024 DS - -This program is free software; you can redistribute it and/or modify -it under the terms of the GNU Lesser General Public License as published by -the Free Software Foundation; either version 2.1 of the License, or -(at your option) any later version. - -This program is distributed in the hope that it will be useful, -but WITHOUT ANY WARRANTY; without even the implied warranty of -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -GNU Lesser General Public License for more details. - -You should have received a copy of the GNU Lesser General Public License along -with this program; if not, write to the Free Software Foundation, Inc., -51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. -*/ +// SPDX-FileCopyrightText: 2024 Luanti authors +// +// SPDX-License-Identifier: LGPL-2.1-or-later #include "catch.h" #include "threading/ipc_channel.h" diff --git a/src/threading/ipc_channel.cpp b/src/threading/ipc_channel.cpp index 24a76f9016..cebac56fa5 100644 --- a/src/threading/ipc_channel.cpp +++ b/src/threading/ipc_channel.cpp @@ -1,22 +1,6 @@ -/* -Minetest -Copyright (C) 2022 TurkeyMcMac, Jude Melton-Houghton -Copyright (C) 2024 DS - -This program is free software; you can redistribute it and/or modify -it under the terms of the GNU Lesser General Public License as published by -the Free Software Foundation; either version 2.1 of the License, or -(at your option) any later version. - -This program is distributed in the hope that it will be useful, -but WITHOUT ANY WARRANTY; without even the implied warranty of -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -GNU Lesser General Public License for more details. - -You should have received a copy of the GNU Lesser General Public License along -with this program; if not, write to the Free Software Foundation, Inc., -51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. -*/ +// SPDX-FileCopyrightText: 2024 Luanti authors +// +// SPDX-License-Identifier: LGPL-2.1-or-later #include "ipc_channel.h" #include "debug.h" diff --git a/src/threading/ipc_channel.h b/src/threading/ipc_channel.h index 50da2e193d..71bb2b2f60 100644 --- a/src/threading/ipc_channel.h +++ b/src/threading/ipc_channel.h @@ -1,22 +1,6 @@ -/* -Minetest -Copyright (C) 2022 TurkeyMcMac, Jude Melton-Houghton -Copyright (C) 2024 DS - -This program is free software; you can redistribute it and/or modify -it under the terms of the GNU Lesser General Public License as published by -the Free Software Foundation; either version 2.1 of the License, or -(at your option) any later version. - -This program is distributed in the hope that it will be useful, -but WITHOUT ANY WARRANTY; without even the implied warranty of -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -GNU Lesser General Public License for more details. - -You should have received a copy of the GNU Lesser General Public License along -with this program; if not, write to the Free Software Foundation, Inc., -51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. -*/ +// SPDX-FileCopyrightText: 2024 Luanti authors +// +// SPDX-License-Identifier: LGPL-2.1-or-later #pragma once