Repository navigation
fix(libkernel): wait for sigtimedwait timeouts with the precise timed wait on Windows - #11167
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed head e899a961c753d87521e080223438b42fb91064da.
The deadline is captured once before the wait loop using the monotonic TimedWait::NowNanos() clock, and every wakeup rechecks the matching pending-signal set before reporting EAGAIN. The existing nanosecond validation remains in sigtimedwait_nid_postfix; the pending-signal-first behavior is consistent with the FreeBSD 11.4 implementation. The large-tv_sec handling follows the existing ScanDeadline convention in Kqueue.cpp. The change also uses the shared alertable wait primitive for this signal condition.
Checks I verified: git diff --check upstream/main...HEAD passes; the three-way merge with current main is clean; eligibility, conventions, intake, labels and report checks pass. Product build jobs are skipped for this PR. I did not run the Windows timing benchmark locally; the Windows/Linux focused and full-suite results in the description are author-reported. The Windows median improvement is the central platform-specific result, so please retain the attached/repeatable focused test output with the PR for reviewers.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
thepoppfish
left a comment
There was a problem hiding this comment.
Independent Windows validation of exact head e899a961c753d87521e080223438b42fb91064da, following the request to retain repeatable Windows output.
Windows 11 build 26200, Intel Core Ultra 7 268V, MSYS2 UCRT64 GCC 16.2.0; C++20 -O1 -fno-inline -fno-lto, CMake 4.3.1/Ninja 1.13.2. Both executables compile the complete production Signals.cpp, one from actual parent 6174b50983ff5b340d36434381bffb8427d14466 and one from this head, with the same unchanged production TimedWait.cpp and headers. These two commits differ only in Signals.cpp; the parent is the PR's test-first commit with the previous wait implementation. This is a native focused harness, separate from the project's shared-library build.
Five fresh-process pairs, alternating order; 31 waits per timeout per process, 155 samples per cell. APS5_COARSE_TIMED_WAITS was unset. Every one of the 620 measured waits returned -1/guest errno 35 and took at least the requested duration.
| Requested | Parent median (range) | Head median (range) |
|---|---|---|
| 100 µs | 15.4455 ms (1.2195–22.826 ms) | 101.0 µs (100.6–244.2 µs) |
| 1 ms | 15.4888 ms (14.6658–16.6063 ms) | 1.0055 ms (1.0010–1.8405 ms) |
The external CTest suite passed 3/3, 0.86 s: common behavior on parent and head, plus the head's huge-timeout cases. It checks poll/negative timeouts, invalid nanoseconds, pending-first behavior including an invalid timeout, real raise_nid_postfix queueing from native sender threads, wakeups through sigtimedwait/sigwaitinfo/sigwait, and repeated unrelated notifications preserving a 40 ms deadline (parent 47.3201 ms; head 40.0062 ms). Both guest signals stay blocked and pending bits are drained before restoring the mask.
The huge-timeout probe on the head waits for and receives the real queued signal for both tv_sec=2147483648 and INT64_MAX. Replaying it on the parent is a separate expected-failure control: the former passes, but INT64_MAX immediately returns -1/35 in 8.9 µs before the sender queues the signal. It is deliberately excluded from the passing CTest count.
Boundary adapters: guest errno uses the real host thread's errno; libc registration stores the callback and interrupt-set pointer with the same atomic ordering as Process.cpp, and the probe verifies registration occurred. The unused guest getpid/pthread-self/liveness paths throw if reached. There is no replacement wait, pending queue, raise, clock or notification implementation. MinGW unwind sections are renamed after linking as shown below.
Scope: I did not run the complete committed GuestSigtimedwait fixture, the project's 40-test group/full build, guest pthread delivery, APC/exception delivery, Linux/macOS or a title. This corroborates the Windows timeout behavior; it is not a game-performance claim or a replacement for those checks. I have not established author agreement for the #8497/#9499 overlap ordering. Comment-only supporting validation. AI-assisted: yes.
Raw timing samples and expected-failure control
{"run": "parent-timing-0", "exit_code": 0}
{"case":"libc-registration","pass":true}
{"requested_ns":100000,"samples_ns":[9481700,16233200,15102300,15291600,15750600,15514500,15574700,15619000,15411100,15787500,15245300,16007100,14963300,15214100,15953200,15927400,15573100,15145600,15917900,15840800,15769000,15120100,16530500,15564000,16038100,15488300,16069200,16376600,15055100,15550000,15441500],"min_ns":9481700,"median_ns":15573100,"max_ns":16530500}
{"requested_ns":1000000,"samples_ns":[15553700,15606500,15656900,14861800,15375700,15414200,15206100,15942200,14903500,15338300,15002300,15679800,15454500,15480800,15758800,15086900,15554300,15557200,15140800,15944300,15043600,15790200,14933100,15193000,15299800,16103300,15986000,15297900,15543200,15546400,15175600],"min_ns":14861800,"median_ns":15454500,"max_ns":16103300}
{"failures":0}
{"run": "head-timing-0", "exit_code": 0}
{"case":"libc-registration","pass":true}
{"requested_ns":100000,"samples_ns":[103700,100700,100900,100900,101000,101100,101100,100900,100800,100900,101100,101000,100900,100800,101000,100800,100900,103200,103300,100900,102700,106900,101600,101700,101700,102300,101800,101200,101800,103000,101000],"min_ns":100700,"median_ns":101100,"max_ns":106900}
{"requested_ns":1000000,"samples_ns":[1039600,1444400,1053200,1005900,1001300,1066300,1091300,1003100,1002800,1002100,1003700,1053900,1003700,1002800,1018700,1108100,1026400,1402000,1001000,1002300,1008500,1005600,1004200,1049200,1006500,1004500,1002600,1003300,1002300,1023300,1004500],"min_ns":1001000,"median_ns":1005600,"max_ns":1444400}
{"failures":0}
{"run": "head-timing-1", "exit_code": 0}
{"case":"libc-registration","pass":true}
{"requested_ns":100000,"samples_ns":[103500,101100,100800,101000,100800,100900,100900,101000,100800,101100,100900,100900,100700,100800,100800,100900,100900,101100,100900,100700,106600,101800,101800,101100,100900,101300,101200,101400,101500,101300,101700],"min_ns":100700,"median_ns":101000,"max_ns":106600}
{"requested_ns":1000000,"samples_ns":[1005400,1003400,1192100,1032700,1338800,1012900,1006400,1003700,1004800,1006600,1093500,1032600,1063200,1120700,1013600,1003500,1008100,1002900,1003100,1002900,1002200,1003200,1002300,1003300,1004100,1008800,1004100,1002900,1002700,1002300,1002400],"min_ns":1002200,"median_ns":1004100,"max_ns":1338800}
{"failures":0}
{"run": "parent-timing-1", "exit_code": 0}
{"case":"libc-registration","pass":true}
{"requested_ns":100000,"samples_ns":[1219500,15829600,15276300,15531600,16288100,15086300,15658800,15407900,15319500,15540400,15182300,15205600,15251900,15158600,15978600,15816900,15046800,15564400,15651300,15271600,15279000,15755800,14985700,15157400,16160000,15230900,16047100,15465300,15612900,15207000,15342700],"min_ns":1219500,"median_ns":15342700,"max_ns":16288100}
{"requested_ns":1000000,"samples_ns":[15599400,15940900,15704900,15728300,15074600,15676800,15505700,15578700,15146700,15701800,15356800,15588400,15437100,16113900,15330800,15227000,15473200,15859600,15506800,15755900,16167600,15293800,15613700,15065700,15162500,15169000,15605900,15551400,15152300,16088600,15213400],"min_ns":15065700,"median_ns":15551400,"max_ns":16167600}
{"failures":0}
{"run": "parent-timing-2", "exit_code": 0}
{"case":"libc-registration","pass":true}
{"requested_ns":100000,"samples_ns":[8015400,14900800,15227200,15278000,15501500,15233500,15462700,15564300,15103700,14767800,15699200,15362900,15146300,15625200,15445600,15646600,15154100,15822800,15028600,15822700,15245700,15212300,15403300,15603100,15110000,15589600,15080700,16382800,15388100,15922500,15152700],"min_ns":8015400,"median_ns":15362900,"max_ns":16382800}
{"requested_ns":1000000,"samples_ns":[15944800,15108700,15357300,15827000,14828300,15051000,15448400,16062800,15180100,14964000,15352900,15613400,15291900,16190100,15884800,15075200,15255100,15695400,15017200,15770700,16111800,15886900,15206500,15278300,15335700,15439500,15329900,15057400,15480200,16188700,15111600],"min_ns":14828300,"median_ns":15352900,"max_ns":16190100}
{"failures":0}
{"run": "head-timing-2", "exit_code": 0}
{"case":"libc-registration","pass":true}
{"requested_ns":100000,"samples_ns":[106100,101700,100800,100900,100700,101800,103600,101100,100900,101000,100900,101000,101100,100800,100900,100700,100900,100900,101000,100700,102600,101000,100800,101000,100900,100900,100700,100800,101000,136800,101300],"min_ns":100700,"median_ns":100900,"max_ns":136800}
{"requested_ns":1000000,"samples_ns":[1004100,1004100,1002500,1002600,1003800,1004000,1007900,1005600,1004400,1005300,1003800,1002900,1002000,1004800,1001500,1005200,1005100,1007600,1005600,1034000,1002800,1015800,1003200,1002400,1010500,1007100,1029000,1029100,1002900,1004100,1002500],"min_ns":1001500,"median_ns":1004100,"max_ns":1034000}
{"failures":0}
{"run": "head-timing-3", "exit_code": 0}
{"case":"libc-registration","pass":true}
{"requested_ns":100000,"samples_ns":[103300,100800,100700,100900,100600,100700,100900,100800,100700,100800,109800,100700,101000,100700,100900,101300,100800,101000,101000,100900,101000,100900,100700,100700,100700,100900,101000,244200,102900,101100,100900],"min_ns":100600,"median_ns":100900,"max_ns":244200}
{"requested_ns":1000000,"samples_ns":[1008000,1005400,1004000,1840500,1006000,1391900,1048300,1015500,1135200,1003400,1003400,1122300,1015200,1414900,1006700,1006400,1003600,1013000,1006200,1003600,1033600,1002800,1004300,1062400,1002300,1496000,1004000,1060500,1038300,1003300,1142400],"min_ns":1002300,"median_ns":1008000,"max_ns":1840500}
{"failures":0}
{"run": "parent-timing-3", "exit_code": 0}
{"case":"libc-registration","pass":true}
{"requested_ns":100000,"samples_ns":[15005400,15374300,15818600,16334500,15540000,15769300,22826000,7987300,15292500,15626500,14810700,15509500,15445500,15580800,15058200,16127000,15604800,15627000,15618500,15300800,15687600,15244800,15290000,15131100,15658400,15162700,15825500,15016400,15121500,15402500,15250300],"min_ns":7987300,"median_ns":15445500,"max_ns":22826000}
{"requested_ns":1000000,"samples_ns":[15134000,15157900,15293900,15041000,15769900,16389000,15335800,15106700,15274200,15907900,16122800,15474100,15552900,15199600,15104600,15488800,15376700,15715600,15606900,15962900,15801200,15655500,15591800,15672300,15189000,15578700,15486000,15577400,16472400,15384300,15884800],"min_ns":15041000,"median_ns":15552900,"max_ns":16472400}
{"failures":0}
{"run": "parent-timing-4", "exit_code": 0}
{"case":"libc-registration","pass":true}
{"requested_ns":100000,"samples_ns":[8778100,15935400,15006900,15701200,15251500,15560600,15669000,15621400,15905700,15286100,15220000,15529100,15164300,15903900,14902600,15106500,15272300,15744900,15831200,15204200,15098100,15292400,14999900,15727000,16059900,15802700,15078800,15193400,15570100,15548000,15277700],"min_ns":8778100,"median_ns":15292400,"max_ns":16059900}
{"requested_ns":1000000,"samples_ns":[15006300,15350200,15521100,16276900,16606300,15160300,15627800,15484500,15198200,16167100,15045100,15424800,16103500,15477800,15525600,15591500,15988200,14665800,15522000,16119100,16177100,15851200,14869100,15500000,15272800,15244100,15643000,16016100,15959800,14808000,15585600],"min_ns":14665800,"median_ns":15522000,"max_ns":16606300}
{"failures":0}
{"run": "head-timing-4", "exit_code": 0}
{"case":"libc-registration","pass":true}
{"requested_ns":100000,"samples_ns":[103600,101600,100700,101000,102600,107400,101900,101900,101800,102800,101200,100900,101600,100900,100700,100800,102700,100800,100900,100700,100700,100700,100800,100800,100900,101000,101200,100700,133900,102000,101900],"min_ns":100700,"median_ns":101000,"max_ns":133900}
{"requested_ns":1000000,"samples_ns":[1005400,1007800,1005300,1002900,1051000,1033900,1027400,1005500,1025500,1441200,1002900,1008300,1004900,1004100,1007400,1005600,1003100,1011900,1002600,1007700,1005300,1002600,1018900,1002500,1073400,1002400,1029000,1024800,1035800,1007500,1027100],"min_ns":1002400,"median_ns":1007500,"max_ns":1441200}
{"failures":0}
{"run": "parent-huge-negative-control", "exit_code": 1}
{"case":"libc-registration","pass":true}
{"case":"huge-pending-wake","pass":true}
{"seconds":2147483648,"result":13,"errno":0,"elapsed_ns":14648600}
{"case":"huge-pending-wake","pass":false}
{"seconds":9223372036854775807,"result":-1,"errno":35,"elapsed_ns":8900}
{"failures":1}Complete external harness and replay
Place review-11167 and native-signal11167 beside each other. With Git, Python, MinGW GCC, CMake and Ninja on PATH, save the three files below under native-signal11167, then run these commands. The parent source is extracted as bytes from the pinned Git object; neither production source is edited.
git clone --no-checkout https://github.com/boykopovar/AnyPS5.git review-11167
git -C review-11167 checkout --detach e899a961c753d87521e080223438b42fb91064da
python -c "import pathlib,subprocess; pathlib.Path('native-signal11167/ParentSignals.cpp').write_bytes(subprocess.check_output(['git','-C','review-11167','show','6174b50983ff5b340d36434381bffb8427d14466:core/libs/prx/libkernel/System/src/Signals.cpp']))"
Remove-Item Env:APS5_COARSE_TIMED_WAITS -ErrorAction SilentlyContinue
cmake -S native-signal11167 -B native-signal11167/build -G Ninja -DCMAKE_BUILD_TYPE=Release
cmake --build native-signal11167/build -j 4
ctest --test-dir native-signal11167/build --output-on-failure -VFor timing, run parent_probe.exe timing and head_probe.exe timing from that build directory five times each in alternating order, preserving their JSON output. Each invocation prints all 31 samples for each timeout and exits nonzero for an incorrect result or an early return. Run parent_probe.exe huge separately for the expected failure (exit 1); head_probe.exe huge should exit 0. The local orchestrator applied an 8 s process timeout; each CTest case also has an 8 s timeout.
CMakeLists.txt
cmake_minimum_required(VERSION 3.20)
project(SignalWaitReview LANGUAGES CXX)
get_filename_component(DEFAULT_REPO "${CMAKE_CURRENT_LIST_DIR}/../review-11167" ABSOLUTE)
set(REPO "${DEFAULT_REPO}" CACHE PATH "Reviewed source checkout")
enable_testing()
foreach(variant IN ITEMS parent head)
if(variant STREQUAL parent)
set(source "${CMAKE_CURRENT_LIST_DIR}/ParentSignals.cpp")
else()
set(source "${REPO}/core/libs/prx/libkernel/System/src/Signals.cpp")
endif()
add_executable(${variant}_probe Probe.cpp Support.cpp "${source}" "${REPO}/core/libs/prx/libkernel/Time/TimedWait.cpp")
target_compile_features(${variant}_probe PRIVATE cxx_std_20)
target_compile_options(${variant}_probe PRIVATE -O1 -g -fno-inline -fno-lto -ffunction-sections -fdata-sections)
target_include_directories(${variant}_probe PRIVATE "${REPO}/core/libs")
target_link_options(${variant}_probe PRIVATE -static-libgcc -static-libstdc++ -Wl,--gc-sections)
add_custom_command(TARGET ${variant}_probe POST_BUILD COMMAND "${CMAKE_OBJCOPY}" --rename-section .eh_frame=.ehfram --rename-section .eh_fram=.ehfram "$<TARGET_FILE:${variant}_probe>" VERBATIM)
add_test(NAME ${variant}_behavior COMMAND ${variant}_probe behavior)
set_tests_properties(${variant}_behavior PROPERTIES TIMEOUT 8)
endforeach()
add_test(NAME head_huge_timeout COMMAND head_probe huge)
set_tests_properties(head_huge_timeout PROPERTIES TIMEOUT 8)
Support.cpp
#include "prx/libkernel/include/Api.hpp"
#include "prx/libkernel/Pthread/include/Pthread.hpp"
#include "prx/libc/include/Api.hpp"
#include "prx/libc/include/General.hpp"
#include <atomic>
#include <cerrno>
#include <stdexcept>
namespace {
std::atomic<SigactionFn_nid_no_patch> registered{nullptr};
std::atomic<std::uint32_t*> interruptSet{nullptr};
}
extern "C" void RegisterSigaction_nid_no_patch(SigactionFn_nid_no_patch fn, std::uint32_t* set) {
interruptSet.store(set, std::memory_order_release);
registered.store(fn, std::memory_order_release);
}
bool RegistrationAvailable() {
return registered.load(std::memory_order_acquire) != nullptr && interruptSet.load(std::memory_order_acquire) != nullptr;
}
extern "C" int* APS5_VABI __error_nid_postfix() { return &errno; }
extern "C" int APS5_VABI getpid_nid_postfix() { throw std::runtime_error("unexpected guest pid path"); }
extern "C" Pthread APS5_VABI scePthreadSelf() { throw std::runtime_error("unexpected guest pthread path"); }
bool GuestThreadIsLive(const PthreadPrivate*) { throw std::runtime_error("unexpected guest pthread liveness path"); }
Probe.cpp
#include "prx/libkernel/include/Api.hpp"
#include <algorithm>
#include <atomic>
#include <cerrno>
#include <chrono>
#include <cstdint>
#include <cstdio>
#include <limits>
#include <stdexcept>
#include <string>
#include <thread>
#include <vector>
bool RegistrationAvailable();
using Clock = std::chrono::steady_clock;
using namespace std::chrono_literals;
namespace {
GuestSignalSet target{{1u << 12, 0, 0, 0}};
GuestSignalSet unrelated{{1u << 1, 0, 0, 0}};
GuestSignalSet both{{(1u << 12) | (1u << 1), 0, 0, 0}};
GuestSignalSet previous{};
int failures = 0;
bool masked = false;
void Check(const char* name, bool ok) {
std::printf("{\"case\":\"%s\",\"pass\":%s}\n", name, ok ? "true" : "false");
if (!ok) ++failures;
}
std::int64_t Elapsed(Clock::time_point start) {
return std::chrono::duration_cast<std::chrono::nanoseconds>(Clock::now() - start).count();
}
void Drain(const GuestSignalSet& set) {
KernelTimespec zero{0, 0};
while (sigtimedwait_nid_postfix(&set, nullptr, &zero) > 0) {}
}
void Cleanup() {
if (!masked) return;
Drain(both);
if (_sigprocmask_nid_postfix(3, &previous, nullptr) != 0) ++failures;
masked = false;
}
void Behavior() {
KernelTimespec zero{0, 0};
KernelTimespec negative{-1, 0};
KernelTimespec invalid{0, 1000000000};
errno = 0;
Check("zero-empty", sigtimedwait_nid_postfix(&target, nullptr, &zero) == -1 && errno == 35);
errno = 0;
Check("negative-empty", sigtimedwait_nid_postfix(&target, nullptr, &negative) == -1 && errno == 35);
errno = 0;
Check("invalid-nanos-empty", sigtimedwait_nid_postfix(&target, nullptr, &invalid) == -1 && errno == 22);
for (auto* timeout : {&zero, &negative, &invalid}) {
Check("enqueue-pending", raise_nid_postfix(13) == 0);
errno = 1234;
Check("pending-first", sigtimedwait_nid_postfix(&target, nullptr, timeout) == 13 && errno == 1234);
}
for (int mode = 0; mode < 4; ++mode) {
std::atomic<int> raised{-999};
std::thread sender([&] {
std::this_thread::sleep_for(5ms);
raised.store(raise_nid_postfix(13));
});
KernelTimespec second{1, 0};
int received = 0;
const auto start = Clock::now();
const int result = mode == 0 ? sigtimedwait_nid_postfix(&target, nullptr, &second) :
mode == 1 ? sigtimedwait_nid_postfix(&target, nullptr, nullptr) :
mode == 2 ? sigwaitinfo_nid_postfix(&target, nullptr) : sigwait_nid_postfix(&target, &received);
const auto elapsed = Elapsed(start);
sender.join();
Check("sender-wake", raised.load() == 0 && (mode == 3 ? result == 0 && received == 13 : result == 13));
std::printf("{\"wake_mode\":%d,\"elapsed_ns\":%lld}\n", mode, static_cast<long long>(elapsed));
Drain(target);
}
std::atomic<int> sends{0};
std::atomic<int> sendErrors{0};
std::thread unrelatedSender([&] {
const auto until = Clock::now() + 160ms;
do {
std::this_thread::sleep_for(5ms);
if (raise_nid_postfix(2) != 0) ++sendErrors;
++sends;
} while (Clock::now() < until);
});
KernelTimespec timeout{0, 40000000};
errno = 0;
const auto start = Clock::now();
const int result = sigtimedwait_nid_postfix(&target, nullptr, &timeout);
const int error = errno;
const auto elapsed = Elapsed(start);
unrelatedSender.join();
Check("unrelated-wakes-preserve-deadline", result == -1 && error == 35 && elapsed >= 40000000 && elapsed < 120000000 && sends.load() >= 2 && sendErrors.load() == 0);
std::printf("{\"unrelated_elapsed_ns\":%lld,\"sends\":%d}\n", static_cast<long long>(elapsed), sends.load());
Drain(unrelated);
}
void Timing() {
for (const long nanos : {100000L, 1000000L}) {
KernelTimespec timeout{0, nanos};
std::vector<std::int64_t> samples;
for (int i = 0; i < 31; ++i) {
errno = 0;
const auto start = Clock::now();
const int result = sigtimedwait_nid_postfix(&target, nullptr, &timeout);
const auto elapsed = Elapsed(start);
if (result != -1 || errno != 35 || elapsed < nanos) ++failures;
samples.push_back(elapsed);
}
std::printf("{\"requested_ns\":%ld,\"samples_ns\":[", nanos);
for (std::size_t i = 0; i < samples.size(); ++i) std::printf("%s%lld", i ? "," : "", static_cast<long long>(samples[i]));
std::sort(samples.begin(), samples.end());
std::printf("],\"min_ns\":%lld,\"median_ns\":%lld,\"max_ns\":%lld}\n", static_cast<long long>(samples.front()), static_cast<long long>(samples[15]), static_cast<long long>(samples.back()));
}
}
void Huge() {
for (auto seconds : {std::int64_t{2147483648LL}, std::numeric_limits<std::int64_t>::max()}) {
KernelTimespec timeout{seconds, 0};
std::atomic<int> raised{-999};
std::thread sender([&] {
std::this_thread::sleep_for(5ms);
raised.store(raise_nid_postfix(13));
});
errno = 0;
const auto start = Clock::now();
const int result = sigtimedwait_nid_postfix(&target, nullptr, &timeout);
const int error = errno;
const auto elapsed = Elapsed(start);
sender.join();
Check("huge-pending-wake", result == 13 && raised.load() == 0);
std::printf("{\"seconds\":%lld,\"result\":%d,\"errno\":%d,\"elapsed_ns\":%lld}\n", static_cast<long long>(seconds), result, error, static_cast<long long>(elapsed));
Drain(target);
}
}
}
int main(int argc, char** argv) {
try {
Check("libc-registration", RegistrationAvailable());
if (_sigprocmask_nid_postfix(1, &both, &previous) != 0) throw std::runtime_error("block failed");
masked = true;
if (argc != 2) throw std::invalid_argument("mode required");
const std::string mode = argv[1];
if (mode == "behavior") Behavior();
else if (mode == "timing") Timing();
else if (mode == "huge") Huge();
else throw std::invalid_argument("unknown mode");
Cleanup();
} catch (const std::exception& error) {
std::fprintf(stderr, "%s\n", error.what());
++failures;
Cleanup();
}
std::printf("{\"failures\":%d}\n", failures);
return failures == 0 ? 0 : 1;
}
|
Thanks for adding the independent paired Windows measurements on the exact reviewed head; they address my request for repeatable output and substantiate the short-timeout improvement, with the harness limitations clearly stated. Since #9499 merged, I refreshed the current-base preview: |
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Reviewed head e899a961c7 by reading the diff and the existing review thread. I did not build or run this, so everything below is from the code, not from a test of mine.
What it does. sigtimedwait waits on TimedWait::Condition with a deadline from the monotonic TimedWait::NowNanos() clock, computed once before the loop, instead of std::condition_variable::wait_until on steady_clock. On Windows that routes the wait through the project's precise timed wait rather than the coarse standard condition variable.
Checked the semantics.
TimedWait::Condition::WaitUntilreturnstruewhen signalled andfalseon timeout in both the Windows and the portable branch, soif (WaitUntil(...)) continue;and the fall-through to theEAGAINreturn are right. The finalPendingInSetre-check after a timeout handles the race where the signal arrives as the wait expires.SignalWaitDeadlinereturnsnowfor a negativetv_sec(immediate timeout, as before), and atv_secaboveINT32_MAXbecomes "wait forever". The nanosecond addition cannot overflow auint64fortv_sec <= INT32_MAX, because that is about 2.1e18 against a 1.8e19 limit.- The Windows
Conditionenqueues a waiter before unlocking, andMarkPendingnotifies after the state change, so I don't see a lost wake-up.
Concerns:
- The new test asserts wall-clock time.
Require(median < 12 ms)for 0.1 ms and 1 ms timeouts is meant to prove the precise wait, but on a loaded shared CI runner the median of 31 samples can exceed 12 ms for reasons unrelated to this change, and it will then fail on Linux as well. The paired Windows measurements in the thread are the real evidence. I'd keep the lower bound (sample >= nanos, which is deterministic) and move the upper bound to the manual measurement, or make it tolerant. - The test prints to
stderron every run; fine for a measurement, noisy otherwise. - The PR is DIRTY. It needs a rebase onto current
main, so my review is of the diff, not of a merge result.
The logic is correct as far as I can read it. I'm leaving a comment instead of approving because of the conflict and the timing assertion.
AI-assisted: yes. Written with AI assistance and reviewed by me.
e899a96 to
d94c409
Compare
|
Windows and Linux runs of head
AI-assisted: yes (Claude Code); results above were run on this machine. |
What
On Windows,
sigtimedwaitrounds a short timeout up to the host's coarse tick: a 100 µs timeout returns after 15.6 ms (median of 31 waits).WaitForPendingInSetincore/libs/prx/libkernel/System/src/Signals.cppwaits on astd::condition_variable, and MinGW'swait_untilgoes through winpthreads and itsGetTickCount64-based timeout. This is another call site of #1471; #7735, #8129 and #8507 fixed others the same way.FreeBSD 11.4
kern_sigtimedwaittakes the deadline fromgetnanouptimeon entry, sleeps the remainder converted bytvtohz(1/hz, 1 ms at the defaulthzof 1000), and returnsEAGAINonce the deadline passes. A timeout too large to reach is effectively a wait without a deadline there, sincetvtohzclamps and the loop sleeps again.The change:
PendingSignals::changedis aTimedWait::Condition, the waiter libkernel already uses for event flags, equeues, semaphores and threads.MarkPendingwakes it withNotifyAll;sigwait,sigwaitinfo,pauseandsigsuspendwait on it withWait.sigtimedwaitcomputes its deadline fromTimedWait::NowNanos()on entry and waits withWaitUntil. A negativetv_sechas already expired; atv_secaboveINT32_MAXwaits without a deadline, asScanDeadlinedoes forkevent. This also removes the signed overflow the oldsteady_clocksum hit for a very largetv_sec.TimedWait::Conditionwait, so an exception raised for a thread blocked in one of these calls runs during the wait instead of when it returns, as thesceKernelRaiseExceptionentry in technical debt describes for alertableTimedWaitwaits.The alarm thread (
setitimer,alarm) keeps its ownstd::condition_variable; it is a separate call site.Tested
Windows 11 Pro, Intel Core i5-14600KF, WinLibs GCC 15.2.0, full Release build:
guest_sigtimedwaitgetsCheckShortTimeouts: 31 waits each at 100 µs and 1 ms on a signal that is never raised. Each must fail withEAGAINno earlier than its timeout, and the median must stay under 12 ms, as in fix(rudp): use precise poll deadlines on Windows #8507's RUDP test. It runs on every host.main'sSignals.cppit fails on Windows: 100 µs median15614300 ns.--repeat until-fail:10: 100 µs medians100600–100900 ns, 1 ms medians1002200–1022200 ns.sig|signal|pause|alarm|itimer|pselect|exception|raisepass with--repeat until-fail:3.ctest:99% tests passed, 5 tests failed out of 2378.agc_write_tracking_metadata_commit,agc_host_import_cut_lifetime,agc_driver_depth_plane_partial_storeandagc_driver_image_plane_address_on_linefail onmainon this machine too, and so doesguest_wide_formatting(wcsftime length matches strftime:%Zgives the host's non-ASCII zone name here, which fix(libc): convert the Windows time zone name from the ANSI code page to UTF-8 for tzname, %Z and tm_zone #10981 and fix(libc): convert Windows time zone names to UTF-8 #10987 address).check_skips.py: all skips expected.Linux (Ubuntu 24.04 container, GCC 13.3.0):
guest_sigtimedwaitalready passes onmainthere (100 µs median171116 ns, 1 ms1081058 ns), since glibc's wait is precise; with the change the 24 tests matching the same pattern there pass with--repeat until-fail:3(100 µs medians157792–167113 ns). Fullctest:99% tests passed, 24 tests failed out of 2285; the same 24 fail onmainin this container (22agc_*on its lavapipe and kernel withoutUFFD_FEATURE_WP_ASYNC,pr_overlap_encodingandguest_system_load_exec_standard_error).macOS: not tested.
Rebased onto
mainb90d009d9(a conflict besideDeliverUnblockedPending, which a merged signal-mask fix added): the 41 tests matchingsig|signal|pause|alarm|itimer|pselect|exception|raisepass on Windows with--repeat until-fail:3, with medians of100500 ns(100 µs) and1019700 ns(1 ms). Linux was not run again for this rebase; the code is unchanged.Overlaps are textual only: #8497 changes the signature of
WaitForPendingInSetand adds lines toGuestSigtimedwait.cpp, and #9499 edits the line just aboveWaitForPendingInSet. Neither changes how the wait itself works; whichever lands second rebases with a small conflict.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)Refs #1471