realtime: wake the interpreter from a dedicated timer thread

[probonopd]

Alternative to #216 with the same goal (drop the idle desktop to ~3% host CPU in realtime mode), but instead of polling the host clock in the interpreter loop it uses a dedicated timer thread that sleeps until the next guest timer deadline and then raises exec_timer, waking the interpreter only to process due timers.

Additional differences from #216:

• The idle throttle now stays disengaged while the user is interacting: the host event poller marks input via mark_host_input and guest_is_idle refuses to sleep right after an input event, so moving the mouse or typing never feels sluggish while throttled.
• The throttle sleep/burst cycle is guarded by g_idle_throttle_active, so the timer thread cannot cut a servicing burst short or race the idle decision (which caused full-speed slices and CPU flapping).
• The confirm/uptime gates are lowered from 15 s to 6 s / 8 s so the throttle engages sooner after the guest settles.
• Realtime globals (exec_timer, g_realtime, g_nanoseconds_base, g_idle_cpu_save) are now atomic since they are touched by both the emulation thread and the timer thread.

Tested end to end on the Power Macintosh G3 machine: normal boot to the desktop at full CPU, settled idle desktop at ~3% host CPU, and full-speed responsiveness while interacting.

[joevt]

Today I learned about std::memory_order_relaxed.

I think the TimerManager changes can be its own commit. I like to split large changes into smaller commits:

• First, commits that don't affect anything (such as cleanup or rename or add a method).
• Second, commits that do the large changes, perhaps using changes from the earlier commits.

So, another commit in the early category that should be added is something like this: ppcexec: Change thread accessed variables to atomic.. This commit doesn't affect the intent of the existing code. It makes the existing code more correct. A benchmark would show the impact of the change. But a negative impact doesn't mean the change shouldn't exist.

The point of the early commits is to make the large commit more focused on the main feature being implemented.

In guest_idle, you have a call to get_virt_time_ns() and cpu_now_ns(). Can cpu_now_ns() be replaced with now_ns? (taking into account that get_virt_time_ns is offset by g_nanoseconds_base)
g_nanoseconds_base is usually never changed during execution. I wonder if there's a way to get rid of it? It exists so that toggling g_realtime can work without affecting the TimerManager queue.

In ppc_exec_until and ppc_exec_dbg, you removed volatile. I think that was added there because of the setjmp. There's a comment in cpu/ppc/CMakeLists.txt:

# The use of volatile is deprecated, but we still need it to avoid function
# parameters being clobbered when using setjmp/longjmp

I don't know for sure that volatile is required. I have not experienced that problem myself.

[probonopd]

Thanks for the detailed review! I've restructured the history and addressed all four points.

1. Split into smaller commits

I rewrote the branch into focused commits:

• core/timermanager: add get_next_timeout_ns() - pure addition, no behavior change (8b94e71)
• ppcexec: change thread-accessed variables to atomic - makes the existing code more correct without changing behavior (1c8b641)
• realtime: wake the interpreter from a dedicated timer thread - the main feature, now only containing the timer thread, the input-wake and the throttle gating (44fc709)

2. cpu_now_ns() vs now_ns

You're right. mark_host_input() now stores get_virt_time_ns() and guest_is_idle() compares against its already-computed now_ns, so the extra clock read is gone.

3. g_nanoseconds_base

It's kept. It is constant during execution (written only at CPU init and on the g_realtime toggle) and exists so that toggling g_realtime does not shift the TimerManager queue, which holds guest-time deadlines. Because it is constant during execution, comparing two guest-time readings as in point 2 is exact.

4. volatile on ppc_exec_until / ppc_exec_dbg

Restored - the removal was unrelated to this PR, and cpu/ppc/CMakeLists.txt documents that volatile is needed to avoid function parameters being clobbered by setjmp/longjmp. Those lines are untouched by the PR now.

[joevt]

