mirror of
https://github.com/ARMSX2/ARMSX3.git
synced 2026-08-24 16:58:52 -07:00
RSX: harden the semaphore event-stream fallback after adversarial review
Findings addressed (blind review, 8 lenses, see PR discussion): - Gate the fallback on HWCAP_EVTSTRM (new utils::has_wfe_event_stream()). The park's wake bound is the kernel's architected timer event stream; on a kernel that does not enable it, a monitor-less WFE parks until the next unrelated interrupt. Such devices now keep the pre-existing armed-spin behavior instead. - Fall through from the event-stream park to the armed one-shot instead of else-ing around it. On cores where the armed WFE parks, this re-arms the exclusive monitor every iteration, so wake-on-write is preserved even after the spin budget is spent; on Oryon the extra call returns immediately and costs nothing measurable. This also shrinks the window in which a written-then-overwritten semaphore value could go unobserved. - Fix the spin's early-out: the call passed a freshly re-read value as old_value, which the compiler sank to immediately before the ldaxr, making the compare a self-comparison that never fired (verified by disassembly). The loop now snapshots its top-of-iteration read and passes that, so an already-changed value returns without waiting on every core class. - Move spin_budget under ARCH_ARM64 (silences -Wunused-variable on x86). - Log awaited and observed values in the driver-recovery timeout message, so a timeout caused by a transient value is distinguishable in reports. - Rewrite the stale comments in place: spin_on_cacheline_once's event- stream rationale is core-class dependent (measured non-parking on Oryon); wait_for_event's usage rule now covers the sustained-idle fallback shape and names the HWCAP_EVTSTRM precondition. Device check after hardening (Odin 3, ME menu, 30 s): 164.1G instructions vs 179.5G for the previous commit and 402.7G pre-fix - the win holds.
This commit is contained in:
Zulux91
@@ -4,6 +4,7 @@
|
||||
|
||||
#include "Emu/RSX/RSXThread.h"
|
||||
#include "Emu/RSX/rsx_profiler.h"
|
||||
#include "util/sysinfo.hpp"
|
||||
|
||||
#include "context_accessors.define.h"
|
||||
|
||||
@@ -50,10 +51,24 @@ namespace rsx
|
||||
|
||||
u64 start = get_system_time();
|
||||
u64 last_check_val = start;
|
||||
u64 spin_budget = 0;
|
||||
|
||||
while (sema != arg)
|
||||
#if defined(ARCH_ARM64)
|
||||
u64 spin_budget = 0;
|
||||
// Without the kernel's architected timer event stream a monitor-less
|
||||
// WFE has no bounded wake, so the event-stream fallback below must
|
||||
// stay disabled and the wait keeps the armed-spin shape.
|
||||
const bool has_event_stream = utils::has_wfe_event_stream();
|
||||
#endif
|
||||
|
||||
while (true)
|
||||
{
|
||||
const RsxSemaphore observed = sema;
|
||||
|
||||
if (observed == arg)
|
||||
{
|
||||
break;
|
||||
}
|
||||
|
||||
if (RSX(ctx)->test_stopped())
|
||||
{
|
||||
RSX(ctx)->state += cpu_flag::again;
|
||||
@@ -76,8 +91,11 @@ namespace rsx
|
||||
|
||||
if ((current - start) > tdr)
|
||||
{
|
||||
// If longer than driver timeout force exit
|
||||
rsx_log.error("nv406e::semaphore_acquire has timed out. semaphore_address=0x%X", addr);
|
||||
// If longer than driver timeout force exit. The awaited and
|
||||
// last-observed values are logged so a timeout caused by a
|
||||
// transient value (stored, then overwritten before the waiter
|
||||
// observed it) is distinguishable from a never-signaled one.
|
||||
rsx_log.error("nv406e::semaphore_acquire has timed out. semaphore_address=0x%X, awaited=0x%X, observed=0x%X", addr, arg, static_cast<u32>(observed));
|
||||
break;
|
||||
}
|
||||
}
|
||||
@@ -100,18 +118,22 @@ namespace rsx
|
||||
// second instead of waiting. Give the armed form a short window first - on
|
||||
// cores where it parks it keeps its instant wake-on-write, and where it
|
||||
// does not it acts as a brief spin that still catches short waits - then
|
||||
// fall back to the event-stream wait, which parks on both classes and
|
||||
// bounds wake latency at the event-stream period.
|
||||
if (++spin_budget > 500)
|
||||
// interleave the event-stream park, which is what actually paces the loop
|
||||
// on a core whose armed WFE spins. The armed one-shot still runs on every
|
||||
// iteration (it costs ~nothing where it is broken), so on cores where it
|
||||
// parks, wake-on-write is kept even after the budget is spent.
|
||||
if (has_event_stream && ++spin_budget > 500)
|
||||
{
|
||||
utils::wait_for_event();
|
||||
}
|
||||
else
|
||||
#endif
|
||||
{
|
||||
// Wait until the value changes or until 100us pass.
|
||||
utils::spin_on_cacheline_once(atomic_sema, sema, 100);
|
||||
}
|
||||
// On x86 this waits on the address (umwait/mwaitx), bounded by the
|
||||
// timeout. On ARM the timeout is ignored: the wait ends on a write
|
||||
// to the armed cache line or an event-stream tick on cores where
|
||||
// the armed WFE parks, and returns immediately on cores where it
|
||||
// does not (measured on Oryon), where wait_for_event() above
|
||||
// provides the pacing instead.
|
||||
utils::spin_on_cacheline_once(atomic_sema, observed, 100);
|
||||
}
|
||||
|
||||
RSX(ctx)->fifo_wake_delay();
|
||||
|
||||
+18
-8
@@ -188,14 +188,19 @@ namespace utils
|
||||
|
||||
// Park the core until an event arrives, with no syscall.
|
||||
//
|
||||
// On arm64 WFE drops the core into a low-power state. Linux enables the architected
|
||||
// event stream, so a wakeup arrives on a fixed short period (tens of microseconds), and
|
||||
// timer interrupts wake it regardless -- it cannot stall indefinitely. SEVL sets the local
|
||||
// event first so the first WFE consumes it and the second genuinely parks, rather than
|
||||
// returning immediately on a stale event.
|
||||
// On arm64 WFE drops the core into a low-power state. When the kernel enables the
|
||||
// architected event stream a wakeup arrives on a fixed short period (tens of
|
||||
// microseconds), and timer interrupts wake it regardless -- it cannot stall
|
||||
// indefinitely. The event stream is a kernel property, not a guarantee: check
|
||||
// utils::has_wfe_event_stream() (HWCAP_EVTSTRM) before making this wait
|
||||
// load-bearing; without the stream the park lasts until the next unrelated
|
||||
// interrupt. SEVL sets the local event first so the first WFE consumes it and the
|
||||
// second genuinely parks, rather than returning immediately on a stale event.
|
||||
//
|
||||
// For polling loops whose exit condition is produced by another agent (GPU, kernel) on a
|
||||
// timescale of tens of microseconds or more. Everywhere else, pause() is still the tool.
|
||||
// For polling loops whose exit condition is produced by another agent (another
|
||||
// thread, GPU, kernel) on a timescale of tens of microseconds or more, including
|
||||
// as the sustained-idle fallback behind a short hot spin (see RSXFIFO idle and
|
||||
// nv406e::semaphore_acquire). Everywhere else, pause() is still the tool.
|
||||
inline void wait_for_event()
|
||||
{
|
||||
#if defined(ARCH_ARM64)
|
||||
@@ -283,7 +288,12 @@ namespace utils
|
||||
const void* addr = &var.raw();
|
||||
|
||||
#if defined(ARCH_ARM64)
|
||||
// WFE will wake from the periodic event stream, so the explicit timeout is ignored on ARM.
|
||||
// The explicit timeout is ignored on ARM. On cores where the armed WFE below
|
||||
// parks, the periodic event stream bounds the wait; but this is core-class
|
||||
// dependent -- on Oryon (Snapdragon 8 Elite class) WFE returns immediately
|
||||
// while the exclusive monitor is armed, making this a plain spin there. A
|
||||
// caller that needs a guaranteed pacing bound must provide it itself (see
|
||||
// nv406e::semaphore_acquire for the fallback pattern).
|
||||
(void)timeout_us;
|
||||
|
||||
using wait_type = std::remove_cvref_t<decltype(var.raw())>;
|
||||
|
||||
@@ -392,6 +392,26 @@ bool utils::has_neon()
|
||||
return g_value;
|
||||
}
|
||||
|
||||
bool utils::has_wfe_event_stream()
|
||||
{
|
||||
static const bool g_value = []() -> bool
|
||||
{
|
||||
#if defined(__linux__)
|
||||
// HWCAP_EVTSTRM: the kernel has enabled the architected timer event
|
||||
// stream (CNTKCTL_EL1.EVNTEN), which is what bounds a bare WFE's wake
|
||||
// latency. Waits that rely on WFE without an armed exclusive monitor
|
||||
// must check this; with the stream off, such a WFE parks until the
|
||||
// next unrelated interrupt.
|
||||
return (getauxval(AT_HWCAP) & HWCAP_EVTSTRM) != 0;
|
||||
#else
|
||||
// Unknown platforms: report false so callers stay on wait shapes that
|
||||
// do not depend on the event stream.
|
||||
return false;
|
||||
#endif
|
||||
}();
|
||||
return g_value;
|
||||
}
|
||||
|
||||
bool utils::has_sha3()
|
||||
{
|
||||
static const bool g_value = []() -> bool
|
||||
|
||||
@@ -57,6 +57,10 @@ namespace utils
|
||||
#ifdef ARCH_ARM64
|
||||
bool has_neon();
|
||||
|
||||
// True when the kernel advertises the architected timer event stream
|
||||
// (HWCAP_EVTSTRM) — the wake-latency bound for monitor-less WFE waits.
|
||||
bool has_wfe_event_stream();
|
||||
|
||||
bool has_sha3();
|
||||
|
||||
bool has_dotprod();
|
||||
|
||||
Reference in New Issue
Block a user