Very nice. For changes that affect the cpu execution, some before and after benchmarks would help characterize the changes as benign or not.

There's the bench1 target in dingusppc. It does no I/O so I don't know how the throttling might affect that. But that shouldn't matter since throttling applies mostly to the g_realtime = true mode and this benchmark has g_realtime = false. I suppose what we really want is for the g_realtime = false mode to not decrease in performance drastically since that is the default mode.

I made a benchmark that uses the time of day since that is mostly accurate in Mac OS 9 or earlier on DingusPPC regardless of mode.
https://68kmla.org/bb/threads/lets-see-your-best-disk-speeds-ppc-68k.49268/post-554194

We might want a method to obtain host nanoseconds in the guest environment by creating an unused PPC special purpose register. Or by adding a special register in the emulated mac-io chip. Or by utilizing special guest CPU instructions that are normally illegal ops. That's something to think about for another time.

A third possible benchmark is to measure boot time. Choose a bootable disk image that is easily obtainable such as one from the mihaip/infinite-mac repository (the Releases section has larger disk images that are not in the repository). These may be missing partition tables which infinitemac or my fork adds automatically to make them usable as a harddisk in DingusPPC. I don't know if boot time is a great metric - because the system is usually unusable during that time. The end time used by the boot time measurement should be the timestamp of an event that exists in the DingusPPC log. If no suitable logged event exists, then perhaps a startup app can be made to trigger a logged event.

As for these commits, I would add them in this order (earliest to latest):

• ppcexec: Change thread-accessed variables to atomic
• timermanager: Add get_next_timeout_ns()
• realtime: Add a --realtime command line flag
• realtime: throttle an idle guest in realtime mode to save host CPU
• realtime: wake the interpreter from a dedicated timer thread

Maybe the last two should be combined. Or maybe not.The switch to using a dedicated timer thread for the realtime mode's event process triggering is a big change by itself. Did that dedicated timer thread change necessitate the other changes? Or were the other changes made as corrections/improvements to the previous commit? If the latter then perhaps those changes should be applied to the previous commit.

[probonopd]

These may be missing partition tables which infinitemac or my fork adds automatically to make them usable as a harddisk in DingusPPC.

I came across that topic and was wondering whether https://github.com/dingusdev/dingusppc/ could/should do the same.

[joevt]

I came across that topic and was wondering whether https://github.com/dingusdev/dingusppc/ could/should do the same.

My fork has these commits (from latest to earliest):

• Use MetaImgFile for hard disks.
• metaimagefile: Use direct filesystem operations.
• metaimagefile: Add missing include.
• Add multi-file disk image support.

Looks like I misspelled metaimgfile. I'll do a reword.

infinite-mac also uses Add multi-file disk image support. but it applies the changes only to ATA hard disks - not SCSI hard disks? See Use MetaImgFile to allow ATA-based machines to mount multiple disks

[dingusdev]

The main issue I had with the MetaImgFile stuff was that it uses a header file from Apple directly. As it had no explicit source license, I opted to not include it. An open-source replacement would be accepted.

[joevt]

The main issue I had with the MetaImgFile stuff was that it uses a header file from Apple directly. As it had no explicit source license, I opted to not include it. An open-source replacement would be accepted.

What about versions from Darwin which have the APSL licence? I've made changes to the Add multi-file disk image support. commit.

[dingusdev]

I think those versions would be covered by Apple's terms under "Larger Works", in which case it would be acceptable. We will also need to include a copy of the APSL license.

[probonopd]

Reordered the history to your suggested order (earliest to latest) and folded the corrections into the throttle commit:

• ppcexec: change thread-accessed variables to atomic
• core/timermanager: add get_next_timeout_ns()
• realtime: add a --realtime command line flag
• realtime: throttle an idle guest in realtime mode to save host CPU (now self-contained, with the input-wake, the lowered 6s/8s confirm gates and the IDLE_MAX_SLEEP_NS sleep cap folded in)
• realtime: wake the interpreter from a dedicated timer thread (now just the loop-condition reorder plus the dedicated thread)

To answer your question: the dedicated timer thread change does necessitate two of the others. It is what makes the atomic conversion of exec_timer / g_idle_throttle_active necessary (exec_timer is now written from a third thread), and it is the reason get_next_timeout_ns() exists (the thread needs to peek at the next timer deadline to know how long to sleep). The --realtime flag is independent but small. The remaining changes (host-input wake in guest_is_idle, the lowered confirm/uptime gates, the 16 ms sleep cap) were corrections/improvements to the throttle behavior itself, so I applied them to the throttle commit as you suggested, rather than leaving them in the wake commit.

I also corrected the wake commit message: the previous wording claimed a per-instruction realtime deadline check, which the final diff does not have - it is only the loop-condition reorder plus the dedicated thread. I kept the last two commits separate rather than combining them, since the throttle commit now stands alone and the timer thread is the orthogonal change.

Regarding benchmarks: I have not added before/after numbers for g_realtime = false yet. The throttle/wake paths are gated on g_realtime and g_idle_cpu_save (both false by default), so the default path only sees the atomic conversions plus process_timers() returning the same value it already returned; I did boot-test the final tree in realtime mode with --idle-cpu-save and the throttle engages as before.

[joevt]

I will include these commits in my fork. I'll place them before @mihaip 's ppc: Model configurable CPU frequencies commit ( #210 ) because that one is still a draft. His commit relates mostly to the g_realtime = false mode.

[joevt]

I have added your commits (as they currently exist) into my fork just before @mihaip 's commits). I have these issues/questions (no testing has been done by me yet):

1. ==================

About this change in ppc_exec.cpp:

        // In realtime mode guest time is the wall clock, so there is no
        // per-instruction time to advance; the instruction budget only
        // bounds the idle burst so a fast host cannot over-run it. The
        // burst is never cut short because neither exec_timer writer (the
        // realtime timer thread and force_cycle_counter_reload) raises it
        // while the throttle is active; the throttle fires due timers
        // itself on wakeup. In non-realtime mode the throttle is never
        // active, so this single condition reduces to plain exec_timer
        // handling and avoids an atomic load and branch on every
        // instruction.
        if (exec_timer.load(std::memory_order_relaxed) || g_icycles++ >= max_cycles) [[unlikely]]
            max_cycles = process_events();

Doesn't the short-circuit evaluation of the || operator mean that g_icycles++ >= max_cycles does not always happen? That means the non-realtime time (or virtual time, which is g_icycles) will be incorrect? My fork has commit ppcexec: Add ppc_exec functions for g_realtime mode (in progress). to fix this by having different ppc_exec_inner functions for realtime mode.

The description in the comment is missing some details (I'm probably missing something)

• In realtime mode guest time is the wall clock, so there is no per-instruction time to advance
  • That makes sense. g_icycles is the virtual time and is only valid for non realtime mode.
• the instruction budget only bounds the idle burst so a fast host cannot over-run it
  • Not sure what an "instruction budget" is. What's the quantity being budgeted? Instructions per second?
  • Not sure what "bounding the idle burst" means. What are the bounds? What's an idle burst? Why should an idle be bursty?
• The burst is never cut short because neither exec_timer writer (the realtime timer thread and force_cycle_counter_reload) raises it while the throttle is active; the throttle fires due timers itself on wakeup.
  • raises it means writing to exec_timer?
  • Isn't writing to exec_timer the only way to fire due timers (because ppc_exec_inner calls process_events() only when exec_timer is true?
• In non-realtime mode the throttle is never active, so this single condition reduces to plain exec_timer handling and avoids an atomic load and branch on every instruction.
  • What single condition? The statement has two conditions exec_timer.load and g_icycles++ >= max_cycles.
  • The atomic load is exec_timer.load? How is it avoided?

2. ==================

You accidentally added a blank link here:

int get_icnt_factor()
{
    return icnt_factor;
}

<-----

3. ==================

Why does this line use g_icycles?
return g_icycles + (burst_ns >> icnt_factor) + 1
The line is only for throttling realtime mode. Is g_icycles used for realtime mode?

Maybe some of the last commits in my fork addresses this issue and some others? These are listed from earliest to latest (the relevant ones are marked with a bullet •):

• • ppcexec: Add ppc_exec functions for g_realtime mode (in progress).
• ppcexec: Add constexpr.
• • timermanager: Add has_next to process_timers.
• ppcexec: Use power_off instead of force_cycle_counter_reload.
• ppcexec: Remove 5 us offset from set_virt_time_ns.
• ppcexec: Don't allow g_instruction_period < 1.
• ppcexec: Change name of po_endian_switch.

These are untested.

4. ==================

What is DP3 in this comment?

// 6 ms per 16 ms window keeps DP3 healthy at ~7% host CPU.

Mac OS X Developer Preview 3 ?

5. ==================

Unnecessary spaces. Spaces for alignment should only be used for lists and tables and similar/related statements. burst_ns and sleep_ns are not related enough to be aligned.

            constexpr uint64_t burst_ns     = 6000000ULL;  // 6 ms
            const uint64_t sleep_ns = (slice_ns > IDLE_MAX_SLEEP_NS) ? IDLE_MAX_SLEEP_NS : slice_ns;

6. ==================

These changes should maybe be grouped together?

// Counter of guest accesses to memory-mapped devices, incremented by
// mmu_read_vmem/mmu_write_vmem. Used by ppcexec.cpp to detect when the
// guest is idling (spinning without touching any device).
extern uint64_t g_mmio_access_count;

/* set_g_idle_cpu_save */
extern void set_g_idle_cpu_save(bool enabled);

/* mark_host_input: the host event poller calls this for every input event */
extern void mark_host_input();

I understand they are defined in different source files but they are used for implementing a single feature.

[dingusdev]

4. Yes, that is Developer Preview 3

[joevt]

6. I see now that the variable declaration was separated by the function declarations so that's ok.

I rearranged and modified the @joevt and @probonopd and @mihaip ppcexec.cpp related commits in my fork (from latest to earliest):

joevt       ppcexec: Add ppc_exec functions for g_realtime mode (in progress).
joevt       ppcexec: Remove 5 us offset from set_virt_time_ns.
joevt       ppcexec: Don't allow g_instruction_period < 1.
mihaip      ppc: Model configurable CPU frequencies.
mihaip      ppcexec: Reduce interpreter register pressure.
probonopd   Realtime: Wake the interpreter from a dedicated timer thread.
probonopd   realtime: Throttle an idle guest in realtime mode to save host CPU.
probonopd   main: Add a --realtime command line flag.
probonopd   ppcexec: Change thread-accessed variables to atomic.
probonopd   timermanager: Add get_next_timeout_ns().
joevt       timermanager: Return time_now from process_timers.
joevt       ppcexec: Use power_off instead of force_cycle_counter_reload.
joevt       ppcexec: Change name of po_endian_switch.
joevt       ppcexec: Add constexpr.
joevt       ppcexec: Add list of AltiVec instructions.
joevt       ppcexec: Output address with message.
joevt       ppcexec: Comment illegal op in Open Firmware.

I understand that the mihaip commits are still draft.

I'll do some testing later.

[probonopd]

@joevt thanks for your thorough review. Let me preface this that I am new to this codebase and by no means an expert, so take everything below with a grain of salt. It is entirely possible that I am missing some things.

1) The || short-circuit and g_cycles accounting — you're right, this is a bug.

The base loop was:

if (g_cycles++ >= max_cycles || exec_timer) [[unlikely]]
    max_cycles = process_events();

where g_cycles++ is the left operand of || and therefore always evaluated, so g_cycles is incremented on every instruction. In non-realtime mode get_virt_time_ns() returns g_cycles << icnt_factor, so g_cycles is the virtual-time counter and must stay accurate.

Reordering to exec_timer.load(...) || g_cycles++ >= max_cycles so the timer-thread wakeup is checked first is wrong: when exec_timer is already true at the top of an iteration, short-circuit evaluation skips g_cycles++, so g_cycles is under-counted and non-realtime virtual time (and the decrementer / timers that derive from it) drifts downward. The reorder also contradicts the comment alongside it, which claims it avoids an atomic load per instruction — in fact it now does an exec_timer.load() on every instruction and only made the g_cycles++ conditional.

The reason for checking exec_timer first was to let the realtime timer thread's wakeup be handled promptly in realtime mode rather than waiting for the burst budget to expire. A cleaner way to get that without overloading this one condition is the split ppc_exec_inner approach (separate functions for realtime vs non-realtime): the non-realtime path keeps the original correct g_cycles++ >= max_cycles || exec_timer (plain volatile read, no atomic per instruction), and the realtime path doesn't need g_cycles for time at all since virtual time is the wall clock — it can budget the burst straight from wall clock instead of g_cycles + (burst_ns >> icnt_factor). That also resolves point 3.

2) Stray blank line. Agreed — there is an extra blank line after get_icnt_factor() that can be removed.

3) return g_cycles + (burst_ns >> icnt_factor) + 1. In realtime mode g_cycles is not the virtual-time source (get_virt_time_ns() uses the wall clock), but the interpreter loop still keeps g_cycles as a monotonically increasing per-call budget counter: process_events() returns g_cycles + budget and the loop runs until g_cycles++ >= max_cycles. So the expression sets the next wakeup to "current cycles plus ~6 ms worth of instructions" (burst_ns >> icnt_factor converts the 6 ms window into an instruction count via the instruction-period scale icnt_factor). It works, but as noted in (1) it is fragile because g_cycles is not reliably incremented, and the realtime ppc_exec_inner split budgets that path more directly.

4) DP3. Sorry for the unclear abbreviation — "DP3" was just shorthand for the Mac OS X Developer Preview 3 Installation CD-ROM image.

5) Alignment spaces. Agreed, the burst_ns = padding is gratuitous since burst_ns and sleep_ns are not a related list. The extra spaces can be dropped.

6) Grouping the feature declarations. Agreed — g_mmio_access_count, set_g_idle_cpu_save(), and mark_host_input() implement the one idle-throttle feature and can sit together in ppcemu.h. The start/stop_realtime_timer_thread pair is a separate feature (the timer thread) and can stay grouped separately.

For the loop rework, the realtime ppc_exec_inner split plus timermanager: Add has_next to process_timers from your fork look like the cleaner foundation to build the throttle / timer-thread changes on top of, rather than keeping the single combined loop.

Since you are already reworking things - would you like to take it from here?

[joevt]

Since you are already reworking things - would you like to take it from here?

I can do that. Is there anything in my changes for these commits that you think should be changed?

I removed burst_ns from process_events_real (the realtime version of process_events) because it was based on virtual time. I hope it's not too big of a departure from your intent. process_events_real doesn't return a time for ppc_exec_inner to use as a deadline for the next time to process events. Instead, we rely on realtime_timer_thread_fn to set exec_timer at the appropriate time.

I think I should revert my Return time_now from process_timers. commit or at least modify it so that if it does process timers then it should return an updated time_now value instead of the originating time_now value, because some timers may take a long time and also, there is little cost in retrieving an updated time since timers don't happen often in relation to CPU instructions. I changed that commit to timermanager: Return next_ns from process_timers.

process_events_real doesn't have a IDLE_MIN_SLEEP_NS ? I wonder if one should be added? Should sleeping happen for slice_ns that is really small? I have to add a check for negative slice.

I've updated the commits in my fork. Still need to do testing.